docs(#929): add shell scripting defensive patterns to AGENTS.md - #930
docs(#929): add shell scripting defensive patterns to AGENTS.md#930fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
Add section 9 with five defensive patterns for agents creating or modifying .sh files, addressing recurring shell scripting bugs found during code review of PR #918 (13 review iterations, 8-10 bugs): - 9a stdin handling: save piped input before branching logic - 9b jq null safety: guard output with // empty or // "default" - 9c GHA output sanitization: pass untrusted values through _gha_sanitize before echoing - 9d stderr preservation: require inline comments justifying 2>/dev/null usage - 9e exit code propagation: capture and return $? from inner commands in wrapper functions These patterns complement issue #131 (review-time shell pitfall checks) without overlap — #131 covers trailing newlines, set -u, mktemp, and pipe-subshell scoping while this covers stdin consumption, jq null, GHA injection, stderr, and exit codes. Note: pre-commit could not run in sandbox (network policy blocked hook environment fetch). Post-script runs authoritative pre-commit. Closes #929
|
🤖 Finished Review · ✅ Success · Started 12:53 AM UTC · Completed 1:09 AM UTC Commit: |
ReviewFindingsMedium
Low
|
|
|
||
| ```bash | ||
| # Wrong — only the first branch consumes stdin; the second gets nothing. | ||
| if [ "$mode" = "a" ]; then |
There was a problem hiding this comment.
[medium] technical-accuracy
Section 9a's 'wrong' example does not demonstrate the stdin consumption failure it claims. In a standard if/else, only one branch executes, so stdin is never consumed by the untaken branch. process_b would correctly receive stdin when $mode is not 'a'. The capture-first pattern is sound defensive advice, but the example should be rewritten to show a scenario where the bug actually manifests.
Suggested fix: Rewrite the 'wrong' example to show a scenario where stdin consumption actually fails (e.g., two sequential commands reading stdin, or a while-read loop), or adjust comments to explain the pattern guards against future refactors.
| # Wrong — if .count is missing, count becomes the string "null". | ||
| count=$(echo "$json" | jq -r '.count') | ||
| total=$(( count + 1 )) # arithmetic error | ||
|
|
There was a problem hiding this comment.
[low] technical-accuracy
Section 9b's comment '# arithmetic error' is only accurate under set -u / set -euo pipefail. Without strict mode, $(( count + 1 )) when count='null' silently evaluates to 1 (null treated as unset variable name). The comment should clarify the behavior depends on shell strictness settings.
Suggested fix: Change the comment to '# errors under set -u; silently gives 1 otherwise (null treated as unset variable name)' or similar.
| found in code review (see PR #918) and are independently valuable | ||
| alongside the review-time shell pitfall checks proposed in issue #131. | ||
|
|
||
| ### 9a. stdin handling |
There was a problem hiding this comment.
[low] heading-naming-convention
Sub-section headings use a numbered ### 9a. / ### 9b. pattern that departs from the single existing sub-heading convention (### Valid SKILL.md frontmatter fields, which is unnumbered). The numbered format is a reasonable choice for independent patterns but introduces a new convention.
Suggested fix: Consider renaming sub-headings to unnumbered descriptive titles: ### stdin handling, ### jq null safety, ### GHA output sanitization, ### stderr preservation, ### Exit code propagation.
Summary
.shfiles// emptyguards), GHA output sanitization (_gha_sanitize), stderr preservation (require justification for2>/dev/null), and exit code propagation (local rc=$?; return $rc)Testing
_gha_sanitizein ops libs,// emptyin jq expressions,local rc=0exit code pattern)Closes #929
Post-script verification
agent/929-shell-defensive-patterns)1787d5ca7ce8adb87ce46d211a834b31e9fa299a..HEAD)