Add script/pr_body_check to validate PR body sections

[kitcommerce]

Fixes #925

Client impact

None (developer tooling only).

Verification Plan

./script/pr_body_check <PR_NUMBER>

[kitcommerce]

Architecture Review

Verdict: PASS

script/pr_body_check is correctly placed at the top-level script/ alongside other developer utilities. Single responsibility: validate PR body sections via gh CLI. The script is acceptably scoped for a v1 — it handles PR numbers and implicitly uses the current repo context.

Note: PR #1122 introduces a second script/pr_body_check (133-line POSIX sh with URL parsing and auth preflight). These two PRs must not both be merged as-is — the later implementation should either replace this one or be reconciled. Recommend coordinating with PR #1122 before merge.

[kitcommerce]

Simplicity Review

Verdict: PASS

46 lines, set -euo pipefail, a single gh pr view call, two grep -qi checks — appropriately minimal. No unnecessary abstraction. The script does exactly what it needs to and nothing more.

Minor suggestion: a brief usage message when $# -lt 1 would improve ergonomics, but not a blocker.

[kitcommerce]

Security Review

Verdict: PASS

The PR argument ($PR_ARG) is passed directly to gh pr view as a positional argument — not interpolated into a shell command string — so shell injection risk is minimal. gh handles argument parsing safely.

PR body content is piped through grep -qi and not eval'd or echoed back into commands. No credential handling in the script itself (defers to gh auth context). set -euo pipefail is present.

No significant security concerns.

[kitcommerce]

Rails Conventions Review

Verdict: PASS (N/A)

Shell script only — no Ruby code. Rails conventions do not apply. Confirmed.

[kitcommerce]

Rails Security Review

Verdict: PASS

Findings

No findings. This PR adds a single bash script (script/pr_body_check) that invokes gh pr view and runs grep checks on the output. It contains:

• No Ruby code
• No Rails controllers, models, views, or routes
• No parameter handling, SQL queries, or template rendering
• No secrets, credentials, or authentication/authorization changes

The script properly quotes its variables ("$PR_ARG", "$PR_BODY", "$PASS"), uses set -euo pipefail for safe shell defaults, and directs error output to stderr. No security concerns from a Rails perspective.

Recommendations

None.

[kitcommerce]

Database Review

Verdict: PASS

No database changes in this PR. The diff adds script/pr_body_check, a shell script for validating PR body sections. No migrations, schema changes, query modifications, or ActiveRecord interactions present.

No findings.

[kitcommerce]

Test Quality Review

Verdict: PASS_WITH_NOTES

Findings

• MEDIUM — script/pr_body_check (lines 24–28, 31–35): The grep -qi check matches the required phrases anywhere in the PR body, not just as a section heading. A PR body containing prose like "this doesn't require a verification plan" or "see the existing verification plan in the wiki" would pass the check even though no dedicated section exists. This creates a false-positive risk that undermines the script's purpose.

• LOW — No guard for null/empty PR body. If gh pr view returns a null .body (e.g., a draft PR with an empty description), echo "$PR_BODY" | grep will receive an empty string and produce a false negative (missing section message) rather than a clear error. A [[ -z "$PR_BODY" ]] early-exit guard would make the behavior explicit.

• LOW — No automated test suite for the script. For a 46-line bash utility with simple grep logic, manual verification is acceptable. However, a test/script/pr_body_check_test (using bats or a simple shell harness) testing: (a) missing both sections → exit 1, (b) missing one section → exit 1, (c) both present → exit 0, (d) inline phrase match (not a heading) → expected behavior, would provide regression protection if the script is extended later.

Recommendations

1. Tighten the section detection: Match a markdown heading pattern rather than bare phrase — e.g., grep -qi '^##[[:space:]]*client impact' or pipe through a heading-aware check. This prevents inline mentions from satisfying the requirement.
2. Add an empty-body guard before the grep checks:

   if [[ -z "$PR_BODY" ]]; then
     echo "❌  PR body is empty — cannot validate sections." >&2
     exit 1
   fi

3. The script's own PR demonstrating both sections in its body is a good self-referential smoke test — this manual approach is sufficient for now, but worth automating if the script evolves.

[kitcommerce]

Accessibility Review — PASS

This PR adds script/pr_body_check, a bash shell script for developer CLI tooling. No user-facing UI, web output, HTML, or ARIA is involved.

Accessibility concerns (VoiceOver, Dynamic Type, color contrast, touch targets, etc.) do not apply to shell scripts. Nothing to flag here.

✅ Verdict: PASS — N/A for this change type.

[kitcommerce]

Frontend Review — Wave 3

This PR adds a bash shell script () for developer CLI tooling. There are no JavaScript, TypeScript, Stimulus, Turbo, Hotwire, or asset pipeline changes.

Frontend concerns do not apply. ✅ N/A — PASS

[kitcommerce]

Performance Review

Verdict: PASS

This is developer tooling, not hot-path production code — performance expectations are calibrated accordingly.

Analysis

The script is lean and well-structured:

• Single network call — one invocation fetches the PR body; no redundant I/O.
• No loops or accumulation — fixed, constant-time execution path regardless of PR body size.
• Memory — the PR body is held in a single shell variable; no arrays, no temp files.

Minor Note (not blocking)

The two grep checks use , which forks two processes per check (one for , one for ). A marginally cheaper alternative is a here-string:

grep -qi "client impact" <<< "$PR_BODY"

This avoids the fork. For a script that runs once per invocation on a developer machine, the difference is immeasurable — mentioning it only for completeness.

No performance concerns worth blocking on. ✅

---

Performance reviewer — automated wave review