fix: harden bash scripts against shell injection and improve error handling

[pierluigilenoci]

Summary

Security hardening and robustness improvements for all bash scripts in scripts/bash/.

Shell injection prevention

• common.sh / get_feature_paths(): Replace heredoc with single-quoted values (REPO_ROOT='$val') with printf '%q' quoting. This properly escapes special characters in branch names and file paths, preventing shell injection when the output is eval'd by consumers.

Safe eval pattern

• check-prerequisites.sh, setup-plan.sh, update-agent-context.sh: Capture get_feature_paths output into a variable before eval, with error checking (|| { error; exit 1 }). Previously, a bare eval $(get_feature_paths) would silently proceed with unset variables if get_feature_paths failed.

Error propagation

• common.sh / find_feature_dir_by_prefix(): Return error (return 1) when multiple spec directories match the same numeric prefix, instead of silently falling back to a potentially incorrect directory.
• update-agent-context.sh / update_specific_agent(): Add || return 1 to all update_agent_file calls so failures are propagated to the caller instead of silently swallowed.

AGENTS.md deduplication

• update-agent-context.sh / update_all_existing_agents(): Replace repetitive if-blocks with a declare -A updated_paths associative array keyed by realpath. This prevents writing to AGENTS.md multiple times when AMP_FILE, KIRO_FILE, and BOB_FILE all resolve to the same file. Also use $AGENTS_FILE variable consistently instead of hardcoding $REPO_ROOT/AGENTS.md.

Trap safety

• update-agent-context.sh / cleanup(): Disarm traps (trap - EXIT INT TERM) at the start of the cleanup function to prevent re-entrant signal handling loops.

JSON output safety

• check-prerequisites.sh, create-new-feature.sh, setup-plan.sh: Use jq for JSON construction when available (properly escapes special characters in values), with automatic fallback to printf when jq is not installed. Added has_jq() helper in common.sh.

Fix misleading export

• create-new-feature.sh: Replace export SPECIFY_FEATURE=... (which has no effect on the parent shell) with an informational message telling the user how to persist the variable themselves.

Test plan

• Run bash -n syntax check on all 5 modified scripts
• Test check-prerequisites.sh --json and --paths-only --json with and without jq installed
• Test create-new-feature.sh --json with and without jq installed
• Test setup-plan.sh --json with and without jq installed
• Test update-agent-context.sh with a repo that has AGENTS.md — verify it is written only once
• Test with a branch name containing special characters (e.g., spaces, quotes)
• Test find_feature_dir_by_prefix with multiple matching spec directories — verify error is raised

[pierluigilenoci]

Requesting review from the main contributors to the bash scripts:

@localden @Mearman @vburckhardt

Could you please review these security and robustness improvements to the bash scripts in scripts/bash/?

[Copilot]

Pull request overview

This PR hardens the bash workflow scripts under scripts/bash/ by reducing shell-injection risk around eval, improving error propagation, and making JSON output construction safer when jq is available.

Changes:

• Harden get_feature_paths() output by switching to printf '%q' quoting and adding has_jq() in common.sh.
• Replace eval $(get_feature_paths) with a safer capture-then-eval pattern plus failure handling across scripts.
• Improve update-agent-context.sh robustness (dedupe AGENTS.md updates, trap disarming, better propagation in update_specific_agent).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
scripts/bash/common.sh	Safer quoting for eval-consumed output; add has_jq(); tighten error behavior for ambiguous prefix matches.
scripts/bash/check-prerequisites.sh	Safer eval pattern + optional jq JSON construction for --json and --paths-only.
scripts/bash/setup-plan.sh	Safer eval pattern + optional jq JSON output.
scripts/bash/create-new-feature.sh	Replace misleading export with user guidance; optional jq JSON output.
scripts/bash/update-agent-context.sh	Safer eval pattern, dedupe AGENTS.md updates, trap disarming, improved per-agent failure propagation.

Comments suppressed due to low confidence (1)

scripts/bash/check-prerequisites.sh:166

• The main --json output uses jq -n without compact mode, which changes output from the prior single-line JSON (printf) to multi-line pretty-printed JSON. If external tooling expects one JSON object per line, this can break it; consider using jq compact output (e.g., -c).

        jq -n \
            --arg feature_dir "$FEATURE_DIR" \
            --argjson docs "$json_docs" \
            '{FEATURE_DIR:$feature_dir,AVAILABLE_DOCS:$docs}'

