WA-VERIFY-054: Add script/pr_client_impact_check to verify PR body has Client impact section

[kitcommerce]

Summary

Adds script/pr_client_impact_check, a portable POSIX-shell script that
verifies a given PR's body contains a "Client impact" section.

What it does

• Accepts a PR number or full GitHub PR URL as its sole argument
• Fetches the PR body from workarea-commerce/workarea via gh pr view
• Performs a case-insensitive search for a "Client impact" heading / label
• Exits 0 when the section is present; exits 1 with a clear error
  message when it is missing
• Is read-only — performs no mutations on the repo or any issue/PR

Usage

./script/pr_client_impact_check 919
./script/pr_client_impact_check https://github.com/workarea-commerce/workarea/pull/919

Closes #919

Client impact

None. This is a developer tooling / verification script only; it is not
shipped with the gem and has no effect on runtime behavior.

Verification Plan

# Pass case — PR with a Client impact section (e.g. PR #985)
./script/pr_client_impact_check 985
# Expected: OK: PR #985 contains a 'Client impact' section.
# Exit code: 0

# Pass case — full URL
./script/pr_client_impact_check https://github.com/workarea-commerce/workarea/pull/985
# Expected: OK: PR #985 contains a 'Client impact' section.
# Exit code: 0

# Fail case — test with a PR that has no Client impact section (use a test number)
# You can also test via synthetic shell check:
PR_BODY='## Summary\nNo impact section here' sh -c '
  echo "$PR_BODY" | grep -qi "client impact" && echo FOUND || echo "NOT FOUND (expected)"
'
# Expected: NOT FOUND (expected)

[kitcommerce]

Architecture Review — PR #986 Wave 1

Verdict: PASS_WITH_NOTES
Severity: LOW

Summary

The script is well-structured and achieves its single goal cleanly. The script/ location is idiomatic for GitHub-flow projects. Exit codes (0/1/2) are well-defined and documented. The design is appropriately read-only with no side effects.

Findings

• 2>&1 in gh subshell (LOW): PR_BODY="$(gh pr view ... 2>&1)" captures stderr into the variable. If gh emits a warning on stderr while exiting 0 (e.g. auth nag), that warning text ends up in $PR_BODY and could theoretically confuse the grep check. Since the error path is already handled by || { ... exit 1 }, the 2>&1 redirect is unnecessary. Recommend removing it so stderr flows directly to the terminal.

• Hardcoded REPO (informational): REPO="workarea-commerce/workarea" is intentional for a project-specific enforcement script. No change required — noted for completeness.

• No architectural concerns with control flow, separation of concerns, or output contract. Pattern-matching logic is isolated and commented.

[kitcommerce]

Simplicity Review — PR #986 Wave 1

Verdict: PASS

Summary

85 lines, most of which are comments explaining intent. The executable logic is ~20 lines. Every comment earns its place: the PATTERN comment explains which Markdown heading styles are matched, the URL parsing comment walks through the extraction, and the usage function gives concrete examples. Easy to read, easy to maintain.

Findings

• POSIX character-class pattern (informational): [Cc][Ll][Ii][Ee][Nn][Tt][ ][Ii][Mm][Pp][Aa][Cc][Tt] — the [ ] bracket (space + tab) is subtle but intentional. The accompanying comment explicitly justifies why -i/-E flags are avoided. No change needed; the approach is correct and well-explained.

• No unnecessary complexity. URL parsing, empty-body guard, and error messaging are all proportionate to the problem.

• Nothing to simplify further without sacrificing portability or clarity.

[kitcommerce]

Security Review — PR #986 Wave 1

Verdict: PASS_WITH_NOTES
Severity: LOW

Summary

Script is read-only with no mutations. No credentials or secrets are embedded. The gh CLI handles authentication externally. Quoted variable expansions prevent word-splitting and glob issues throughout. No shell injection vectors identified.

Findings

• Weak numeric validation for PR_NUM (LOW): The [0-9]* case arm matches any string starting with a digit (e.g. 123foo). That value is passed directly to gh pr view "$PR_NUM" — quoted, so no shell injection — but gh would receive a malformed PR number and produce an error. Suggest tightening to ^[0-9]+$ validation (or a case with [0-9][0-9]* and a trailing anchor via expr). In practice the risk is limited to a confusing error message rather than a security breach.

• 2>&1 stderr capture (LOW, shared with Architecture finding): If gh writes a warning to stderr while still exiting 0, that text ends up in $PR_BODY. Worst case: a warning message coincidentally containing "client impact" would produce a false-positive OK exit. Probability is negligible but worth fixing by removing 2>&1.

• No injection risk from echo "$PR_BODY" | grep "$PATTERN" — both variables are properly quoted, and PATTERN contains no user-controlled content.

• No privilege escalation, no network writes, no file writes. Fully read-only. ✅

[kitcommerce]

Rails Conventions Review — PR #986 Wave 1

Verdict: PASS

Summary

This is a POSIX shell script, not Ruby/Rails code, so most Rails conventions do not apply. The relevant conventions that do apply are all satisfied.

Findings

• script/ directory placement ✅ — Rails projects conventionally use script/ for developer tooling. This is the right location.

• Naming convention ✅ — pr_client_impact_check is snake_case, consistent with other scripts in this directory.

• No test coverage (informational) — A shell script in script/ is not expected to have a corresponding RSpec spec. The PR body includes a Verification Plan section with manual test steps covering pass, fail, and URL-input cases. Acceptable for tooling scripts.

• No gem runtime impact ✅ — Script is not loaded by the gem, not required anywhere in the Rails boot path, not included in the gemspec. Purely developer tooling.

