WA-VERIFY-055: Add script/pr_verification_plan_check

[kitcommerce]

Summary

Adds script/pr_verification_plan_check, a portable POSIX shell script that validates a given PR body contains:

1. A Verification or Verification Plan section heading (case-insensitive Markdown heading)
2. At least one fenced code block (...)

The script is read-only — it makes no repo mutations. It fetches PR body via gh pr view and exits 0 on success, exits 1 with a clear diagnostic message on failure. Accepts a PR number or a full GitHub PR URL.

Client impact

No client-facing changes. Dev/CI tooling only. Helps enforce consistent PR quality by making the verification plan requirement machine-checkable.

Verification Plan

Run the script against an existing PR with a verification section:

./script/pr_verification_plan_check 985

Expected output:

Fetching PR #985 from workarea-commerce/workarea ...
✓ Verification section found.
✓ Fenced code block found.

PR #985 passes verification plan checks.

Test with a URL form:

./script/pr_verification_plan_check https://github.com/workarea-commerce/workarea/pull/985

Test failure case (missing section) by checking a PR without the heading — script should print FAIL: and exit 1.

Closes #920

[kitcommerce]

Architecture Review — PR #987 Wave 1

Verdict: PASS_WITH_NOTES
Severity: LOW

Summary

The script has a clear single responsibility: validate PR body format. It fits naturally in script/ (Rails convention), is read-only, and exits cleanly with meaningful codes. A few low-severity architectural notes below.

Findings

• Hardcoded REPO: REPO="workarea-commerce/workarea" is baked in. Forks or future repo renames silently break this. Consider falling back to gh repo view --json nameWithOwner -q .nameWithOwner or accepting a --repo flag. LOW impact for now but limits portability.
• stderr/stdout mixing in BODY capture: gh pr view ... 2>&1 redirects stderr into $BODY. If gh emits a warning (not an error), it contaminates the body string and could cause false negatives on the grep checks. The || guard only catches non-zero exit; warnings on success are unguarded. Prefer 2>/dev/null and surface errors through the || path only.
• Diff line count discrepancy: The hunk header shows +1,82 but the provided diff is ~32 lines. If the full 82-line version contains additional logic, this review was conducted on an incomplete view. No blocking concern assuming the remainder is comments/whitespace.
• No usage docs in script/README or inline help beyond usage line: Minor — the usage comment at the top is adequate for a single-file utility.

[kitcommerce]

Simplicity Review — PR #987 Wave 1

Verdict: PASS

Summary

The script is concise, idiomatic POSIX sh, and does exactly what it says. No unnecessary abstractions, no extraneous dependencies beyond gh and standard grep. Easy to read in under a minute.

Findings

• The case-based URL/number parsing (${PR_INPUT##*/}) is clean and avoids a sed/awk subprocess. Good.
• Two-check structure (heading → code block) is linear and obvious. No hidden branching.
• Emoji checkmarks (✓) in stdout are a nice human-readable touch without adding complexity.
• Minor style note: grep -qi "^##\{0,\} *verification" uses BRE \{0,\} — portable but slightly unusual. grep -Eqi "^#{1,} *verification" (ERE) would be marginally more readable, though BRE works fine on all POSIX targets.
• No unnecessary temp files, no subshells beyond the one $() capture. Efficient.

[kitcommerce]

Security Review — PR #987 Wave 1

Verdict: PASS_WITH_NOTES
Severity: LOW

Summary

Script is read-only, uses gh CLI auth (no credentials in code), and validates PR_NUM to digits before use. No injection vectors found. One low-severity robustness issue noted.

Findings

• echo "$BODY" edge case: If the PR body starts with a flag-like string (e.g., -e, -n), some echo implementations (notably /bin/sh on some BSDs) may misinterpret it. This is a robustness issue rather than a true security risk since the value only flows to grep via pipe. Fix: use printf '%s\n' "$BODY" instead of echo "$BODY" for strict POSIX safety. LOW severity.
• Input validation is solid: The ''|*[!0-9]* case match ensures only numeric PR numbers reach gh. No injection path via $PR_NUM.
• No eval, no dynamic code execution: The body content is never executed or passed to a shell interpreter — only piped to grep. Clean.
• Hardcoded REPO is actually a security positive here: Prevents the script from being pointed at arbitrary repos by accident or misuse.
• set -e present: Ensures unexpected failures abort early rather than silently continuing.

[kitcommerce]

Rails Conventions Review — PR #987 Wave 1

Verdict: PASS

Summary

This is a shell utility, not Ruby/Rails code, so Rails-specific conventions have limited direct applicability. What can be evaluated: placement, executable bit, and fit within the Rails project structure. All are correct.

Findings

• script/ placement is conventional: Rails has used script/ for utility executables since Rails 2.x. This is exactly the right location for a developer tool script.
• Executable bit set (100755): Correct — script is directly runnable without sh prefix.
• No Rake task provided: A Rake wrapper (rake verify:pr_check PR=987) could improve discoverability for Rails developers who default to rake. Not required, but worth considering for a follow-up.
• No test coverage for the script: The Rails project likely has a test suite; shell script behavior could be validated with a minimal Bats or inline test harness. Not blocking, but a gap worth noting for long-term maintainability.
• No dependency on Rails internals: The script is pure shell + gh CLI. Zero risk of Rails version coupling or Bundler conflicts.

[kitcommerce]

🏁 Wave 1 Review Summary — PR #987

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

Overall Wave 1 Result: ✅ PASS WITH NOTES

All findings are LOW severity and non-blocking. Key notes for author consideration (not required for merge):

1. REPO hardcoded — consider making it auto-detected or parameterizable for fork portability.
2. 2>&1 in BODY capture — prefer 2>/dev/null; let the || path handle errors.
3. echo "$BODY" → printf '%s\n' "$BODY" — defensive POSIX hygiene.
4. Optional follow-ups: Rake wrapper task; shell script test coverage (Bats or similar).

Status: Advancing to Wave 2. PR remains status:ready-for-review.

[kitcommerce]

Rails Security Review — PR #987 Wave 2

Verdict: ✅ PASS

Findings

1. Shell injection on $PR_NUM — SAFE
The case "$PR_NUM" in ''|*[!0-9]*) gate ensures only pure digit strings reach gh pr view. Any metacharacters (semicolons, backticks, pipes) trigger immediate exit. Variable is also properly double-quoted.

2. URL parsing with adversarial input — SAFE
${PR_INPUT##*/} extracts the suffix after the last /, but the result passes through strict numeric validation. URLs with embedded shell metacharacters (e.g., http://evil.com;rm -rf /pull/123;whoami) would extract 123;whoami, which fails the digits-only check.

3. 2>&1 credential exposure — LOW RISK
Stderr captured into $BODY could theoretically contain auth hints from gh failures, but $BODY is never echoed on failure — the || { ... } block prints a generic message and exits. No credential leakage path.

4. grep pattern safety — SAFE
grep is a pure pattern matcher; adversarial PR body content cannot cause injection.

5. set -e behavior — SAFE
grep calls are inside if conditions (exempt from set -e). The gh call uses explicit || { exit 1 } handling.

Minor note: echo "$BODY" could misinterpret content starting with -n/-e as echo flags on some shells. printf '%s\n' "$BODY" would be marginally more robust, but this is cosmetic (affects match accuracy, not security).

---

Automated review by Rails Security reviewer (Wave 2)