---

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

[mnriem]

Please address Copilot feedback and if not applicable please explain why

[pierluigilenoci]

@mnriem @dhilipkumars I've addressed the Copilot review feedback:

• Added jq -c (compact mode) to all jq -n calls to preserve single-line JSON output (check-prerequisites.sh, setup-plan.sh, create-new-feature.sh)
• Replaced command -v jq with has_jq() in create-new-feature.sh for consistency with common.sh
• Shell-escaped BRANCH_NAME with printf '%q' in the persistence hint

Regarding the || return 1 suggestion on update_all_existing_agents: I've explained in the inline reply why the current best-effort approach is intentional.

Could you please take another look? Thanks!

[mnriem]

In-depth review: bash security hardening PR

Summary

The intent of this PR is solid — it addresses real shell injection risks, improves error propagation, and fixes a genuine multi-write bug with AGENTS.md. However, there are two blocking issues that need to be fixed before merge.

---

🔴 Blocking Issues

1. create-new-feature.sh calls undefined has_jq (Regression)

create-new-feature.sh does not source common.sh. The first commit correctly used an inline command -v jq >/dev/null 2>&1, but the second commit (abf57c2) replaced it with has_jq based on an incorrect Copilot automated review suggestion that claimed the script "already sources common.sh". It doesn't. This will cause a runtime failure (bash: has_jq: command not found) under set -e when --json is used.

2. declare -A requires bash 4+ (macOS incompatibility)