• No Rails conventions violated. ✅

[kitcommerce]

Wave 1 Review Summary — PR #986

Reviewer	Verdict	Severity
Architecture	PASS_WITH_NOTES	LOW
Simplicity	PASS	—
Security	PASS_WITH_NOTES	LOW
Rails Conventions	PASS	—

Overall: PASS_WITH_NOTES — No blocking issues. All findings are LOW severity and informational.

Consolidated Notes for Author

Two overlapping LOW findings across Architecture and Security reviewers (no need to address both separately):

1. Remove 2>&1 from the gh subshell — stderr should flow to the terminal, not be captured in $PR_BODY. The || { ... exit 1 } block already handles command failure; the redirect is unnecessary and creates a minor false-positive risk.

2. Tighten PR number validation — [0-9]* in the case arm allows non-numeric trailing characters. Consider validating the extracted PR_NUM is strictly numeric before passing to gh.

Both are LOW and non-blocking. Author may address in a follow-up or as part of this PR at their discretion.

---

✅ Ready for Wave 2 review. Issue #919 remains at status:ready-for-review.

[kitcommerce]

Database Review — PR #986 Wave 2

Verdict: PASS

No database interaction. Shell utility script only — N/A for database review.

[kitcommerce]

Rails Security Review — PR #986 Wave 2

Verdict: ✅ PASS

Findings

#	Area	Result	Notes
1	Shell injection (PR_ARG/PR_NUM)	✅ Safe	Input parsed via case pattern matching + parameter expansion. No eval, no unquoted expansions. [0-9]* guard on numeric input.
2	gh pr view "$PR_NUM" with adversarial input	✅ Safe	Double-quoted argument — no word splitting or glob expansion. Adversarial payloads (e.g. "; rm -rf /") passed as a literal string to gh, which rejects invalid PR identifiers.
3	Credential / token exposure	✅ None	gh CLI never prints auth tokens in error output. 2>&1 captures stderr into PR_BODY, but that variable is only consumed by grep -q (no echo of raw body). Diagnostic messages only print PR_NUM (user-supplied).
4	Trust of external data (PR body)	✅ Safe	PR body is passed through `echo

Minor Observations (non-blocking)

• [0-9]* case match is loose: Accepts strings like 9foo or 123/../../etc. Not a security issue — gh pr view rejects invalid identifiers gracefully — but a stricter numeric validation (e.g., checking all digits) would be cleaner.
• 2>&1 in command substitution: If gh fails, the error message lands in PR_BODY and gets grep'd. A PR body error message containing "client impact" could theoretically produce a false positive. Functional concern, not security.

Summary

This is a read-only POSIX shell script with no privileged operations. All user input is properly quoted, parsed via safe shell constructs, and passed as a single argument to gh. External data (PR body) is only consumed by grep, never executed. No credentials are exposed. No injection vectors found.

---

Reviewed by: automated Rails Security reviewer (Wave 2)

[kitcommerce]

Test Quality Review — PR #986 Wave 2

Verdict: PASS_WITH_NOTES

The script is functionally correct and handles its core purpose well. Test quality gaps are worth documenting, but none block merge given this is developer tooling (not shipped application code).

---

Findings

F1 — No test suite shipped (moderate)
The PR adds no automated test file (bats, shunit2, etc.). All verification is manual shell commands in the PR description. For a CI-gate script, even a small bats test file covering pass/fail/empty-body would significantly raise confidence.

F2 — CI testability gap (moderate)
gh pr view is called directly with no override mechanism (e.g., no GH_CMD env var, no stdin mode). Running end-to-end in CI requires a live GH_TOKEN with repo access. Consider supporting GH_CMD override or a --body-stdin flag so CI can inject a synthetic body without real GitHub access.

F3 — [0-9]* case-match is over-permissive (low)
The pattern [0-9]* matches any string beginning with a digit — e.g., 123abc passes the case match and gets forwarded to gh, yielding a confusing ERROR: Failed to fetch PR #123abc rather than an input-validation error. Suggest tightening to [0-9][0-9]*) and adding a post-case guard: case "$PR_NUM" in *[!0-9]*) echo "ERROR: invalid PR number" >&2; usage;; esac.

F4 — 2>&1 in command substitution mixes concerns (low)
gh stderr is captured into PR_BODY. Functionally safe — gh exits non-zero on failure so the || handler fires before PR_BODY is ever used — but conceptually misleading. Prefer capturing stderr separately to improve maintainability.

F5 — Verification Plan tests different grep semantics than the script (low)
The inline synthetic test uses grep -qi (the -i flag), but the actual script uses POSIX character classes ([Cc][Ll]...). Behaviorally equivalent in practice, but the verification snippet is testing a code path that doesn't exist in the script. The plan should mirror the actual command used.

F6 — Verification Plan omits edge-case paths (low)
Acceptance criteria cover pass (number), pass (URL), and a partial fail. Not covered: empty body, non-numeric argument (e.g. foobar), auth failure. These handlers exist in the code — they just aren't exercised by any documented test.

---

What's working well

• Both primary exit paths (0/1) are implemented and message clearly
• Empty-body guard is present and correct ([ -z "$PR_BODY" ])
• PATTERN covers all common Markdown heading styles via substring grep — ##, **bold**, plain text, trailing colon all match
• Error messages are directed to stderr correctly
• The || handler is the right idiom for catching gh failures

---

Recommendation

Consider adding a minimal bats test file (3–5 tests) with a GH_CMD override that echoes a synthetic PR body. This would resolve F1 and F2 in one shot and make the script verifiable in any CI environment without credentials.