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]

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.

[pierluigilenoci]

@mnriem @dhilipkumars @localden @Mearman @vburckhardt

All review findings have been addressed in 12fe594:

Finding	Status
has_jq undefined in create-new-feature.sh (regression from Copilot's incorrect suggestion)	Fixed — reverted to inline command -v jq check
declare -A incompatible with bash 3.2/macOS	Fixed — replaced with indexed array + linear scan
printf JSON fallback without escaping (check-prerequisites.sh, setup-plan.sh, create-new-feature.sh)	Fixed — added json_escape() helper
Stdout echo without shell-escaping (create-new-feature.sh)	Fixed — uses printf '%q' now
update_if_new best-effort vs fail-fast	Kept as-is (best-effort is intentional)
Behavioral change: first-writer-wins for deduped AGENTS.md	Acknowledged, correct behavior
Positive feedback (trap re-entrancy guard, return 1 on ambiguous path, printf '%q' in get_feature_paths)	No changes needed

All review threads have been resolved. Could you please take another look? Thanks!

[Copilot]

Pull request overview

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

---

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

scripts/bash/check-prerequisites.sh:166

• The main --json output path uses jq without compact mode, so the JSON object may be pretty-printed across multiple lines. Add -c so stdout remains a single-line JSON object like the previous printf output.

        jq -cn \
            --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. If not applicable, please explain why

[mnriem]

@pierluigilenoci Thank you!