fix(scripts): harden bash scripts — escape, compat, and error handling#1869
Conversation
- common.sh: complete RFC 8259 JSON escape (\b, \f, strip control chars) - common.sh: distinguish python3 success-empty vs failure in resolve_template - check-prerequisites.sh: escape doc names through json_escape in fallback path - create-new-feature.sh: remove duplicate json_escape (already in common.sh) - create-new-feature.sh: warn on stderr when spec template is not found - update-agent-context.sh: move nested function to top-level for bash 3.2 compat
…gent updates - common.sh: resolve_template now returns 1 when no template is found, making the "not found" case explicit instead of relying on empty stdout - setup-plan.sh, create-new-feature.sh: add || true to resolve_template calls so set -e does not abort on missing templates (non-fatal) - update-agent-context.sh: accumulate errors in update_all_existing_agents instead of silently discarding them — all agents are attempted and the composite result is returned, matching the PowerShell equivalent behavior
|
@mnriem @dhilipkumars, please take a look. |
There was a problem hiding this comment.
Pull request overview
Hardens the repository’s bash workflow scripts by improving JSON escaping, making template resolution behavior explicit under set -e, and improving update-agent error handling/compatibility (notably for macOS bash 3.2).
Changes:
- Strengthens
json_escape()to cover additional RFC 8259 escapes and strip remaining control characters. - Makes
resolve_templatereturn non-zero when not found; updates call sites to avoidset -eaborts and adds a missing-template warning. - Refactors
update-agent-context.shto avoid nested helper scope issues on bash 3.2 and to accumulate failures while updating all agents.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/bash/update-agent-context.sh | Moves update helper to top-level for bash 3.2, and accumulates per-agent update failures while attempting all updates. |
| scripts/bash/setup-plan.sh | Adjusts resolve_template call to tolerate “not found” under set -e. |
| scripts/bash/create-new-feature.sh | Removes duplicate JSON escape helper, tolerates missing template, and warns on stderr when creating an empty spec. |
| scripts/bash/common.sh | Enhances JSON escaping and changes resolve_template to return 1 on “template not found”; improves python3 result handling logic. |
| scripts/bash/check-prerequisites.sh | Ensures doc names are JSON-escaped in the jq-fallback JSON output path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback. If not applicable, please explain why
Move the python3 command substitution in resolve_template into an if-condition so that a non-zero exit (e.g. invalid .registry JSON) does not abort the function under set -e. The fallback directory scan now executes as intended regardless of caller errexit settings.
There was a problem hiding this comment.
Pull request overview
Hardens the Bash workflow scripts by improving JSON escaping, making template resolution safer under set -e, and improving macOS Bash 3.2 compatibility and error reporting—aligning these scripts with more robust/defensive execution in typical CI and developer shells.
Changes:
- Strengthen
json_escape()to cover additional RFC 8259 escapes and strip remaining control characters. - Change
resolve_template()to return non-zero when not found and update callers to tolerate “not found” underset -e. - Refactor
update-agent-context.shagent-update flow for Bash 3.2 compatibility and best-effort error accumulation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/bash/common.sh | Harden JSON escaping; make resolve_template safer with set -e and return 1 on “not found”. |
| scripts/bash/setup-plan.sh | Adjust template resolution call site to tolerate resolve_template returning non-zero. |
| scripts/bash/create-new-feature.sh | Remove duplicated json_escape; tolerate missing template and emit a warning. |
| scripts/bash/check-prerequisites.sh | Ensure doc names are JSON-escaped in the no-jq JSON output path. |
| scripts/bash/update-agent-context.sh | Move helper to top-level for Bash 3.2 and accumulate failures while continuing updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback. Hang in there we are getting there!
…level globals - _update_if_new now records the path and sets _found_agent before calling update_agent_file, so that failures do not cause duplicate attempts on aliased paths (AMP/KIRO/BOB -> AGENTS_FILE) or false "no agent files found" fallback triggers - Remove top-level initialisation of _updated_paths and _found_agent; they are now created exclusively inside update_all_existing_agents, keeping the script side-effect free when sourced
|
@mnriem Both new Copilot findings from the second review have been addressed in db9fb09:
Ready for re-review. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the repository’s bash workflow scripts by improving JSON escaping, making template resolution behavior explicit under set -e, and improving update-all error reporting/compatibility on older bash (macOS 3.2).
Changes:
- Strengthen
json_escape()to cover additional RFC 8259 escapes and strip remaining ASCII control characters to prevent malformed JSON output. - Make
resolve_template()return non-zero when no template is found, and update callers to tolerate “not found” underset -e. - Refactor
update-agent-context.shfor bash 3.2 compatibility and best-effort updates with an aggregated success/failure result.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/bash/common.sh | Harden JSON escaping and make resolve_template return code meaningful without breaking set -e execution. |
| scripts/bash/setup-plan.sh | Update template lookup to tolerate “not found” via ` |
| scripts/bash/create-new-feature.sh | Remove duplicate json_escape, tolerate missing template, and emit a clear warning when creating an empty spec. |
| scripts/bash/check-prerequisites.sh | Escape doc names in the jq-less JSON output path to avoid invalid JSON. |
| scripts/bash/update-agent-context.sh | Move helper to top-level for bash 3.2 and accumulate failures while still attempting all agent updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@pierluigilenoci Thank you! |
Summary
json_escape()incommon.shnow handles\b,\f, and strips remaining control characters (U+0000–U+001F), preventing malformed JSON outputresolve_templateincommon.shnow distinguishes between python3 returning empty results vs python3 failing entirelyresolve_template: returns 1 when no template is found (was always 0); callers updated with|| trueto preserve non-fatal behavior underset -echeck-prerequisites.shnow passes doc names throughjson_escapein the jq-fallback pathjson_escape:create-new-feature.shhad its own copy; now uses the one fromcommon.shcreate-new-feature.shemits a warning to stderr when the spec template is missingupdate-agent-context.shmoves the nestedupdate_if_newhelper to top-level (_update_if_new), avoiding scope issues on macOS default bashupdate_all_existing_agentsnow tracks failures with an accumulator (like the PowerShell equivalent) instead of silently discarding them — all agents are attempted and the composite result is returnedTest plan
specify initin a fresh project and verify all scripts are copied correctlycreate-new-feature.shwith and without a spec template present — verify warning on stderr when missingcheck-prerequisites.sh --jsonwith doc names containing special charactersupdate-agent-context.shwithout arguments (update-all path) and verify errors are reported/bin/bash --version) thatupdate-agent-context.shworks correctly