feat(github): add async hooks for PR state tracking

.claude-plugin/marketplace.json

@@ -327,7 +327,7 @@
     {
       "name": "github",
       "description": "GitHub CLI installation, authentication, and workflow skill for Claude Code sessions. Consolidates gh-tool and github-auth-skill into a single plugin.",
-      "version": "0.1.15",
+      "version": "0.1.16",
       "author": {
         "name": "Nathan Heaps"
       },

.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/OVERALL-REPORT.md

@@ -0,0 +1,90 @@
+# Overall Review Report — PR #306
+
+**PR:** [nsheaps/ai-mktpl#306](https://github.com/nsheaps/ai-mktpl/pull/306) — feat(github): add async hooks for PR state tracking
+
+## Review Iteration Summary
+
+Initial review identified 4 hard-block categories (<70%). An iteration commit (`417d818`) addressed:
+
+- Operator precedence bug in main/master branch skip
+- Consolidated ~20 individual jq calls into a single jq invocation
+- Replaced O(N²) check diff with a single jq join-by-name call
+- Added input validation for owner/repo (path traversal prevention)
+- Added head_sha hex format validation
+- Made cache writes atomic (write to tmp, mv to final)
+- Set cache directory permissions to 700
+- Moved throttle check before heavy library sourcing
+- Validated check_interval is a positive integer before arithmetic
+- Removed dead code (pr_state_changes_summary, unused PLUGIN_NAME)
+- Updated PR body to remove stale hook-output.sh reference
+
+## Scores
+
+| Category         | Pre-Iteration | Post-Iteration | Status |
+| ---------------- | ------------- | -------------- | ------ |
+| Simplicity       | 58            | 82             | ⚠️     |
+| Flexibility      | 78            | 82             | ⚠️     |
+| Usability        | 81            | 84             | ⚠️     |
+| Documentation    | 87            | 89             | ✅     |
+| Security         | 62            | 85             | ✅     |
+| Repo Patterns    | 84            | 84             | ⚠️     |
+| Best Practices   | 62            | 82             | ⚠️     |
+| QA & Engineering | 47            | 72             | ⚠️     |
+| **Overall**      | **70**        | **82**         | ⚠️     |
+
+**Max overall with ⚠️ categories: 94%**
+
+## Remaining Non-Blocking Issues
+
+### Simplicity (82)
+
+- 🔕 3-file library split has no current reuse consumers (acceptable for future channels integration)
+- 🔕 Global `_PR_OWNER`/`_PR_REPO` variables as return mechanism (idiomatic bash, works correctly)
+
+### Flexibility (82)
+
+- 🔕 No way to restrict discovery to primary project only (filter out sibling repos)
+- 🔕 No cache TTL or pruning mechanism for stale cache files
+- 🔕 Per-hook timeout not configurable without editing hooks.json
+
+### Usability (84)
+
+- 🔕 Missing `jq`/`gh` silently disables tracking with no user feedback
+- 🔕 PR body change notification provides no content summary (agent must re-fetch)
+- 🔕 Label change messages emit raw JSON arrays instead of human-readable text
+
+### Documentation (89)
+
+- ℹ️ PR body now accurately reflects all changes (hook-output.sh reference removed)
+- ℹ️ Settings docs are comprehensive with inline examples
+- 🔕 README doesn't link to SKILL.md for discoverability
+
+### Security (85)
+
+- ℹ️ Owner/repo validated against `^[A-Za-z0-9._-]+$`
+- ℹ️ head_sha validated against hex SHA format
+- ℹ️ Cache directory created with 700 permissions
+- 🔕 TOCTOU race in throttle (low severity, concurrent double-check is harmless)
+- 🔕 Unquoted `${gh_hostname_flag}` relies on word splitting (works correctly, shellcheck warning)
+
+### Repo Patterns (84)
+
+- ⚠️ Hook output uses raw `echo` instead of `hook_respond`/`hook-logging.sh` pattern
+- 🔕 Libraries at `hooks/scripts/lib/` instead of plugin-level `lib/` (minor pattern deviation)
+
+### Best Practices (82)
+
+- 🔕 API calls 2-5 silently fall back to empty on failure, risking false-positive diffs
+- 🔕 No pagination on comments/reviews API (first page only)
+- 🔕 No rate-limit awareness or backoff
+
+### QA & Engineering (72)
+
+- ⚠️ No automated tests for ~400 lines of shell logic
+- 🔕 Comment/review count diffing assumes append-only (deletions not detected)
+- 🔕 PostToolUse `*` matcher retained (throttle mitigates but doesn't eliminate)
+- 🔕 Unrelated commits on branch from rebased PR #300
+
+## Verdict
+
+The iteration addressed all hard-block issues. The PR is now in a reviewable state for a draft PR. The remaining ⚠️ items (automated tests, hook output pattern, partial-failure handling) are reasonable follow-up items for a v1 feature. The architecture is solid and extensible for the planned channels integration.

.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/best-practices/REPORT.md

@@ -0,0 +1,233 @@
+# Best Practices Review — PR #306
+
+Score: 62/100
+
+## Summary
+
+This PR introduces a well-structured async hook system for tracking PR state changes. The overall architecture (library separation, guard patterns, config tiers) reflects solid engineering thinking. However, several concrete shell scripting problems drag the score down: the `_pr_state_diff` function spawns ~20 subshells piping the same large JSON blob through `jq` one field at a time, which is both slow and fragile; cache writes are non-atomic, risking corrupt state on crash; the throttle file has a TOCTOU race; and a subtle operator-precedence bug in `pr-discover.sh` causes the main/master branch guard to always pass on non-`main` branches. Rate limiting is handled only by a coarse time gate with no backoff, and API failures silently produce empty strings rather than propagating errors.
+
+---
+
+## Findings
+
+### F1 — CRITICAL: Operator-precedence logic bug in branch skip guard
+
+**File:** `plugins/github/hooks/scripts/lib/pr-discover.sh`, line ~65
+
+```bash
+[ "$branch" = "main" ] || [ "$branch" = "master" ] && return 0
+```
+
+In bash, `&&` binds more tightly than `||` in the context of `[ ]` list operators. This line parses as:
+
+```
+[ "$branch" = "main" ]  OR  ([ "$branch" = "master" ] AND return 0)
+```
+
+So a branch named `main` does NOT trigger `return 0` — it falls through to the PR lookup. The intended check requires parenthesization:
+
+```bash
+{ [ "$branch" = "main" ] || [ "$branch" = "master" ]; } && return 0
+```
+
+or an `if` statement. As written, repos on `main` will generate spurious API calls every check cycle.
+
+---
+
+### F2 — HIGH: Non-atomic cache writes risk corrupt state
+
+**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, line ~56
+
+```bash
+echo "$new_state" > "$cache_file"
+```
+
+`echo … >` is a truncate-then-write, not atomic. If the process is killed mid-write (or if two hook invocations overlap — plausible given PostToolUse fires concurrently with tool activity), the cache file is left in a partial/empty state. On the next read, `old_state` would be empty, suppressing all change detection silently.
+
+The standard fix is a write-then-rename pattern:
+
+```bash
+local tmp_file
+tmp_file="$(mktemp "${cache_file}.XXXXXX")"
+echo "$new_state" > "$tmp_file" && mv "$tmp_file" "$cache_file"
+```
+
+`mv` on the same filesystem is atomic. The same issue applies to the throttle timestamp file at `pr-state-check.sh` line ~83:
+
+```bash
+echo "$now" > "$last_check_file"
+```
+
+---
+
+### F3 — HIGH: `_pr_state_diff` spawns ~20 subshells over the same JSON
+
+**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines ~130–260
+
+Each field comparison does:
+
+```bash
+old_body="$(echo "$old" | jq -r '.pr.body // ""')"
+new_body="$(echo "$new" | jq -r '.pr.body // ""')"
+```
+
+With ~10 compared fields, each requiring two `echo | jq` subshells, `_pr_state_diff` forks at minimum 20 processes per PR per invocation. On a slow system or when many PRs are tracked, this is a measurable performance issue. It also means the full JSON string (potentially large with long PR bodies) is passed through process substitution repeatedly.
+
+The idiomatic approach is a single `jq` call that extracts all fields at once into shell variables via `read`:
+
+```bash
+read -r old_body old_title old_draft old_state … <<< \
+  "$(echo "$old" | jq -r '[.pr.body // "", .pr.title // "", ...] | @tsv')"
+```
+
+Or, better: parse both old and new in a single `jq -n` invocation that emits all comparison results as a structured object, then parse that object once in shell. Either approach reduces 20+ forks to 1–2.
+
+---
+
+### F4 — HIGH: `_pr_state_diff_checks` is O(N²) in check count
+
+**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines ~265–295
+
+For each check name in `all_checks`, the loop spawns 4 `echo "$old/$new" | jq` subshells:
+
+```bash
+old_status="$(echo "$old" | jq -r --arg name "$check_name" \
+  '.checks.checks[] | select(.name == $name) | .status // "missing"' | head -1)"
+```
+
+If there are N checks, this is 4N subshells per diff call. Repos with many CI checks (20–50 is common) will make this very slow. The checks diff should be done in a single `jq` expression comparing both arrays together.
+
+---
+
+### F5 — MEDIUM: TOCTOU race in throttle timestamp
+
+**File:** `plugins/github/hooks/scripts/pr-state-check.sh`, lines ~77–88
+
+```bash
+if [ -f "$last_check_file" ]; then
+  last_check="$(cat "$last_check_file")"
+  elapsed=$((now - last_check))
+  if [ "$elapsed" -lt "$check_interval" ]; then
+    exit 0
+  fi
+fi
+echo "$now" > "$last_check_file"
+```
+
+There is a check-then-act gap between reading the file and writing the new timestamp. Two concurrent PostToolUse invocations can both read the same stale timestamp, both decide to proceed, and both fire simultaneous API batches. With `set -euo pipefail` active, this won't corrupt state, but it can cause double API calls. Using `ln` or `flock` would prevent this, but for a 60s throttle window this is low-severity in practice.
+
+---
+
+### F6 — MEDIUM: Unquoted variable expansion in `gh api` flag
+
+**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines ~85, ~91, ~96, etc.
+**File:** `plugins/github/hooks/scripts/lib/pr-discover.sh`, line ~73
+
+```bash
+pr_json="$(gh api ${gh_hostname_flag} \
+  "repos/${owner}/${repo}/pulls/${pr_number}" …)"
+```
+
+`${gh_hostname_flag}` is unquoted. When empty, this expands correctly, but when set to `--hostname github.com` it relies on word splitting to pass two arguments. This is an accidental correct use of unquoted expansion. The idiomatic approach is an array:
+
+```bash
+local -a gh_flags=()
+if [ "${CLAUDE_CODE_REMOTE:-}" = "true" ]; then
+  gh_flags=(--hostname github.com)
+fi
+gh api "${gh_flags[@]}" "repos/…"
+```
+
+This is more robust and passes `shellcheck`.
+
+---
+
+### F7 — MEDIUM: API rate limits not handled; no exponential backoff
+
+**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines ~80–115
+
+Each `_pr_state_fetch` call makes 4–5 sequential API requests (PR, reviews, comments, review comments, check-runs). With multiple PRs across multiple projects, a session could easily generate 20+ requests per check cycle. There is no handling of HTTP 429 / rate-limit responses — `gh api` will print an error to stderr (suppressed by `2>/dev/null`) and fall back to empty strings or `[]`, meaning the cache is overwritten with incomplete data and legitimate changes are silently dropped.
+
+At minimum, the fetch should log a warning rather than silently discarding rate-limit errors. A better approach: check `gh api --include` for a `Retry-After` header or `X-RateLimit-Remaining` and back off.
+
+---
+
+### F8 — MEDIUM: `cat > /dev/null` idiom for stdin drain is misleading
+
+**File:** `plugins/github/hooks/scripts/pr-state-check.sh`, lines ~36–44
+
+```bash
+if [ "$pr_state_enabled" = "false" ]; then
+  cat > /dev/null
+  exit 0
+fi
+```
+
+The early-exit guards drain stdin before exiting using `cat > /dev/null`. This is functionally correct but semantically odd — it looks like a typo. The intent (consuming stdin to avoid SIGPIPE) is not documented at these guard sites. The stdin drain at line ~72 is documented but the guard drains are not. Either add a comment or refactor the drain into a function called `_drain_stdin` for clarity.
+
+---
+
+### F9 — LOW: Global mutable variables used as return values in sourced library
+
+**File:** `plugins/github/hooks/scripts/lib/pr-discover.sh`, lines ~78–82
+
+```bash
+_PR_OWNER=""
+_PR_REPO=""
+```
+
+`_pr_extract_owner_repo` communicates results via global variables. This works but is fragile in sourced-library contexts: any caller that sources this file gets these globals in its namespace. Because `pr-state.sh` also sources `pr-discover.sh` transitively, these globals are shared across both libraries. The pattern is common in bash but makes the API implicit. A `nameref` (bash 4.3+) or stdout-with-parsing would be more explicit.
+
+---
+
+### F10 — LOW: `jq` labels filter has incorrect array comprehension
+
+**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, line ~192
+
+```bash
+old_labels="$(echo "$old" | jq -c '[.pr.labels // [] | sort[]]')"
+```
+
+`sort[]` is not valid jq for sorting an array and iterating. The correct idiom is `(.pr.labels // []) | sort` then wrapping. The actual expression `[.pr.labels // [] | sort[]]` works in practice because jq's `[]` after `sort` iterates and `[…]` re-collects, but the expression is confusing and may not behave as expected if `labels` is already an array of strings (it is). The more readable and unambiguous form is:
+
+```bash
+jq -c '[(.pr.labels // []) | sort[]]'
+# or
+jq -c '(.pr.labels // []) | sort'
+```
+
+---
+
+### F11 — LOW: No pagination on API calls for comments/reviews
+
+**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines ~91–101
+
+`gh api` without `--paginate` returns at most the first page (typically 30 items). For PRs with many comments or reviews, only the first page is fetched. This means new items added before a page boundary are never surfaced. Since this is a change-detection system, missed events are a correctness problem. The fix is either `--paginate` (returns all pages but costs more API calls) or accept the limitation and document it.
+
+---
+
+### F12 — LOW: `PLUGIN_NAME` variable is declared but never used
+
+**File:** `plugins/github/hooks/scripts/pr-state-check.sh`, line ~18
+
+```bash
+PLUGIN_NAME="github"
+```
+
+This variable is set at the top of `pr-state-check.sh` but never referenced. Dead code.
+
+---
+
+## References
+
+- Files reviewed:
+  - `plugins/github/hooks/scripts/pr-state-check.sh`
+  - `plugins/github/hooks/scripts/lib/pr-state.sh`
+  - `plugins/github/hooks/scripts/lib/pr-discover.sh`
+  - `plugins/github/hooks/hooks.json`
+- https://github.com/nsheaps/ai-mktpl/pull/306
+- Shell scripting references:
+  - [Bash Pitfalls — Greg's Wiki](https://mywiki.wooledge.org/BashPitfalls)
+  - [ShellCheck](https://www.shellcheck.net/) — F6 (unquoted flag var) and F1 (operator precedence) are detectable by shellcheck
+  - [Bash FAQ: Atomic file writes](https://mywiki.wooledge.org/AtomicWriting)
+  - [GitHub REST API — Rate Limits](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api)