The declare -A updated_paths in update_all_existing_agents() introduces the first bash 4+ dependency in the codebase. macOS ships with bash 3.2 (Apple won't ship GPLv3 software), so this will fail on any Mac using the system bash. A regular indexed array with a linear scan would work identically for ~20 agents and be compatible with bash 3.2.

---

🟡 Non-blocking observations

• find_feature_dir_by_prefix now returns error on multiple matches: Previously it returned a possibly-wrong fallback path. This is a correctness improvement, and callers are updated to handle the error. No backwards compat concern since the old behavior was silently wrong.

• AGENTS.md label changes from "IBM Bob" to "Codex/opencode": Due to the dedup logic, the label applied to AGENTS.md changes (old code: last-writer-wins → "IBM Bob"; new code: first-writer-wins → "Codex/opencode"). This is cosmetic (label is only used in log messages), but noted for awareness.

• export SPECIFY_FEATURE removal: Correct fix — export in a subprocess doesn't affect the parent shell, so the old behavior was misleading. The new message telling users how to persist it themselves is better UX.

• printf '%q' quoting in get_feature_paths: This is the core security fix and it's well done. The old heredoc with single-quoting was vulnerable to paths/branches containing single quotes.

• jq with -cn for JSON construction: Good approach with clean fallback. The -c flag preserves backwards-compatible single-line output.

• Trap disarming in cleanup(): Good defensive improvement against re-entrant signal handling.

• update_specific_agent || return 1 additions: Correct — propagates failures to the caller.

---

Security Assessment

The security improvements are genuine and well-motivated:

• ✅ Shell injection via printf '%q' quoting is properly addressed
• ✅ eval pattern is hardened with failure checking
• ✅ JSON construction with jq properly escapes special characters
• ✅ No new injection vectors introduced
• ✅ No SSRF, path traversal, or privilege escalation concerns
• ⚠️ Existing /tmp/agent_update_*_$$ pattern (PID-based temp files) has known weaknesses but is pre-existing and out of scope

---

Recommendation

Request changes — fix the two blocking issues above, then this is ready to merge.

[mnriem]

Bug (Regression): create-new-feature.sh does not source common.sh, so has_jq is undefined here. This will produce bash: has_jq: command not found at runtime when --json is used, causing the script to fail under set -e.

The earlier Copilot automated review comment incorrectly stated "This script already sources common.sh" — it doesn't. The first commit used an inline command -v jq >/dev/null 2>&1 which was self-contained and correct. The second commit (abf57c2) replaced it with has_jq per that incorrect suggestion, introducing this regression.

Fix options (pick one):

1. Add source "$SCRIPT_DIR/common.sh" after the SCRIPT_DIR= line (line 168).
2. Revert to the inline command -v jq >/dev/null 2>&1 check.
3. Define has_jq() locally in this script.

Option 1 is cleanest but requires auditing that sourcing common.sh doesn't introduce side effects (function name clashes with the local find_repo_root, get_highest_from_specs, etc.). Option 2 is safest and most minimal.

[mnriem]

Backwards compatibility (macOS): declare -A (associative arrays) requires bash 4.0+. macOS ships with bash 3.2 by default (Apple won't upgrade past GPLv2-licensed versions). Users running these scripts with the system /bin/bash on macOS will get:

bash: declare: -A: invalid option

The existing codebase has no bash 4+ features, so this would be the first incompatibility.

Suggested alternatives that work in bash 3.2:

1. Use a regular array of already-updated paths and grep against it:

_updated_paths=()
update_if_new() {
    local file="$1" name="$2"
    [[ -f "$file" ]] || return 0
    local real_path
    real_path=$(realpath "$file" 2>/dev/null || echo "$file")
    # Check if already processed
    local p
    for p in "${_updated_paths[@]+"${_updated_paths[@]}"}"; do
        [[ "$p" == "$real_path" ]] && return 0
    done
    update_agent_file "$file" "$name" || return 1
    _updated_paths+=("$real_path")
    found_agent=true
}

2. Or use a simple colon-separated string with pattern matching.

The linear scan is fine here — there are only ~20 agents, so O(n²) is irrelevant.

[mnriem]

Behavioral change: Previously, when AGENTS.md existed, update_all_existing_agents would write to it multiple times — once as "Codex/opencode", then as "Kiro CLI", then as "IBM Bob" (last writer wins). After this change, the dedup logic means AGENTS.md is only written once with the label "Codex/opencode", and the subsequent AMP/KIRO/BOB entries are skipped.

This is clearly the correct fix (multi-write was a bug), but worth noting that the effective label applied to AGENTS.md changes from "IBM Bob" (old last-writer-wins) to "Codex/opencode" (new first-writer-wins). This is fine since the label is just for logging, but calling it out for awareness.

[mnriem]

Good security fix. The old heredoc approach was vulnerable to shell injection when $repo_root or $current_branch contained single quotes or shell metacharacters (e.g., a path like /Users/O'Brien/project would break out of single quotes). printf '%q' properly handles all cases.

[mnriem]

Good catch changing this from a silent fallback to return 1. The old behavior of printing an error to stderr but then returning a potentially wrong path was a subtle correctness bug — callers would silently operate on the wrong spec directory.

Minor note: This is a backwards-incompatible behavior change for callers that relied on always getting a path back (even if wrong). Since the only callers are get_feature_paths (which now checks the return code) and the eval pattern now handles failures, this is properly guarded.

[mnriem]

Good improvement. The old trap cleanup EXIT INT TERM without disarming could cause the cleanup function to re-enter itself if a signal arrives during cleanup (e.g., user hits Ctrl+C while rm -f is running → TERM fires → calls cleanup again → race condition).

[mnriem]

Minor inconsistency: The stderr message correctly uses printf '%q' to shell-escape the branch name, but the text-mode output on line 321 still uses raw interpolation:

echo "# To persist in your shell: export SPECIFY_FEATURE=$BRANCH_NAME"

If a branch name contained spaces or shell metacharacters, the stderr hint would be safe to copy-paste but the stdout hint would not. In practice, BRANCH_NAME is sanitized by clean_branch_name() earlier in the script (only [a-z0-9-]), so this isn't exploitable — but it's worth aligning for consistency.

Also: the export SPECIFY_FEATURE=... removal is a correct behavioral change — export in a subshell has no effect on the parent shell, so the old code was misleading.

[mnriem]

See comments above

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

scripts/bash/check-prerequisites.sh:174

• The printf-based JSON fallback for the final --json output doesn't escape FEATURE_DIR (and would also be fragile if any future doc names contain quotes/backslashes). This can yield invalid JSON in repos/paths with special characters when jq is unavailable. Consider adding JSON escaping for the fallback or requiring jq for JSON mode.

            json_docs=$(printf '"%s",' "${docs[@]}")
            json_docs="[${json_docs%,}]"
        fi
        printf '{"FEATURE_DIR":"%s","AVAILABLE_DOCS":%s}\n' "$FEATURE_DIR" "$json_docs"

---

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