From 5258e67b7c74bafb678d220d65589292a5132a21 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 24 Mar 2026 19:49:58 +0000 Subject: [PATCH 01/11] feat(github): add async hooks for PR state tracking Add PostToolUse, SessionStart, and Stop hooks that silently monitor PR state across all projects in multi-repo sessions. Fetches comments, reviews, CI status, merge readiness, and body content via gh CLI and caches snapshots locally. When state changes are detected on resume or between tool calls, reports the diff to the agent. - PR discovery across sibling git repos for multi-project sessions - Configurable cache dir (default: ~/.claude/plugin-cache/github) - Project-specific cache subdirectories - Detailed change detection: reviews, comments, CI, labels, merge status - Skill documentation for pr-state-tracking - Future: channels integration for autonomous session wake on PR changes https://claude.ai/code/session_01HJDTfa1KwAnxW1oFVgHqvc --- plugins/github/.claude-plugin/plugin.json | 9 +- plugins/github/README.md | 89 ++++- plugins/github/github.settings.yaml | 20 ++ plugins/github/hooks/hooks.json | 31 +- .../github/hooks/scripts/lib/pr-discover.sh | 114 ++++++ plugins/github/hooks/scripts/lib/pr-state.sh | 330 ++++++++++++++++++ .../github/hooks/scripts/pr-state-check.sh | 133 +++++++ plugins/github/lib/hook-output.sh | 1 + .../github/skills/pr-state-tracking/SKILL.md | 85 +++++ 9 files changed, 807 insertions(+), 5 deletions(-) create mode 100644 plugins/github/hooks/scripts/lib/pr-discover.sh create mode 100644 plugins/github/hooks/scripts/lib/pr-state.sh create mode 100644 plugins/github/hooks/scripts/pr-state-check.sh create mode 120000 plugins/github/lib/hook-output.sh create mode 100644 plugins/github/skills/pr-state-tracking/SKILL.md diff --git a/plugins/github/.claude-plugin/plugin.json b/plugins/github/.claude-plugin/plugin.json index 625255cfe..4061ba7f3 100644 --- a/plugins/github/.claude-plugin/plugin.json +++ b/plugins/github/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "github", "version": "0.1.13", - "description": "GitHub CLI installation, authentication, and workflow skill for Claude Code sessions. Consolidates gh-tool and github-auth-skill into a single plugin.", + "description": "GitHub CLI installation, authentication, PR state tracking, and workflow skill for Claude Code sessions. Monitors comments, reviews, CI status, and merge readiness across multi-project sessions.", "author": { "name": "Nathan Heaps", "email": "nsheaps@gmail.com", @@ -21,6 +21,11 @@ "issues", "session-start", "web-session", - "auto-install" + "auto-install", + "pr-state-tracking", + "ci-status", + "reviews", + "async-hooks", + "multi-project" ] } diff --git a/plugins/github/README.md b/plugins/github/README.md index 7e59b4b27..67aa6c4b7 100644 --- a/plugins/github/README.md +++ b/plugins/github/README.md @@ -1,6 +1,6 @@ # github -GitHub CLI installation, authentication, and workflow skill for Claude Code sessions. +GitHub CLI installation, authentication, and PR state tracking for Claude Code sessions. Consolidates the former `gh-tool` and `github-auth-skill` plugins into a single plugin. @@ -10,11 +10,15 @@ Consolidates the former `gh-tool` and `github-auth-skill` plugins into a single - **Auto-update**: Checks for and installs updates when version is "latest" - **Auth verification**: Optionally runs `gh auth status` after install - **Background install**: Optional non-blocking installation +- **PR state tracking**: Monitors comments, reviews, CI status, merge status across all session projects +- **Multi-project support**: Discovers PRs across sibling project directories in multi-repo sessions - **GitHub CLI skill**: Full gh CLI reference (PRs, issues, releases, actions, API) - **Authentication skill**: Covers device code flow, PATs, fine-grained tokens, and GitHub App auth ## How It Works +### GitHub CLI Installation + On session start (web sessions only): 1. Checks if gh is already available on PATH @@ -25,6 +29,68 @@ On session start (web sessions only): The `bin/.local/` directory is gitignored, so installed binaries don't pollute the repo. +### PR State Tracking + +The plugin includes async hooks that monitor PR state changes across all projects in the session: + +**SessionStart**: Discovers all active PRs for the session's projects and establishes a baseline snapshot of their state (comments, reviews, CI checks, body, merge status). + +**PostToolUse**: After each tool use, silently re-fetches PR state and compares against the cached snapshot. When changes are detected (e.g., a new review was submitted, CI completed, a comment was added), the agent is notified with details about what changed. + +**Stop**: Performs a final state check before session end. + +#### Multi-Project Discovery + +In multi-repo sessions (e.g., sessions launched with ai-mktpl, github-actions, and claude-utils), the plugin scans the parent directory of `CLAUDE_PROJECT_DIR` for sibling git repositories. Each repo's current branch is checked for an open PR, and all discovered PRs are tracked. + +#### State Cache + +PR state snapshots are stored as JSON files in the cache directory: + +``` +~/.claude/plugin-cache/github//pr-state/__.json +``` + +Each snapshot includes: +- PR metadata (title, body, state, draft status, labels) +- Reviews (user, state, body) +- Comments (issue comments and inline review comments) +- CI check runs (name, status, conclusion) +- Merge status (mergeable, mergeable_state) + +#### Change Detection + +The following changes are detected and reported: + +| Change | Example | +|--------|---------| +| New review | "nsheaps APPROVED" | +| New comment | "bot: CI passed" | +| New review comment | "nsheaps on src/main.ts: Consider using..." | +| CI status change | "lint: in_progress/pending -> completed/success" | +| PR body updated | Body content changed | +| PR title changed | Old title -> new title | +| Draft status | Converted to draft / marked ready | +| State change | open -> closed, merged | +| Merge status | mergeable changed, conflicts detected | +| Label changes | Labels added/removed | + +### Future: Claude Code Channels + +> **Planned feature**: When Claude Code adds support for channels (the ability to +> programmatically kick off or resume an idle session), the PR state tracking hooks +> could be extended to automatically wake a sleeping session when significant PR +> changes are detected. For example: +> +> - A review with "request changes" could trigger the session to resume and address feedback +> - CI failure could trigger auto-investigation and fix attempts +> - A merge conflict could trigger automatic rebase +> +> This would transform the current "check and report" pattern into a fully +> autonomous "detect and respond" workflow. The cache infrastructure built here +> provides the foundation — state diffs would become channel triggers instead of +> advisory messages. + ## Skills ### gh (GitHub CLI Reference) @@ -46,12 +112,31 @@ github: backgroundInstall: false version: "latest" autoAuthCheck: true + + # PR state tracking + prStateTracking: true + # prStateCacheDir: "~/.claude/plugin-cache/github" +``` + +### Cache Directory + +The default cache location is `~/.claude/plugin-cache/github`. The plugin appends `//pr-state/` to create project-specific cache directories. + +For project-specific overrides: + +```yaml +# In $CLAUDE_PROJECT_DIR/.claude/plugins.settings.yaml +github: + prStateCacheDir: "~/.claude/plugin-cache/github/my-project" ``` +When installed at the user level, the plugin handles multiple projects automatically by using the project directory name as a cache key. Each project's PRs are tracked independently. + ## Local Sessions -On local sessions (`CLAUDE_CODE_REMOTE` is not `true`), the install hook does nothing. It assumes gh is already installed locally via Homebrew, mise, or another method. +On local sessions (`CLAUDE_CODE_REMOTE` is not `true`), the install hook does nothing. It assumes gh is already installed locally via Homebrew, mise, or another method. PR state tracking runs on all sessions (local and web) as long as `gh` and `jq` are available on PATH. ## Related Plugins - **[github-app](../github-app)** — GitHub App token refresh for long-running agent sessions +- **[scm-utils](../scm-utils)** — Source control management utilities (commit, rebase, PR workflows) diff --git a/plugins/github/github.settings.yaml b/plugins/github/github.settings.yaml index b31ceef08..849535e6b 100644 --- a/plugins/github/github.settings.yaml +++ b/plugins/github/github.settings.yaml @@ -26,3 +26,23 @@ github: # Auto-run `gh auth status` after install to verify authentication autoAuthCheck: true + + # --- PR State Tracking --- + # + # Async hooks that monitor PR state (comments, reviews, CI status, + # merge status) across all projects in the session. State is cached + # locally and compared on each hook invocation. When changes are + # detected (e.g., new review, CI status change), the agent is notified. + + # Enable/disable PR state tracking hooks. + prStateTracking: true + + # Cache directory for PR state snapshots. + # Default: ~/.claude/plugin-cache/github + # The plugin appends //pr-state/ to this path. + # For multi-project sessions, each project gets its own subdirectory. + # + # Suggested structure for project-specific overrides: + # ~/.claude/plugin-cache/github//pr-state/ + # + # prStateCacheDir: "~/.claude/plugin-cache/github" diff --git a/plugins/github/hooks/hooks.json b/plugins/github/hooks/hooks.json index 5e8e5e80c..caeb0e325 100644 --- a/plugins/github/hooks/hooks.json +++ b/plugins/github/hooks/hooks.json @@ -1,5 +1,5 @@ { - "description": "Install/update GitHub CLI on session start for web sessions", + "description": "GitHub CLI installation and PR state tracking hooks", "hooks": { "SessionStart": [ { @@ -9,6 +9,35 @@ "type": "command", "command": "bash ${CLAUDE_PLUGIN_ROOT}/hooks/scripts/install-gh.sh", "timeout": 60 + }, + { + "type": "command", + "command": "HOOK_EVENT=SessionStart bash ${CLAUDE_PLUGIN_ROOT}/hooks/scripts/pr-state-check.sh", + "timeout": 30 + } + ] + } + ], + "PostToolUse": [ + { + "matcher": "*", + "hooks": [ + { + "type": "command", + "command": "HOOK_EVENT=PostToolUse bash ${CLAUDE_PLUGIN_ROOT}/hooks/scripts/pr-state-check.sh", + "timeout": 15 + } + ] + } + ], + "Stop": [ + { + "matcher": "*", + "hooks": [ + { + "type": "command", + "command": "HOOK_EVENT=Stop bash ${CLAUDE_PLUGIN_ROOT}/hooks/scripts/pr-state-check.sh", + "timeout": 15 } ] } diff --git a/plugins/github/hooks/scripts/lib/pr-discover.sh b/plugins/github/hooks/scripts/lib/pr-discover.sh new file mode 100644 index 000000000..7d79df673 --- /dev/null +++ b/plugins/github/hooks/scripts/lib/pr-discover.sh @@ -0,0 +1,114 @@ +#!/usr/bin/env bash +# pr-discover.sh — Discover active PRs for the current session's projects +# +# Supports multi-project sessions by scanning CLAUDE_PROJECT_DIR and +# any additional project directories for git repos with active PRs. +# +# Usage: +# source "${SCRIPT_DIR}/lib/pr-discover.sh" +# pr_discover_all # prints "owner repo pr_number" lines to stdout + +# Guard against double-sourcing +if [ "${_PR_DISCOVER_LOADED:-}" = "true" ]; then + return 0 2>/dev/null || true +fi +_PR_DISCOVER_LOADED="true" + +# Discover PRs for all projects in the current session. +# Checks CLAUDE_PROJECT_DIR and scans for sibling project dirs +# that are git repos on branches with open PRs. +# Output: "owner repo pr_number" lines on stdout +pr_discover_all() { + local dirs=() + + # Primary project directory + if [ -n "${CLAUDE_PROJECT_DIR:-}" ]; then + dirs+=("$CLAUDE_PROJECT_DIR") + fi + + # Multi-project: check parent directory for sibling repos + # Claude Code web sessions with multiple repos clone them as siblings + if [ -n "${CLAUDE_PROJECT_DIR:-}" ]; then + local parent_dir + parent_dir="$(dirname "$CLAUDE_PROJECT_DIR")" + for sibling in "$parent_dir"/*/; do + [ -d "$sibling" ] || continue + local abs_sibling + abs_sibling="$(cd "$sibling" && pwd)" + # Skip the primary project (already added) + [ "$abs_sibling" = "$CLAUDE_PROJECT_DIR" ] && continue + # Only include git repos + [ -d "$abs_sibling/.git" ] && dirs+=("$abs_sibling") + done + fi + + # For each directory, find the current branch's PR + for dir in "${#dirs[@]+${dirs[@]}}"; do + [ -d "$dir/.git" ] || continue + _pr_discover_for_dir "$dir" + done +} + +# Discover PRs for a single directory. +# Args: $1=directory +# Output: "owner repo pr_number" on stdout (one per PR found) +_pr_discover_for_dir() { + local dir="$1" + local branch remote_url owner repo pr_number + + # Get current branch + branch="$(git -C "$dir" rev-parse --abbrev-ref HEAD 2>/dev/null)" || return 0 + [ "$branch" = "HEAD" ] && return 0 + [ "$branch" = "main" ] || [ "$branch" = "master" ] && return 0 + + # Get remote URL and extract owner/repo + remote_url="$(git -C "$dir" config --get remote.origin.url 2>/dev/null)" || return 0 + _pr_extract_owner_repo "$remote_url" || return 0 + + # Find open PR for this branch + local gh_hostname_flag="" + if [ "${CLAUDE_CODE_REMOTE:-}" = "true" ]; then + gh_hostname_flag="--hostname github.com" + fi + + pr_number="$(gh api ${gh_hostname_flag} \ + "repos/${_PR_OWNER}/${_PR_REPO}/pulls?head=${_PR_OWNER}:${branch}&state=open" \ + --jq '.[0].number // empty' 2>/dev/null)" || return 0 + + if [ -n "$pr_number" ]; then + echo "${_PR_OWNER} ${_PR_REPO} ${pr_number}" + fi +} + +# Internal variables for owner/repo extraction +_PR_OWNER="" +_PR_REPO="" + +# Extract owner and repo from a git remote URL. +# Supports HTTPS and SSH formats, including local proxy URLs. +# Args: $1=remote_url +# Sets: _PR_OWNER, _PR_REPO +# Returns: 0 on success, 1 on failure +_pr_extract_owner_repo() { + local url="$1" + _PR_OWNER="" + _PR_REPO="" + + # Handle various URL formats: + # https://github.com/owner/repo.git + # git@github.com:owner/repo.git + # http://127.0.0.1:PORT/git/owner/repo (web session proxy) + if echo "$url" | grep -qE '/git/[^/]+/[^/]+$'; then + # Web session proxy format + _PR_OWNER="$(echo "$url" | sed 's|.*/git/\([^/]*\)/\([^/]*\)$|\1|')" + _PR_REPO="$(echo "$url" | sed 's|.*/git/\([^/]*\)/\([^/]*\)$|\2|' | sed 's/\.git$//')" + elif echo "$url" | grep -qE 'github\.com[:/]'; then + # Standard GitHub URL + _PR_OWNER="$(echo "$url" | sed 's|.*github\.com[:/]\([^/]*\)/.*|\1|')" + _PR_REPO="$(echo "$url" | sed 's|.*github\.com[:/][^/]*/\([^/]*\)$|\1|' | sed 's/\.git$//')" + else + return 1 + fi + + [ -n "$_PR_OWNER" ] && [ -n "$_PR_REPO" ] +} diff --git a/plugins/github/hooks/scripts/lib/pr-state.sh b/plugins/github/hooks/scripts/lib/pr-state.sh new file mode 100644 index 000000000..5ff63e19f --- /dev/null +++ b/plugins/github/hooks/scripts/lib/pr-state.sh @@ -0,0 +1,330 @@ +#!/usr/bin/env bash +# pr-state.sh — Library for fetching and caching PR state from GitHub +# +# Fetches PR metadata (comments, reviews, CI status, body, merge status) +# via `gh` CLI and stores snapshots in a local cache directory. On subsequent +# calls, compares the new state against the cached state and reports changes. +# +# Usage: +# source "${SCRIPT_DIR}/lib/pr-state.sh" +# pr_state_init "/path/to/cache/dir" +# pr_state_fetch_and_compare "owner" "repo" "pr_number" +# # Returns 0 if no changes, 1 if changes detected +# # Changes are accumulated in PR_STATE_CHANGES array +# +# Requires: gh CLI on PATH, jq on PATH + +# Guard against double-sourcing +if [ "${_PR_STATE_LOADED:-}" = "true" ]; then + return 0 2>/dev/null || true +fi +_PR_STATE_LOADED="true" + +# --- State --- +_PR_STATE_CACHE_DIR="" +declare -a PR_STATE_CHANGES=() + +# Initialize the cache directory. +# Args: $1=cache_dir +pr_state_init() { + _PR_STATE_CACHE_DIR="$1" + mkdir -p "$_PR_STATE_CACHE_DIR" + PR_STATE_CHANGES=() +} + +# Fetch current PR state from GitHub and compare against cache. +# Updates the cache file with the new state. +# Args: $1=owner $2=repo $3=pr_number +# Returns: 0 if no changes, 1 if changes detected +# Side effects: populates PR_STATE_CHANGES array +pr_state_fetch_and_compare() { + local owner="$1" repo="$2" pr_number="$3" + local cache_file="${_PR_STATE_CACHE_DIR}/${owner}_${repo}_${pr_number}.json" + local old_state="" new_state="" + + # Load old state if it exists + if [ -f "$cache_file" ]; then + old_state="$(cat "$cache_file")" + fi + + # Fetch new state + new_state="$(_pr_state_fetch "$owner" "$repo" "$pr_number")" || return 0 + + # Save new state + echo "$new_state" > "$cache_file" + + # If no old state, this is the first fetch — no changes to report + if [ -z "$old_state" ]; then + return 0 + fi + + # Compare states + _pr_state_diff "$old_state" "$new_state" "$owner" "$repo" "$pr_number" +} + +# Fetch all PR state into a single JSON object. +# Args: $1=owner $2=repo $3=pr_number +# Returns: JSON string via stdout +_pr_state_fetch() { + local owner="$1" repo="$2" pr_number="$3" + local gh_hostname_flag="" + + # In web sessions, gh remote is a proxy — use --hostname + if [ "${CLAUDE_CODE_REMOTE:-}" = "true" ]; then + gh_hostname_flag="--hostname github.com" + fi + + # Fetch PR details, reviews, comments, and check runs in parallel + local pr_json review_json comment_json checks_json + + # PR core data (body, state, mergeable, title, labels, draft, reviewDecision) + pr_json="$(gh api ${gh_hostname_flag} \ + "repos/${owner}/${repo}/pulls/${pr_number}" \ + --jq '{ + title: .title, + body: .body, + state: .state, + draft: .draft, + mergeable: .mergeable, + mergeable_state: .mergeable_state, + merged: .merged, + merge_commit_sha: .merge_commit_sha, + review_decision: .auto_merge, + labels: [.labels[].name], + head_sha: .head.sha, + updated_at: .updated_at + }' 2>/dev/null)" || { echo "{}" ; return 1; } + + # Reviews + review_json="$(gh api ${gh_hostname_flag} \ + "repos/${owner}/${repo}/pulls/${pr_number}/reviews" \ + --jq '[.[] | {user: .user.login, state: .state, submitted_at: .submitted_at, body: .body}]' \ + 2>/dev/null)" || review_json="[]" + + # PR comments (issue comments) + comment_json="$(gh api ${gh_hostname_flag} \ + "repos/${owner}/${repo}/issues/${pr_number}/comments" \ + --jq '[.[] | {user: .user.login, body: .body, created_at: .created_at, id: .id}]' \ + 2>/dev/null)" || comment_json="[]" + + # Review comments (inline code comments) + local review_comment_json + review_comment_json="$(gh api ${gh_hostname_flag} \ + "repos/${owner}/${repo}/pulls/${pr_number}/comments" \ + --jq '[.[] | {user: .user.login, body: .body, path: .path, created_at: .created_at, id: .id}]' \ + 2>/dev/null)" || review_comment_json="[]" + + # Check runs for head SHA + local head_sha + head_sha="$(echo "$pr_json" | jq -r '.head_sha // empty')" + if [ -n "$head_sha" ]; then + checks_json="$(gh api ${gh_hostname_flag} \ + "repos/${owner}/${repo}/commits/${head_sha}/check-runs" \ + --jq '{ + total_count: .total_count, + checks: [.check_runs[] | {name: .name, status: .status, conclusion: .conclusion, completed_at: .completed_at}] + }' 2>/dev/null)" || checks_json='{"total_count":0,"checks":[]}' + else + checks_json='{"total_count":0,"checks":[]}' + fi + + # Combine into single JSON + jq -n \ + --argjson pr "$pr_json" \ + --argjson reviews "$review_json" \ + --argjson comments "$comment_json" \ + --argjson review_comments "$review_comment_json" \ + --argjson checks "$checks_json" \ + --arg fetched_at "$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ + '{ + pr: $pr, + reviews: $reviews, + comments: $comments, + review_comments: $review_comments, + checks: $checks, + fetched_at: $fetched_at + }' +} + +# Compare old and new state, populating PR_STATE_CHANGES array. +# Args: $1=old_state_json $2=new_state_json $3=owner $4=repo $5=pr_number +# Returns: 0 if no changes, 1 if changes detected +_pr_state_diff() { + local old="$1" new="$2" owner="$3" repo="$4" pr_number="$5" + PR_STATE_CHANGES=() + + # Compare PR body + local old_body new_body + old_body="$(echo "$old" | jq -r '.pr.body // ""')" + new_body="$(echo "$new" | jq -r '.pr.body // ""')" + if [ "$old_body" != "$new_body" ]; then + PR_STATE_CHANGES+=("PR body updated on ${owner}/${repo}#${pr_number}") + fi + + # Compare PR title + local old_title new_title + old_title="$(echo "$old" | jq -r '.pr.title // ""')" + new_title="$(echo "$new" | jq -r '.pr.title // ""')" + if [ "$old_title" != "$new_title" ]; then + PR_STATE_CHANGES+=("PR title changed: '${old_title}' -> '${new_title}' on ${owner}/${repo}#${pr_number}") + fi + + # Compare draft status + local old_draft new_draft + old_draft="$(echo "$old" | jq -r '.pr.draft // false')" + new_draft="$(echo "$new" | jq -r '.pr.draft // false')" + if [ "$old_draft" != "$new_draft" ]; then + if [ "$new_draft" = "true" ]; then + PR_STATE_CHANGES+=("PR ${owner}/${repo}#${pr_number} converted to draft") + else + PR_STATE_CHANGES+=("PR ${owner}/${repo}#${pr_number} marked ready for review") + fi + fi + + # Compare PR state (open/closed/merged) + local old_state new_state old_merged new_merged + old_state="$(echo "$old" | jq -r '.pr.state // ""')" + new_state="$(echo "$new" | jq -r '.pr.state // ""')" + old_merged="$(echo "$old" | jq -r '.pr.merged // false')" + new_merged="$(echo "$new" | jq -r '.pr.merged // false')" + if [ "$old_state" != "$new_state" ]; then + if [ "$new_merged" = "true" ] && [ "$old_merged" != "true" ]; then + PR_STATE_CHANGES+=("PR ${owner}/${repo}#${pr_number} was MERGED") + else + PR_STATE_CHANGES+=("PR state changed: ${old_state} -> ${new_state} on ${owner}/${repo}#${pr_number}") + fi + fi + + # Compare mergeable status + local old_mergeable new_mergeable old_mergeable_state new_mergeable_state + old_mergeable="$(echo "$old" | jq -r '.pr.mergeable // ""')" + new_mergeable="$(echo "$new" | jq -r '.pr.mergeable // ""')" + old_mergeable_state="$(echo "$old" | jq -r '.pr.mergeable_state // ""')" + new_mergeable_state="$(echo "$new" | jq -r '.pr.mergeable_state // ""')" + if [ "$old_mergeable" != "$new_mergeable" ] || [ "$old_mergeable_state" != "$new_mergeable_state" ]; then + PR_STATE_CHANGES+=("Merge status changed on ${owner}/${repo}#${pr_number}: mergeable=${old_mergeable}->${new_mergeable}, state=${old_mergeable_state}->${new_mergeable_state}") + fi + + # Compare labels + local old_labels new_labels + old_labels="$(echo "$old" | jq -c '[.pr.labels // [] | sort[]]')" + new_labels="$(echo "$new" | jq -c '[.pr.labels // [] | sort[]]')" + if [ "$old_labels" != "$new_labels" ]; then + PR_STATE_CHANGES+=("Labels changed on ${owner}/${repo}#${pr_number}: ${old_labels} -> ${new_labels}") + fi + + # Compare reviews (count and latest states) + local old_review_count new_review_count + old_review_count="$(echo "$old" | jq '.reviews | length')" + new_review_count="$(echo "$new" | jq '.reviews | length')" + if [ "$old_review_count" != "$new_review_count" ]; then + local new_reviews + new_reviews="$(echo "$new" | jq -r ".reviews[$old_review_count:][] | \"\(.user) \(.state)\"" 2>/dev/null || true)" + if [ -n "$new_reviews" ]; then + while IFS= read -r review_line; do + PR_STATE_CHANGES+=("New review on ${owner}/${repo}#${pr_number}: ${review_line}") + done <<< "$new_reviews" + else + PR_STATE_CHANGES+=("Reviews changed on ${owner}/${repo}#${pr_number}: ${old_review_count} -> ${new_review_count} reviews") + fi + fi + + # Compare comments (count) + local old_comment_count new_comment_count + old_comment_count="$(echo "$old" | jq '.comments | length')" + new_comment_count="$(echo "$new" | jq '.comments | length')" + if [ "$old_comment_count" != "$new_comment_count" ]; then + local new_comments + new_comments="$(echo "$new" | jq -r ".comments[$old_comment_count:][] | \"\(.user): \(.body[0:100])\"" 2>/dev/null || true)" + if [ -n "$new_comments" ]; then + while IFS= read -r comment_line; do + PR_STATE_CHANGES+=("New comment on ${owner}/${repo}#${pr_number}: ${comment_line}") + done <<< "$new_comments" + else + PR_STATE_CHANGES+=("Comments changed on ${owner}/${repo}#${pr_number}: ${old_comment_count} -> ${new_comment_count} comments") + fi + fi + + # Compare review comments (inline code comments) + local old_rc_count new_rc_count + old_rc_count="$(echo "$old" | jq '.review_comments | length')" + new_rc_count="$(echo "$new" | jq '.review_comments | length')" + if [ "$old_rc_count" != "$new_rc_count" ]; then + local new_rcs + new_rcs="$(echo "$new" | jq -r ".review_comments[$old_rc_count:][] | \"\(.user) on \(.path): \(.body[0:100])\"" 2>/dev/null || true)" + if [ -n "$new_rcs" ]; then + while IFS= read -r rc_line; do + PR_STATE_CHANGES+=("New review comment on ${owner}/${repo}#${pr_number}: ${rc_line}") + done <<< "$new_rcs" + else + PR_STATE_CHANGES+=("Review comments changed on ${owner}/${repo}#${pr_number}: ${old_rc_count} -> ${new_rc_count}") + fi + fi + + # Compare CI check results + local old_checks new_checks + old_checks="$(echo "$old" | jq -c '[.checks.checks // [] | sort_by(.name)[] | {name, status, conclusion}]')" + new_checks="$(echo "$new" | jq -c '[.checks.checks // [] | sort_by(.name)[] | {name, status, conclusion}]')" + if [ "$old_checks" != "$new_checks" ]; then + # Find specific check changes + local check_diff + check_diff="$(_pr_state_diff_checks "$old" "$new")" + if [ -n "$check_diff" ]; then + while IFS= read -r check_line; do + PR_STATE_CHANGES+=("CI on ${owner}/${repo}#${pr_number}: ${check_line}") + done <<< "$check_diff" + else + PR_STATE_CHANGES+=("CI status changed on ${owner}/${repo}#${pr_number}") + fi + fi + + # Return whether changes were detected + [ ${#PR_STATE_CHANGES[@]} -gt 0 ] && return 1 || return 0 +} + +# Diff individual check runs between old and new state. +# Args: $1=old_state_json $2=new_state_json +# Returns: human-readable diff lines via stdout +_pr_state_diff_checks() { + local old="$1" new="$2" + + # Get all check names from both states + local all_checks + all_checks="$(jq -r -n \ + --argjson old "$(echo "$old" | jq '.checks.checks // []')" \ + --argjson new "$(echo "$new" | jq '.checks.checks // []')" \ + '[$old[].name, $new[].name] | unique[]')" + + while IFS= read -r check_name; do + [ -z "$check_name" ] && continue + local old_status old_conclusion new_status new_conclusion + old_status="$(echo "$old" | jq -r --arg name "$check_name" '.checks.checks[] | select(.name == $name) | .status // "missing"' | head -1)" + old_conclusion="$(echo "$old" | jq -r --arg name "$check_name" '.checks.checks[] | select(.name == $name) | .conclusion // "pending"' | head -1)" + new_status="$(echo "$new" | jq -r --arg name "$check_name" '.checks.checks[] | select(.name == $name) | .status // "missing"' | head -1)" + new_conclusion="$(echo "$new" | jq -r --arg name "$check_name" '.checks.checks[] | select(.name == $name) | .conclusion // "pending"' | head -1)" + + [ -z "$old_status" ] && old_status="missing" + [ -z "$new_status" ] && new_status="missing" + [ -z "$old_conclusion" ] && old_conclusion="pending" + [ -z "$new_conclusion" ] && new_conclusion="pending" + + if [ "$old_status/$old_conclusion" != "$new_status/$new_conclusion" ]; then + echo "${check_name}: ${old_status}/${old_conclusion} -> ${new_status}/${new_conclusion}" + fi + done <<< "$all_checks" +} + +# Get a human-readable summary of all changes. +# Returns: formatted string via stdout +pr_state_changes_summary() { + if [ ${#PR_STATE_CHANGES[@]} -eq 0 ]; then + echo "No changes detected" + return + fi + + local summary="PR state changes detected:\n" + for change in "${PR_STATE_CHANGES[@]}"; do + summary+=" - ${change}\n" + done + printf '%b' "$summary" +} diff --git a/plugins/github/hooks/scripts/pr-state-check.sh b/plugins/github/hooks/scripts/pr-state-check.sh new file mode 100644 index 000000000..482eb684e --- /dev/null +++ b/plugins/github/hooks/scripts/pr-state-check.sh @@ -0,0 +1,133 @@ +#!/usr/bin/env bash +# pr-state-check.sh — Async hook for tracking PR state changes +# +# Used by PostToolUse, SessionStart, and Stop hooks. +# Silently fetches PR state (comments, reviews, CI, body, merge status) +# and caches it locally. When state changes are detected (e.g., new review, +# CI failure, new comment), outputs a message to inform the agent. +# +# On SessionStart: establishes baseline state for all discovered PRs +# On PostToolUse: checks for changes since last fetch +# On Stop: final check and summary of any pending changes +# +# Environment: +# HOOK_EVENT — which hook triggered this (SessionStart, PostToolUse, Stop) +# CLAUDE_PROJECT_DIR — project directory +# CLAUDE_PLUGIN_ROOT — plugin root for accessing libs +set -euo pipefail + +PLUGIN_NAME="github" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +source "${CLAUDE_PLUGIN_ROOT}/lib/plugin-config-read.sh" +source "${CLAUDE_PLUGIN_ROOT}/lib/log.sh" +source "${SCRIPT_DIR}/lib/pr-state.sh" +source "${SCRIPT_DIR}/lib/pr-discover.sh" + +# --- Read config --- + +pr_state_enabled="$(plugin_get_config "prStateTracking" "true")" +cache_base="$(plugin_get_config "prStateCacheDir" "")" + +# --- Guards --- + +if [ "$pr_state_enabled" = "false" ]; then + # Consume stdin and exit silently + cat > /dev/null + exit 0 +fi + +if ! command -v gh &>/dev/null; then + cat > /dev/null + exit 0 +fi + +if ! command -v jq &>/dev/null; then + cat > /dev/null + exit 0 +fi + +# --- Resolve cache directory --- + +if [ -z "$cache_base" ]; then + cache_base="${HOME}/.claude/plugin-cache/github" +fi + +# Add project-specific subdirectory based on project dir name +if [ -n "${CLAUDE_PROJECT_DIR:-}" ]; then + project_slug="$(basename "$CLAUDE_PROJECT_DIR")" +else + project_slug="default" +fi +cache_dir="${cache_base}/${project_slug}/pr-state" +pr_state_init "$cache_dir" + +# --- Consume stdin --- +# Hook input is provided via stdin; consume it to avoid broken pipe +hook_input="$(cat)" + +# --- Determine hook event --- + +hook_event="${HOOK_EVENT:-PostToolUse}" + +# --- Discover PRs --- + +log_info "Checking PR state (event: ${hook_event})" + +pr_list="$(pr_discover_all 2>/dev/null)" || pr_list="" + +if [ -z "$pr_list" ]; then + log_info "No active PRs found for tracked projects" + exit 0 +fi + +# --- Fetch and compare state for each PR --- + +all_changes=() +pr_count=0 + +while IFS=' ' read -r owner repo pr_number; do + [ -z "$owner" ] && continue + pr_count=$((pr_count + 1)) + log_info "Checking ${owner}/${repo}#${pr_number}" + + if ! pr_state_fetch_and_compare "$owner" "$repo" "$pr_number"; then + # Changes detected + for change in "${PR_STATE_CHANGES[@]}"; do + all_changes+=("$change") + done + fi +done <<< "$pr_list" + +log_info "Checked ${pr_count} PR(s)" + +# --- Output results --- + +if [ ${#all_changes[@]} -eq 0 ]; then + # No changes — silent success + if [ "$hook_event" = "SessionStart" ]; then + echo "github: PR state baseline established for ${pr_count} PR(s)" + fi + exit 0 +fi + +# Changes detected — build output message +{ + echo "github: PR state changes detected since last check:" + echo "" + for change in "${all_changes[@]}"; do + echo " - ${change}" + done + echo "" + echo "Review these changes and determine if any action is needed." +} > /dev/stdout + +# For PostToolUse and Stop hooks, exit 2 to provide feedback to Claude +# For SessionStart, exit 0 with the message in stdout +if [ "$hook_event" = "SessionStart" ]; then + exit 0 +else + # Exit 2 signals "feedback" for Stop hooks (shown to Claude) + # For PostToolUse, stdout is advisory (additionalContext) + exit 0 +fi diff --git a/plugins/github/lib/hook-output.sh b/plugins/github/lib/hook-output.sh new file mode 120000 index 000000000..4c67a420b --- /dev/null +++ b/plugins/github/lib/hook-output.sh @@ -0,0 +1 @@ +../../../shared/lib/hook-output.sh \ No newline at end of file diff --git a/plugins/github/skills/pr-state-tracking/SKILL.md b/plugins/github/skills/pr-state-tracking/SKILL.md new file mode 100644 index 000000000..4e1f164de --- /dev/null +++ b/plugins/github/skills/pr-state-tracking/SKILL.md @@ -0,0 +1,85 @@ +--- +name: pr-state-tracking +description: > + PR state tracking via async hooks. Monitors comments, reviews, CI status, + merge readiness, and body changes across all projects in a multi-repo session. + Automatically detects and reports changes between checks. +--- + +# PR State Tracking + +The github plugin includes async hooks that silently monitor PR state changes across all projects in your session. + +## What It Tracks + +- **Reviews**: New approvals, change requests, comments from reviewers +- **Comments**: Issue comments and inline review comments on code +- **CI Status**: Check run status and conclusion changes (pass/fail/pending) +- **PR Body**: Content changes to the PR description +- **Merge Status**: Mergeable state, conflicts, merge readiness +- **Labels**: Label additions and removals +- **Draft Status**: Draft <-> ready for review transitions +- **PR State**: Open/closed/merged transitions + +## How It Works + +### Session Start + +On session start, the plugin: +1. Discovers all sibling git repositories (multi-project support) +2. Finds open PRs for each repo's current branch +3. Fetches a full state snapshot for each PR +4. Caches the snapshot as the baseline + +### Post Tool Use + +After each tool use, the plugin: +1. Re-fetches state for all tracked PRs +2. Compares against the cached snapshot +3. If changes are detected, reports them via stdout (shown as additionalContext) +4. Updates the cache with the new state + +### Stop + +On session stop, performs a final state check for any last-minute changes. + +## Configuration + +```yaml +# In plugins.settings.yaml (project or user level) +github: + prStateTracking: true # Enable/disable (default: true) + prStateCacheDir: "" # Custom cache dir (default: ~/.claude/plugin-cache/github) +``` + +### Cache Structure + +``` +~/.claude/plugin-cache/github/ + / + pr-state/ + __.json +``` + +For multi-project sessions, the project slug is derived from the project directory name. + +## Multi-Project Sessions + +When a session spans multiple repositories (e.g., ai-mktpl + github-actions + claude-utils), the plugin discovers all sibling git repos by scanning the parent directory of `CLAUDE_PROJECT_DIR`. Each repo on a non-default branch with an open PR is tracked. + +## Requirements + +- `gh` CLI on PATH (installed by this plugin or externally) +- `jq` on PATH (for JSON processing) +- GitHub authentication configured (`gh auth status` passing) + +## Future: Channels Integration + +When Claude Code supports channels (programmatic session wake/resume), the state tracking infrastructure can be extended to automatically trigger sessions: + +- **Review received**: Wake session to address reviewer feedback +- **CI failed**: Wake session to investigate and fix failures +- **Merge conflict**: Wake session to rebase automatically +- **Label change**: Wake session when "ready-to-merge" label is applied + +The current cache-and-compare pattern provides the foundation. State diffs would become channel triggers, transforming passive monitoring into autonomous response. From f724b6f381d146c640873735bb9064e7965362c7 Mon Sep 17 00:00:00 2001 From: nsheaps <1282393+nsheaps@users.noreply.github.com> Date: Tue, 24 Mar 2026 19:51:24 +0000 Subject: [PATCH 02/11] chore: `mise run lint` --- plugins/github/README.md | 25 ++++++++++--------- .../github/skills/pr-state-tracking/SKILL.md | 6 +++-- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/plugins/github/README.md b/plugins/github/README.md index 67aa6c4b7..99d20e44c 100644 --- a/plugins/github/README.md +++ b/plugins/github/README.md @@ -52,6 +52,7 @@ PR state snapshots are stored as JSON files in the cache directory: ``` Each snapshot includes: + - PR metadata (title, body, state, draft status, labels) - Reviews (user, state, body) - Comments (issue comments and inline review comments) @@ -62,18 +63,18 @@ Each snapshot includes: The following changes are detected and reported: -| Change | Example | -|--------|---------| -| New review | "nsheaps APPROVED" | -| New comment | "bot: CI passed" | -| New review comment | "nsheaps on src/main.ts: Consider using..." | -| CI status change | "lint: in_progress/pending -> completed/success" | -| PR body updated | Body content changed | -| PR title changed | Old title -> new title | -| Draft status | Converted to draft / marked ready | -| State change | open -> closed, merged | -| Merge status | mergeable changed, conflicts detected | -| Label changes | Labels added/removed | +| Change | Example | +| ------------------ | ------------------------------------------------ | +| New review | "nsheaps APPROVED" | +| New comment | "bot: CI passed" | +| New review comment | "nsheaps on src/main.ts: Consider using..." | +| CI status change | "lint: in_progress/pending -> completed/success" | +| PR body updated | Body content changed | +| PR title changed | Old title -> new title | +| Draft status | Converted to draft / marked ready | +| State change | open -> closed, merged | +| Merge status | mergeable changed, conflicts detected | +| Label changes | Labels added/removed | ### Future: Claude Code Channels diff --git a/plugins/github/skills/pr-state-tracking/SKILL.md b/plugins/github/skills/pr-state-tracking/SKILL.md index 4e1f164de..ed02cb876 100644 --- a/plugins/github/skills/pr-state-tracking/SKILL.md +++ b/plugins/github/skills/pr-state-tracking/SKILL.md @@ -26,6 +26,7 @@ The github plugin includes async hooks that silently monitor PR state changes ac ### Session Start On session start, the plugin: + 1. Discovers all sibling git repositories (multi-project support) 2. Finds open PRs for each repo's current branch 3. Fetches a full state snapshot for each PR @@ -34,6 +35,7 @@ On session start, the plugin: ### Post Tool Use After each tool use, the plugin: + 1. Re-fetches state for all tracked PRs 2. Compares against the cached snapshot 3. If changes are detected, reports them via stdout (shown as additionalContext) @@ -48,8 +50,8 @@ On session stop, performs a final state check for any last-minute changes. ```yaml # In plugins.settings.yaml (project or user level) github: - prStateTracking: true # Enable/disable (default: true) - prStateCacheDir: "" # Custom cache dir (default: ~/.claude/plugin-cache/github) + prStateTracking: true # Enable/disable (default: true) + prStateCacheDir: "" # Custom cache dir (default: ~/.claude/plugin-cache/github) ``` ### Cache Structure From b1d5b7d78d664d8ae1d8109d12c9076e2cefd36d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 24 Mar 2026 23:58:25 +0000 Subject: [PATCH 03/11] fix(github): address PR review feedback on async hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix review_decision field mapping (.auto_merge was wrong, REST API doesn't expose reviewDecision — removed the field) - Fix array expansion syntax in pr-discover.sh (use "${dirs[@]}") - Add throttle/debounce for PostToolUse (60s cooldown, configurable via prStateCheckInterval setting) - Remove dead code at end of pr-state-check.sh (both branches exited 0 with misleading comment about exit 2) - Fix misleading "parallel" comment — API calls are sequential - Add tilde expansion for user-configured cache directory paths - Remove unused hook-output.sh symlink (YAGNI) - Update docs to reflect throttle interval and tilde support https://claude.ai/code/session_01HJDTfa1KwAnxW1oFVgHqvc --- plugins/github/README.md | 6 +++- plugins/github/github.settings.yaml | 6 ++++ .../github/hooks/scripts/lib/pr-discover.sh | 2 +- plugins/github/hooks/scripts/lib/pr-state.sh | 6 ++-- .../github/hooks/scripts/pr-state-check.sh | 36 ++++++++++++------- plugins/github/lib/hook-output.sh | 1 - .../github/skills/pr-state-tracking/SKILL.md | 8 ++--- 7 files changed, 43 insertions(+), 22 deletions(-) delete mode 120000 plugins/github/lib/hook-output.sh diff --git a/plugins/github/README.md b/plugins/github/README.md index 99d20e44c..c44c35e67 100644 --- a/plugins/github/README.md +++ b/plugins/github/README.md @@ -35,7 +35,7 @@ The plugin includes async hooks that monitor PR state changes across all project **SessionStart**: Discovers all active PRs for the session's projects and establishes a baseline snapshot of their state (comments, reviews, CI checks, body, merge status). -**PostToolUse**: After each tool use, silently re-fetches PR state and compares against the cached snapshot. When changes are detected (e.g., a new review was submitted, CI completed, a comment was added), the agent is notified with details about what changed. +**PostToolUse**: Periodically re-fetches PR state (throttled to once per `prStateCheckInterval` seconds, default 60s) and compares against the cached snapshot. When changes are detected (e.g., a new review was submitted, CI completed, a comment was added), the agent is notified with details about what changed. **Stop**: Performs a final state check before session end. @@ -116,6 +116,7 @@ github: # PR state tracking prStateTracking: true + prStateCheckInterval: 60 # seconds between PostToolUse checks # prStateCacheDir: "~/.claude/plugin-cache/github" ``` @@ -129,8 +130,11 @@ For project-specific overrides: # In $CLAUDE_PROJECT_DIR/.claude/plugins.settings.yaml github: prStateCacheDir: "~/.claude/plugin-cache/github/my-project" + prStateCheckInterval: 120 # check less frequently ``` +Both `~` and `$HOME` are supported in `prStateCacheDir` paths. + When installed at the user level, the plugin handles multiple projects automatically by using the project directory name as a cache key. Each project's PRs are tracked independently. ## Local Sessions diff --git a/plugins/github/github.settings.yaml b/plugins/github/github.settings.yaml index 849535e6b..a9b177cb9 100644 --- a/plugins/github/github.settings.yaml +++ b/plugins/github/github.settings.yaml @@ -37,10 +37,16 @@ github: # Enable/disable PR state tracking hooks. prStateTracking: true + # Minimum seconds between PostToolUse checks (throttle/debounce). + # Prevents excessive API calls on every tool use. + # SessionStart and Stop always run regardless of this interval. + prStateCheckInterval: 60 + # Cache directory for PR state snapshots. # Default: ~/.claude/plugin-cache/github # The plugin appends //pr-state/ to this path. # For multi-project sessions, each project gets its own subdirectory. + # Both ~ and $HOME are supported in paths. # # Suggested structure for project-specific overrides: # ~/.claude/plugin-cache/github//pr-state/ diff --git a/plugins/github/hooks/scripts/lib/pr-discover.sh b/plugins/github/hooks/scripts/lib/pr-discover.sh index 7d79df673..a61a7ec66 100644 --- a/plugins/github/hooks/scripts/lib/pr-discover.sh +++ b/plugins/github/hooks/scripts/lib/pr-discover.sh @@ -43,7 +43,7 @@ pr_discover_all() { fi # For each directory, find the current branch's PR - for dir in "${#dirs[@]+${dirs[@]}}"; do + for dir in "${dirs[@]}"; do [ -d "$dir/.git" ] || continue _pr_discover_for_dir "$dir" done diff --git a/plugins/github/hooks/scripts/lib/pr-state.sh b/plugins/github/hooks/scripts/lib/pr-state.sh index 5ff63e19f..d668d8ad0 100644 --- a/plugins/github/hooks/scripts/lib/pr-state.sh +++ b/plugins/github/hooks/scripts/lib/pr-state.sh @@ -74,10 +74,11 @@ _pr_state_fetch() { gh_hostname_flag="--hostname github.com" fi - # Fetch PR details, reviews, comments, and check runs in parallel + # Fetch PR details, reviews, comments, and check runs sequentially local pr_json review_json comment_json checks_json - # PR core data (body, state, mergeable, title, labels, draft, reviewDecision) + # PR core data (body, state, mergeable, title, labels, draft) + # Note: reviewDecision is only available via GraphQL, not REST API pr_json="$(gh api ${gh_hostname_flag} \ "repos/${owner}/${repo}/pulls/${pr_number}" \ --jq '{ @@ -89,7 +90,6 @@ _pr_state_fetch() { mergeable_state: .mergeable_state, merged: .merged, merge_commit_sha: .merge_commit_sha, - review_decision: .auto_merge, labels: [.labels[].name], head_sha: .head.sha, updated_at: .updated_at diff --git a/plugins/github/hooks/scripts/pr-state-check.sh b/plugins/github/hooks/scripts/pr-state-check.sh index 482eb684e..b477b1317 100644 --- a/plugins/github/hooks/scripts/pr-state-check.sh +++ b/plugins/github/hooks/scripts/pr-state-check.sh @@ -7,7 +7,7 @@ # CI failure, new comment), outputs a message to inform the agent. # # On SessionStart: establishes baseline state for all discovered PRs -# On PostToolUse: checks for changes since last fetch +# On PostToolUse: checks for changes since last fetch (throttled) # On Stop: final check and summary of any pending changes # # Environment: @@ -28,11 +28,11 @@ source "${SCRIPT_DIR}/lib/pr-discover.sh" pr_state_enabled="$(plugin_get_config "prStateTracking" "true")" cache_base="$(plugin_get_config "prStateCacheDir" "")" +check_interval="$(plugin_get_config "prStateCheckInterval" "60")" # --- Guards --- if [ "$pr_state_enabled" = "false" ]; then - # Consume stdin and exit silently cat > /dev/null exit 0 fi @@ -53,6 +53,9 @@ if [ -z "$cache_base" ]; then cache_base="${HOME}/.claude/plugin-cache/github" fi +# Expand tilde in user-configured paths +cache_base="${cache_base/#\~/$HOME}" + # Add project-specific subdirectory based on project dir name if [ -n "${CLAUDE_PROJECT_DIR:-}" ]; then project_slug="$(basename "$CLAUDE_PROJECT_DIR")" @@ -70,6 +73,23 @@ hook_input="$(cat)" hook_event="${HOOK_EVENT:-PostToolUse}" +# --- Throttle for PostToolUse --- +# PostToolUse fires on every tool use. To avoid excessive API calls, +# enforce a cooldown period (default 60s) between checks. + +if [ "$hook_event" = "PostToolUse" ]; then + last_check_file="${cache_dir}/.last-check" + now="$(date +%s)" + 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" +fi + # --- Discover PRs --- log_info "Checking PR state (event: ${hook_event})" @@ -120,14 +140,6 @@ fi done echo "" echo "Review these changes and determine if any action is needed." -} > /dev/stdout +} -# For PostToolUse and Stop hooks, exit 2 to provide feedback to Claude -# For SessionStart, exit 0 with the message in stdout -if [ "$hook_event" = "SessionStart" ]; then - exit 0 -else - # Exit 2 signals "feedback" for Stop hooks (shown to Claude) - # For PostToolUse, stdout is advisory (additionalContext) - exit 0 -fi +exit 0 diff --git a/plugins/github/lib/hook-output.sh b/plugins/github/lib/hook-output.sh deleted file mode 120000 index 4c67a420b..000000000 --- a/plugins/github/lib/hook-output.sh +++ /dev/null @@ -1 +0,0 @@ -../../../shared/lib/hook-output.sh \ No newline at end of file diff --git a/plugins/github/skills/pr-state-tracking/SKILL.md b/plugins/github/skills/pr-state-tracking/SKILL.md index ed02cb876..75380f9ff 100644 --- a/plugins/github/skills/pr-state-tracking/SKILL.md +++ b/plugins/github/skills/pr-state-tracking/SKILL.md @@ -34,8 +34,7 @@ On session start, the plugin: ### Post Tool Use -After each tool use, the plugin: - +After each tool use (throttled to once per `prStateCheckInterval` seconds, default 60s): 1. Re-fetches state for all tracked PRs 2. Compares against the cached snapshot 3. If changes are detected, reports them via stdout (shown as additionalContext) @@ -50,8 +49,9 @@ On session stop, performs a final state check for any last-minute changes. ```yaml # In plugins.settings.yaml (project or user level) github: - prStateTracking: true # Enable/disable (default: true) - prStateCacheDir: "" # Custom cache dir (default: ~/.claude/plugin-cache/github) + prStateTracking: true # Enable/disable (default: true) + prStateCheckInterval: 60 # Seconds between PostToolUse checks (default: 60) + prStateCacheDir: "" # Custom cache dir (default: ~/.claude/plugin-cache/github) ``` ### Cache Structure From 97190a6add3cf90b39494006f4e6ef93fbcbd744 Mon Sep 17 00:00:00 2001 From: nsheaps <1282393+nsheaps@users.noreply.github.com> Date: Wed, 25 Mar 2026 00:00:09 +0000 Subject: [PATCH 04/11] chore: `mise run lint` --- plugins/github/README.md | 4 ++-- plugins/github/skills/pr-state-tracking/SKILL.md | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/plugins/github/README.md b/plugins/github/README.md index c44c35e67..8fcdfd323 100644 --- a/plugins/github/README.md +++ b/plugins/github/README.md @@ -116,7 +116,7 @@ github: # PR state tracking prStateTracking: true - prStateCheckInterval: 60 # seconds between PostToolUse checks + prStateCheckInterval: 60 # seconds between PostToolUse checks # prStateCacheDir: "~/.claude/plugin-cache/github" ``` @@ -130,7 +130,7 @@ For project-specific overrides: # In $CLAUDE_PROJECT_DIR/.claude/plugins.settings.yaml github: prStateCacheDir: "~/.claude/plugin-cache/github/my-project" - prStateCheckInterval: 120 # check less frequently + prStateCheckInterval: 120 # check less frequently ``` Both `~` and `$HOME` are supported in `prStateCacheDir` paths. diff --git a/plugins/github/skills/pr-state-tracking/SKILL.md b/plugins/github/skills/pr-state-tracking/SKILL.md index 75380f9ff..e1d8424e8 100644 --- a/plugins/github/skills/pr-state-tracking/SKILL.md +++ b/plugins/github/skills/pr-state-tracking/SKILL.md @@ -35,6 +35,7 @@ On session start, the plugin: ### Post Tool Use After each tool use (throttled to once per `prStateCheckInterval` seconds, default 60s): + 1. Re-fetches state for all tracked PRs 2. Compares against the cached snapshot 3. If changes are detected, reports them via stdout (shown as additionalContext) @@ -49,9 +50,9 @@ On session stop, performs a final state check for any last-minute changes. ```yaml # In plugins.settings.yaml (project or user level) github: - prStateTracking: true # Enable/disable (default: true) - prStateCheckInterval: 60 # Seconds between PostToolUse checks (default: 60) - prStateCacheDir: "" # Custom cache dir (default: ~/.claude/plugin-cache/github) + prStateTracking: true # Enable/disable (default: true) + prStateCheckInterval: 60 # Seconds between PostToolUse checks (default: 60) + prStateCacheDir: "" # Custom cache dir (default: ~/.claude/plugin-cache/github) ``` ### Cache Structure From 901b850f7e1db26e7b24af3d49ddcf236a5f17a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 25 Mar 2026 00:48:04 +0000 Subject: [PATCH 05/11] =?UTF-8?q?fix(github):=20address=20review=20iterati?= =?UTF-8?q?on=20findings=20=E2=80=94=20simplicity,=20security,=20perf?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplicity: - Consolidate ~20 individual jq calls in _pr_state_diff into a single jq call that extracts all comparable fields and outputs tab-separated diffs - Replace O(N²) _pr_state_diff_checks with a single jq call using from_entries to join old and new check arrays by name - Remove dead code: pr_state_changes_summary (never called), unused PLUGIN_NAME variable in main script Security: - Add _pr_validate_identifier to reject owner/repo values containing path traversal characters (../, /, etc.) — only [A-Za-z0-9._-] allowed - Validate head_sha matches hex SHA format before using in API URL - Set cache directory permissions to 700 (mkdir -m 700) Performance: - Move throttle check before sourcing heavy libraries (pr-state.sh, pr-discover.sh) — most PostToolUse invocations now exit after reading only plugin-config-read.sh - Consume stdin before any other work to avoid blocking Best Practices: - Fix operator precedence bug: `[ = main ] || [ = master ] && return 0` now uses explicit if/then to ensure both branches are skipped - Atomic cache writes: write to tmp file then mv to prevent corruption - Atomic throttle timestamp writes - Validate check_interval is a positive integer before arithmetic https://claude.ai/code/session_01HJDTfa1KwAnxW1oFVgHqvc --- .../github/hooks/scripts/lib/pr-discover.sh | 19 +- plugins/github/hooks/scripts/lib/pr-state.sh | 325 +++++++++--------- .../github/hooks/scripts/pr-state-check.sh | 53 ++- 3 files changed, 202 insertions(+), 195 deletions(-) diff --git a/plugins/github/hooks/scripts/lib/pr-discover.sh b/plugins/github/hooks/scripts/lib/pr-discover.sh index a61a7ec66..62ba03ab5 100644 --- a/plugins/github/hooks/scripts/lib/pr-discover.sh +++ b/plugins/github/hooks/scripts/lib/pr-discover.sh @@ -14,6 +14,13 @@ if [ "${_PR_DISCOVER_LOADED:-}" = "true" ]; then fi _PR_DISCOVER_LOADED="true" +# Validate that a string contains only safe GitHub identifier characters. +# Args: $1=string to validate +# Returns: 0 if safe, 1 if not +_pr_validate_identifier() { + [[ "$1" =~ ^[A-Za-z0-9._-]+$ ]] +} + # Discover PRs for all projects in the current session. # Checks CLAUDE_PROJECT_DIR and scans for sibling project dirs # that are git repos on branches with open PRs. @@ -54,17 +61,25 @@ pr_discover_all() { # Output: "owner repo pr_number" on stdout (one per PR found) _pr_discover_for_dir() { local dir="$1" - local branch remote_url owner repo pr_number + local branch remote_url pr_number # Get current branch branch="$(git -C "$dir" rev-parse --abbrev-ref HEAD 2>/dev/null)" || return 0 [ "$branch" = "HEAD" ] && return 0 - [ "$branch" = "main" ] || [ "$branch" = "master" ] && return 0 + # Skip default branches — no PRs to track + if [ "$branch" = "main" ] || [ "$branch" = "master" ]; then + return 0 + fi # Get remote URL and extract owner/repo remote_url="$(git -C "$dir" config --get remote.origin.url 2>/dev/null)" || return 0 _pr_extract_owner_repo "$remote_url" || return 0 + # Validate extracted values to prevent path traversal / injection + if ! _pr_validate_identifier "$_PR_OWNER" || ! _pr_validate_identifier "$_PR_REPO"; then + return 1 + fi + # Find open PR for this branch local gh_hostname_flag="" if [ "${CLAUDE_CODE_REMOTE:-}" = "true" ]; then diff --git a/plugins/github/hooks/scripts/lib/pr-state.sh b/plugins/github/hooks/scripts/lib/pr-state.sh index d668d8ad0..cf4040acf 100644 --- a/plugins/github/hooks/scripts/lib/pr-state.sh +++ b/plugins/github/hooks/scripts/lib/pr-state.sh @@ -28,7 +28,7 @@ declare -a PR_STATE_CHANGES=() # Args: $1=cache_dir pr_state_init() { _PR_STATE_CACHE_DIR="$1" - mkdir -p "$_PR_STATE_CACHE_DIR" + mkdir -p -m 700 "$_PR_STATE_CACHE_DIR" PR_STATE_CHANGES=() } @@ -50,8 +50,10 @@ pr_state_fetch_and_compare() { # Fetch new state new_state="$(_pr_state_fetch "$owner" "$repo" "$pr_number")" || return 0 - # Save new state - echo "$new_state" > "$cache_file" + # Atomic cache write: write to temp file then rename + local tmp_file="${cache_file}.tmp.$$" + echo "$new_state" > "$tmp_file" + mv -f "$tmp_file" "$cache_file" # If no old state, this is the first fetch — no changes to report if [ -z "$old_state" ]; then @@ -114,10 +116,10 @@ _pr_state_fetch() { --jq '[.[] | {user: .user.login, body: .body, path: .path, created_at: .created_at, id: .id}]' \ 2>/dev/null)" || review_comment_json="[]" - # Check runs for head SHA + # Check runs for head SHA — validate SHA format before using in URL local head_sha head_sha="$(echo "$pr_json" | jq -r '.head_sha // empty')" - if [ -n "$head_sha" ]; then + if [ -n "$head_sha" ] && [[ "$head_sha" =~ ^[0-9a-f]{7,40}$ ]]; then checks_json="$(gh api ${gh_hostname_flag} \ "repos/${owner}/${repo}/commits/${head_sha}/check-runs" \ --jq '{ @@ -147,184 +149,175 @@ _pr_state_fetch() { } # Compare old and new state, populating PR_STATE_CHANGES array. +# Uses a single jq call to extract all comparable fields from both states, +# then compares in bash to generate human-readable change messages. # Args: $1=old_state_json $2=new_state_json $3=owner $4=repo $5=pr_number # Returns: 0 if no changes, 1 if changes detected _pr_state_diff() { local old="$1" new="$2" owner="$3" repo="$4" pr_number="$5" + local prefix="${owner}/${repo}#${pr_number}" PR_STATE_CHANGES=() - # Compare PR body - local old_body new_body - old_body="$(echo "$old" | jq -r '.pr.body // ""')" - new_body="$(echo "$new" | jq -r '.pr.body // ""')" - if [ "$old_body" != "$new_body" ]; then - PR_STATE_CHANGES+=("PR body updated on ${owner}/${repo}#${pr_number}") - fi - - # Compare PR title - local old_title new_title - old_title="$(echo "$old" | jq -r '.pr.title // ""')" - new_title="$(echo "$new" | jq -r '.pr.title // ""')" - if [ "$old_title" != "$new_title" ]; then - PR_STATE_CHANGES+=("PR title changed: '${old_title}' -> '${new_title}' on ${owner}/${repo}#${pr_number}") - fi - - # Compare draft status - local old_draft new_draft - old_draft="$(echo "$old" | jq -r '.pr.draft // false')" - new_draft="$(echo "$new" | jq -r '.pr.draft // false')" - if [ "$old_draft" != "$new_draft" ]; then - if [ "$new_draft" = "true" ]; then - PR_STATE_CHANGES+=("PR ${owner}/${repo}#${pr_number} converted to draft") - else - PR_STATE_CHANGES+=("PR ${owner}/${repo}#${pr_number} marked ready for review") - fi - fi - - # Compare PR state (open/closed/merged) - local old_state new_state old_merged new_merged - old_state="$(echo "$old" | jq -r '.pr.state // ""')" - new_state="$(echo "$new" | jq -r '.pr.state // ""')" - old_merged="$(echo "$old" | jq -r '.pr.merged // false')" - new_merged="$(echo "$new" | jq -r '.pr.merged // false')" - if [ "$old_state" != "$new_state" ]; then - if [ "$new_merged" = "true" ] && [ "$old_merged" != "true" ]; then - PR_STATE_CHANGES+=("PR ${owner}/${repo}#${pr_number} was MERGED") - else - PR_STATE_CHANGES+=("PR state changed: ${old_state} -> ${new_state} on ${owner}/${repo}#${pr_number}") - fi - fi - - # Compare mergeable status - local old_mergeable new_mergeable old_mergeable_state new_mergeable_state - old_mergeable="$(echo "$old" | jq -r '.pr.mergeable // ""')" - new_mergeable="$(echo "$new" | jq -r '.pr.mergeable // ""')" - old_mergeable_state="$(echo "$old" | jq -r '.pr.mergeable_state // ""')" - new_mergeable_state="$(echo "$new" | jq -r '.pr.mergeable_state // ""')" - if [ "$old_mergeable" != "$new_mergeable" ] || [ "$old_mergeable_state" != "$new_mergeable_state" ]; then - PR_STATE_CHANGES+=("Merge status changed on ${owner}/${repo}#${pr_number}: mergeable=${old_mergeable}->${new_mergeable}, state=${old_mergeable_state}->${new_mergeable_state}") + # Extract all comparable fields from both old and new state in a single jq call. + # Output is a tab-separated line per field: "field_name\told_value\tnew_value" + local diff_output + diff_output="$(jq -r -n \ + --argjson old "$old" \ + --argjson new "$new" \ + ' + def s: . // ""; + def sorted_labels: [. // [] | sort[]]; + + # Scalar PR fields + [ + ["body", ($old.pr.body | s), ($new.pr.body | s)], + ["title", ($old.pr.title | s), ($new.pr.title | s)], + ["draft", ($old.pr.draft | tostring), ($new.pr.draft | tostring)], + ["state", ($old.pr.state | s), ($new.pr.state | s)], + ["merged", ($old.pr.merged | tostring), ($new.pr.merged | tostring)], + ["mergeable", ($old.pr.mergeable | s), ($new.pr.mergeable | s)], + ["mergeable_state", ($old.pr.mergeable_state | s), ($new.pr.mergeable_state | s)], + ["labels", ($old.pr.labels | sorted_labels | tojson), ($new.pr.labels | sorted_labels | tojson)], + ["review_count", ($old.reviews | length | tostring), ($new.reviews | length | tostring)], + ["comment_count", ($old.comments | length | tostring), ($new.comments | length | tostring)], + ["rc_count", ($old.review_comments | length | tostring),($new.review_comments | length | tostring)], + ["checks", ([$old.checks.checks // [] | sort_by(.name)[] | {name,status,conclusion}] | tojson), + ([$new.checks.checks // [] | sort_by(.name)[] | {name,status,conclusion}] | tojson)] + ] + | .[] | select(.[1] != .[2]) | @tsv + ')" || return 0 + + # No differences found + if [ -z "$diff_output" ]; then + return 0 fi - # Compare labels - local old_labels new_labels - old_labels="$(echo "$old" | jq -c '[.pr.labels // [] | sort[]]')" - new_labels="$(echo "$new" | jq -c '[.pr.labels // [] | sort[]]')" - if [ "$old_labels" != "$new_labels" ]; then - PR_STATE_CHANGES+=("Labels changed on ${owner}/${repo}#${pr_number}: ${old_labels} -> ${new_labels}") - fi + # Process each changed field + while IFS=$'\t' read -r field old_val new_val; do + case "$field" in + body) + PR_STATE_CHANGES+=("PR body updated on ${prefix}") + ;; + title) + PR_STATE_CHANGES+=("PR title changed: '${old_val}' -> '${new_val}' on ${prefix}") + ;; + draft) + if [ "$new_val" = "true" ]; then + PR_STATE_CHANGES+=("PR ${prefix} converted to draft") + else + PR_STATE_CHANGES+=("PR ${prefix} marked ready for review") + fi + ;; + state) + if [ "$new_val" = "closed" ] && _pr_check_merged "$new" ; then + PR_STATE_CHANGES+=("PR ${prefix} was MERGED") + else + PR_STATE_CHANGES+=("PR state changed: ${old_val} -> ${new_val} on ${prefix}") + fi + ;; + merged) + # Handled by state change above; only fire if state didn't change + ;; + mergeable|mergeable_state) + PR_STATE_CHANGES+=("Merge status changed on ${prefix}: ${field}=${old_val}->${new_val}") + ;; + labels) + PR_STATE_CHANGES+=("Labels changed on ${prefix}: ${old_val} -> ${new_val}") + ;; + review_count) + _pr_state_diff_new_reviews "$new" "$old_val" "$prefix" + ;; + comment_count) + _pr_state_diff_new_comments "$new" "$old_val" "$prefix" "comments" "comment" + ;; + rc_count) + _pr_state_diff_new_review_comments "$new" "$old_val" "$prefix" + ;; + checks) + _pr_state_diff_checks_unified "$old" "$new" "$prefix" + ;; + esac + done <<< "$diff_output" - # Compare reviews (count and latest states) - local old_review_count new_review_count - old_review_count="$(echo "$old" | jq '.reviews | length')" - new_review_count="$(echo "$new" | jq '.reviews | length')" - if [ "$old_review_count" != "$new_review_count" ]; then - local new_reviews - new_reviews="$(echo "$new" | jq -r ".reviews[$old_review_count:][] | \"\(.user) \(.state)\"" 2>/dev/null || true)" - if [ -n "$new_reviews" ]; then - while IFS= read -r review_line; do - PR_STATE_CHANGES+=("New review on ${owner}/${repo}#${pr_number}: ${review_line}") - done <<< "$new_reviews" - else - PR_STATE_CHANGES+=("Reviews changed on ${owner}/${repo}#${pr_number}: ${old_review_count} -> ${new_review_count} reviews") - fi - fi + [ ${#PR_STATE_CHANGES[@]} -gt 0 ] && return 1 || return 0 +} - # Compare comments (count) - local old_comment_count new_comment_count - old_comment_count="$(echo "$old" | jq '.comments | length')" - new_comment_count="$(echo "$new" | jq '.comments | length')" - if [ "$old_comment_count" != "$new_comment_count" ]; then - local new_comments - new_comments="$(echo "$new" | jq -r ".comments[$old_comment_count:][] | \"\(.user): \(.body[0:100])\"" 2>/dev/null || true)" - if [ -n "$new_comments" ]; then - while IFS= read -r comment_line; do - PR_STATE_CHANGES+=("New comment on ${owner}/${repo}#${pr_number}: ${comment_line}") - done <<< "$new_comments" - else - PR_STATE_CHANGES+=("Comments changed on ${owner}/${repo}#${pr_number}: ${old_comment_count} -> ${new_comment_count} comments") - fi - fi +# Check if a PR state JSON shows merged=true +_pr_check_merged() { + local state="$1" + [ "$(echo "$state" | jq -r '.pr.merged')" = "true" ] +} - # Compare review comments (inline code comments) - local old_rc_count new_rc_count - old_rc_count="$(echo "$old" | jq '.review_comments | length')" - new_rc_count="$(echo "$new" | jq '.review_comments | length')" - if [ "$old_rc_count" != "$new_rc_count" ]; then - local new_rcs - new_rcs="$(echo "$new" | jq -r ".review_comments[$old_rc_count:][] | \"\(.user) on \(.path): \(.body[0:100])\"" 2>/dev/null || true)" - if [ -n "$new_rcs" ]; then - while IFS= read -r rc_line; do - PR_STATE_CHANGES+=("New review comment on ${owner}/${repo}#${pr_number}: ${rc_line}") - done <<< "$new_rcs" - else - PR_STATE_CHANGES+=("Review comments changed on ${owner}/${repo}#${pr_number}: ${old_rc_count} -> ${new_rc_count}") - fi +# Extract and report new reviews (single jq call). +_pr_state_diff_new_reviews() { + local new="$1" old_count="$2" prefix="$3" + local new_reviews + new_reviews="$(echo "$new" | jq -r --argjson skip "$old_count" \ + '.reviews[$skip:][] | "\(.user) \(.state)"' 2>/dev/null || true)" + if [ -n "$new_reviews" ]; then + while IFS= read -r line; do + PR_STATE_CHANGES+=("New review on ${prefix}: ${line}") + done <<< "$new_reviews" + else + PR_STATE_CHANGES+=("Reviews changed on ${prefix}") fi +} - # Compare CI check results - local old_checks new_checks - old_checks="$(echo "$old" | jq -c '[.checks.checks // [] | sort_by(.name)[] | {name, status, conclusion}]')" - new_checks="$(echo "$new" | jq -c '[.checks.checks // [] | sort_by(.name)[] | {name, status, conclusion}]')" - if [ "$old_checks" != "$new_checks" ]; then - # Find specific check changes - local check_diff - check_diff="$(_pr_state_diff_checks "$old" "$new")" - if [ -n "$check_diff" ]; then - while IFS= read -r check_line; do - PR_STATE_CHANGES+=("CI on ${owner}/${repo}#${pr_number}: ${check_line}") - done <<< "$check_diff" - else - PR_STATE_CHANGES+=("CI status changed on ${owner}/${repo}#${pr_number}") - fi +# Extract and report new comments (single jq call). +_pr_state_diff_new_comments() { + local new="$1" old_count="$2" prefix="$3" field="$4" label="$5" + local new_items + new_items="$(echo "$new" | jq -r --argjson skip "$old_count" \ + ".${field}"'[$skip:][] | "\(.user): \(.body[0:100])"' 2>/dev/null || true)" + if [ -n "$new_items" ]; then + while IFS= read -r line; do + PR_STATE_CHANGES+=("New ${label} on ${prefix}: ${line}") + done <<< "$new_items" + else + PR_STATE_CHANGES+=("${label}s changed on ${prefix}") fi - - # Return whether changes were detected - [ ${#PR_STATE_CHANGES[@]} -gt 0 ] && return 1 || return 0 } -# Diff individual check runs between old and new state. -# Args: $1=old_state_json $2=new_state_json -# Returns: human-readable diff lines via stdout -_pr_state_diff_checks() { - local old="$1" new="$2" - - # Get all check names from both states - local all_checks - all_checks="$(jq -r -n \ - --argjson old "$(echo "$old" | jq '.checks.checks // []')" \ - --argjson new "$(echo "$new" | jq '.checks.checks // []')" \ - '[$old[].name, $new[].name] | unique[]')" - - while IFS= read -r check_name; do - [ -z "$check_name" ] && continue - local old_status old_conclusion new_status new_conclusion - old_status="$(echo "$old" | jq -r --arg name "$check_name" '.checks.checks[] | select(.name == $name) | .status // "missing"' | head -1)" - old_conclusion="$(echo "$old" | jq -r --arg name "$check_name" '.checks.checks[] | select(.name == $name) | .conclusion // "pending"' | head -1)" - new_status="$(echo "$new" | jq -r --arg name "$check_name" '.checks.checks[] | select(.name == $name) | .status // "missing"' | head -1)" - new_conclusion="$(echo "$new" | jq -r --arg name "$check_name" '.checks.checks[] | select(.name == $name) | .conclusion // "pending"' | head -1)" - - [ -z "$old_status" ] && old_status="missing" - [ -z "$new_status" ] && new_status="missing" - [ -z "$old_conclusion" ] && old_conclusion="pending" - [ -z "$new_conclusion" ] && new_conclusion="pending" - - if [ "$old_status/$old_conclusion" != "$new_status/$new_conclusion" ]; then - echo "${check_name}: ${old_status}/${old_conclusion} -> ${new_status}/${new_conclusion}" - fi - done <<< "$all_checks" +# Extract and report new review comments (single jq call). +_pr_state_diff_new_review_comments() { + local new="$1" old_count="$2" prefix="$3" + local new_rcs + new_rcs="$(echo "$new" | jq -r --argjson skip "$old_count" \ + '.review_comments[$skip:][] | "\(.user) on \(.path): \(.body[0:100])"' 2>/dev/null || true)" + if [ -n "$new_rcs" ]; then + while IFS= read -r line; do + PR_STATE_CHANGES+=("New review comment on ${prefix}: ${line}") + done <<< "$new_rcs" + else + PR_STATE_CHANGES+=("Review comments changed on ${prefix}") + fi } -# Get a human-readable summary of all changes. -# Returns: formatted string via stdout -pr_state_changes_summary() { - if [ ${#PR_STATE_CHANGES[@]} -eq 0 ]; then - echo "No changes detected" - return +# Diff CI check runs using a single jq call that joins old and new by name. +# Args: $1=old_state_json $2=new_state_json $3=prefix +_pr_state_diff_checks_unified() { + local old="$1" new="$2" prefix="$3" + local check_diff + check_diff="$(jq -r -n \ + --argjson old_checks "$(echo "$old" | jq '.checks.checks // []')" \ + --argjson new_checks "$(echo "$new" | jq '.checks.checks // []')" \ + ' + # Index checks by name + def by_name: [.[] | {key: .name, value: {s: .status, c: (.conclusion // "pending")}}] | from_entries; + ($old_checks | by_name) as $o | + ($new_checks | by_name) as $n | + ([$o | keys[], $n | keys[]] | unique[]) as $name | + ($o[$name] // {s:"missing",c:"pending"}) as $ov | + ($n[$name] // {s:"missing",c:"pending"}) as $nv | + select("\($ov.s)/\($ov.c)" != "\($nv.s)/\($nv.c)") | + "\($name): \($ov.s)/\($ov.c) -> \($nv.s)/\($nv.c)" + ')" || true + + if [ -n "$check_diff" ]; then + while IFS= read -r line; do + PR_STATE_CHANGES+=("CI on ${prefix}: ${line}") + done <<< "$check_diff" + else + PR_STATE_CHANGES+=("CI status changed on ${prefix}") fi - - local summary="PR state changes detected:\n" - for change in "${PR_STATE_CHANGES[@]}"; do - summary+=" - ${change}\n" - done - printf '%b' "$summary" } diff --git a/plugins/github/hooks/scripts/pr-state-check.sh b/plugins/github/hooks/scripts/pr-state-check.sh index b477b1317..f407d34ec 100644 --- a/plugins/github/hooks/scripts/pr-state-check.sh +++ b/plugins/github/hooks/scripts/pr-state-check.sh @@ -16,38 +16,30 @@ # CLAUDE_PLUGIN_ROOT — plugin root for accessing libs set -euo pipefail -PLUGIN_NAME="github" SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${CLAUDE_PLUGIN_ROOT}/lib/plugin-config-read.sh" -source "${CLAUDE_PLUGIN_ROOT}/lib/log.sh" -source "${SCRIPT_DIR}/lib/pr-state.sh" -source "${SCRIPT_DIR}/lib/pr-discover.sh" +# --- Consume stdin early --- +# Hook input is provided via stdin; consume it to avoid broken pipe +hook_input="$(cat)" -# --- Read config --- +# --- Minimal config for early exit checks --- +# Source only what's needed for guards and throttle — defer heavy libs +PLUGIN_NAME="github" +source "${CLAUDE_PLUGIN_ROOT}/lib/plugin-config-read.sh" pr_state_enabled="$(plugin_get_config "prStateTracking" "true")" -cache_base="$(plugin_get_config "prStateCacheDir" "")" -check_interval="$(plugin_get_config "prStateCheckInterval" "60")" - -# --- Guards --- - if [ "$pr_state_enabled" = "false" ]; then - cat > /dev/null exit 0 fi -if ! command -v gh &>/dev/null; then - cat > /dev/null +if ! command -v gh &>/dev/null || ! command -v jq &>/dev/null; then exit 0 fi -if ! command -v jq &>/dev/null; then - cat > /dev/null - exit 0 -fi +# --- Resolve cache directory (needed for throttle check) --- -# --- Resolve cache directory --- +cache_base="$(plugin_get_config "prStateCacheDir" "")" +check_interval="$(plugin_get_config "prStateCheckInterval" "60")" if [ -z "$cache_base" ]; then cache_base="${HOME}/.claude/plugin-cache/github" @@ -63,11 +55,6 @@ else project_slug="default" fi cache_dir="${cache_base}/${project_slug}/pr-state" -pr_state_init "$cache_dir" - -# --- Consume stdin --- -# Hook input is provided via stdin; consume it to avoid broken pipe -hook_input="$(cat)" # --- Determine hook event --- @@ -76,20 +63,32 @@ hook_event="${HOOK_EVENT:-PostToolUse}" # --- Throttle for PostToolUse --- # PostToolUse fires on every tool use. To avoid excessive API calls, # enforce a cooldown period (default 60s) between checks. +# This runs BEFORE sourcing heavy libraries for performance. if [ "$hook_event" = "PostToolUse" ]; then + mkdir -p -m 700 "$cache_dir" last_check_file="${cache_dir}/.last-check" now="$(date +%s)" if [ -f "$last_check_file" ]; then last_check="$(cat "$last_check_file")" - elapsed=$((now - last_check)) - if [ "$elapsed" -lt "$check_interval" ]; then + # Validate check_interval is a positive integer + if [[ "$check_interval" =~ ^[0-9]+$ ]] && [ "$((now - last_check))" -lt "$check_interval" ]; then exit 0 fi fi - echo "$now" > "$last_check_file" + # Atomic timestamp write + echo "$now" > "${last_check_file}.tmp.$$" + mv -f "${last_check_file}.tmp.$$" "$last_check_file" fi +# --- Source heavy libraries (only reached when we actually need to check) --- + +source "${CLAUDE_PLUGIN_ROOT}/lib/log.sh" +source "${SCRIPT_DIR}/lib/pr-state.sh" +source "${SCRIPT_DIR}/lib/pr-discover.sh" + +pr_state_init "$cache_dir" + # --- Discover PRs --- log_info "Checking PR state (event: ${hook_event})" From d0fb3393913bc1ea7501fa14619fed8e87fc2442 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 25 Mar 2026 00:56:19 +0000 Subject: [PATCH 06/11] =?UTF-8?q?fix(github):=20improve=20PR=20state=20tra?= =?UTF-8?q?cking=20=E2=80=94=20hook=20patterns,=20tests,=20usability?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Convert output to hook-logging.sh pattern (repo conventions) - Fix jq 1.7 compat bug in check diff (from_entries + unique keys) - Add human-readable label change messages (was raw JSON) - Add missing jq/gh CLI warning on SessionStart - Add cache TTL pruning (7-day stale file cleanup) - Add 35 automated unit tests for URL parsing, validation, and state diffing https://claude.ai/code/session_01HJDTfa1KwAnxW1oFVgHqvc --- plugins/github/hooks/scripts/lib/pr-state.sh | 10 +- .../github/hooks/scripts/pr-state-check.sh | 29 ++- plugins/github/hooks/scripts/test-pr-state.sh | 240 ++++++++++++++++++ 3 files changed, 265 insertions(+), 14 deletions(-) create mode 100755 plugins/github/hooks/scripts/test-pr-state.sh diff --git a/plugins/github/hooks/scripts/lib/pr-state.sh b/plugins/github/hooks/scripts/lib/pr-state.sh index cf4040acf..337dcd1ed 100644 --- a/plugins/github/hooks/scripts/lib/pr-state.sh +++ b/plugins/github/hooks/scripts/lib/pr-state.sh @@ -222,7 +222,11 @@ _pr_state_diff() { PR_STATE_CHANGES+=("Merge status changed on ${prefix}: ${field}=${old_val}->${new_val}") ;; labels) - PR_STATE_CHANGES+=("Labels changed on ${prefix}: ${old_val} -> ${new_val}") + # Convert JSON arrays to human-readable comma-separated lists + local old_labels new_labels + old_labels="$(echo "$old_val" | jq -r 'join(", ")' 2>/dev/null || echo "$old_val")" + new_labels="$(echo "$new_val" | jq -r 'join(", ")' 2>/dev/null || echo "$new_val")" + PR_STATE_CHANGES+=("Labels changed on ${prefix}: [${old_labels}] -> [${new_labels}]") ;; review_count) _pr_state_diff_new_reviews "$new" "$old_val" "$prefix" @@ -302,11 +306,11 @@ _pr_state_diff_checks_unified() { --argjson old_checks "$(echo "$old" | jq '.checks.checks // []')" \ --argjson new_checks "$(echo "$new" | jq '.checks.checks // []')" \ ' - # Index checks by name + # Index checks by name, merge to get union of all check names def by_name: [.[] | {key: .name, value: {s: .status, c: (.conclusion // "pending")}}] | from_entries; ($old_checks | by_name) as $o | ($new_checks | by_name) as $n | - ([$o | keys[], $n | keys[]] | unique[]) as $name | + ($o + $n | keys[]) as $name | ($o[$name] // {s:"missing",c:"pending"}) as $ov | ($n[$name] // {s:"missing",c:"pending"}) as $nv | select("\($ov.s)/\($ov.c)" != "\($nv.s)/\($nv.c)") | diff --git a/plugins/github/hooks/scripts/pr-state-check.sh b/plugins/github/hooks/scripts/pr-state-check.sh index f407d34ec..77de0422e 100644 --- a/plugins/github/hooks/scripts/pr-state-check.sh +++ b/plugins/github/hooks/scripts/pr-state-check.sh @@ -33,6 +33,10 @@ if [ "$pr_state_enabled" = "false" ]; then fi if ! command -v gh &>/dev/null || ! command -v jq &>/dev/null; then + # Log a one-time warning on SessionStart so the user knows tracking is disabled + if [ "${HOOK_EVENT:-}" = "SessionStart" ]; then + echo "github: PR state tracking disabled — missing $(command -v gh &>/dev/null || echo 'gh')$(command -v jq &>/dev/null || echo ' jq') CLI" + fi exit 0 fi @@ -83,12 +87,16 @@ fi # --- Source heavy libraries (only reached when we actually need to check) --- -source "${CLAUDE_PLUGIN_ROOT}/lib/log.sh" +source "${CLAUDE_PLUGIN_ROOT}/lib/hook-logging.sh" source "${SCRIPT_DIR}/lib/pr-state.sh" source "${SCRIPT_DIR}/lib/pr-discover.sh" pr_state_init "$cache_dir" +# --- Prune stale cache files (older than 7 days) --- + +find "$cache_dir" -name "*.json" -mtime +7 -delete 2>/dev/null || true + # --- Discover PRs --- log_info "Checking PR state (event: ${hook_event})" @@ -97,6 +105,7 @@ pr_list="$(pr_discover_all 2>/dev/null)" || pr_list="" if [ -z "$pr_list" ]; then log_info "No active PRs found for tracked projects" + hook_respond exit 0 fi @@ -125,20 +134,18 @@ log_info "Checked ${pr_count} PR(s)" if [ ${#all_changes[@]} -eq 0 ]; then # No changes — silent success if [ "$hook_event" = "SessionStart" ]; then - echo "github: PR state baseline established for ${pr_count} PR(s)" + hook_log "PR state baseline established for ${pr_count} PR(s)" fi + hook_respond exit 0 fi # Changes detected — build output message -{ - echo "github: PR state changes detected since last check:" - echo "" - for change in "${all_changes[@]}"; do - echo " - ${change}" - done - echo "" - echo "Review these changes and determine if any action is needed." -} +hook_log "PR state changes detected since last check:" +for change in "${all_changes[@]}"; do + hook_log " - ${change}" +done +hook_log "Review these changes and determine if any action is needed." +hook_respond exit 0 diff --git a/plugins/github/hooks/scripts/test-pr-state.sh b/plugins/github/hooks/scripts/test-pr-state.sh new file mode 100755 index 000000000..5b30959ff --- /dev/null +++ b/plugins/github/hooks/scripts/test-pr-state.sh @@ -0,0 +1,240 @@ +#!/usr/bin/env bash +# test-pr-state.sh — Unit tests for PR state tracking libraries +# +# Tests pure logic functions (URL parsing, validation, state diffing) +# without requiring GitHub API access. +# +# Usage: bash test-pr-state.sh +set -uo pipefail +# Note: errexit (-e) is intentionally disabled because functions under test +# return non-zero to indicate "changes detected" which is expected behavior. + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PASS=0 +FAIL=0 + +# --- Test helpers --- + +assert_eq() { + local label="$1" expected="$2" actual="$3" + if [ "$expected" = "$actual" ]; then + PASS=$((PASS + 1)) + else + FAIL=$((FAIL + 1)) + echo "FAIL: ${label}" + echo " expected: ${expected}" + echo " actual: ${actual}" + fi +} + +assert_ok() { + local label="$1" + shift + if "$@" 2>/dev/null; then + PASS=$((PASS + 1)) + else + FAIL=$((FAIL + 1)) + echo "FAIL: ${label} (expected success, got failure)" + fi +} + +assert_fail() { + local label="$1" + shift + if "$@" 2>/dev/null; then + FAIL=$((FAIL + 1)) + echo "FAIL: ${label} (expected failure, got success)" + else + PASS=$((PASS + 1)) + fi +} + +# --- Stub dependencies --- + +# Stub CLAUDE_PLUGIN_ROOT and log.sh for sourcing +export CLAUDE_PLUGIN_ROOT="${SCRIPT_DIR}/../.." +LOG_PREFIX="test" + +# Minimal log stubs (avoid sourcing actual log.sh which writes to stderr) +log_info() { :; } +log_warn() { :; } +log_error() { :; } + +# Source the libraries under test +source "${SCRIPT_DIR}/lib/pr-discover.sh" + +# pr-state.sh needs jq; skip if not available +if ! command -v jq &>/dev/null; then + echo "SKIP: jq not available, skipping pr-state tests" + exit 0 +fi + +source "${SCRIPT_DIR}/lib/pr-state.sh" + +# ============================================================ +# Test: _pr_extract_owner_repo +# ============================================================ + +echo "--- _pr_extract_owner_repo ---" + +_pr_extract_owner_repo "https://github.com/nsheaps/ai-mktpl.git" +assert_eq "HTTPS .git URL owner" "nsheaps" "$_PR_OWNER" +assert_eq "HTTPS .git URL repo" "ai-mktpl" "$_PR_REPO" + +_pr_extract_owner_repo "https://github.com/nsheaps/ai-mktpl" +assert_eq "HTTPS URL owner" "nsheaps" "$_PR_OWNER" +assert_eq "HTTPS URL repo" "ai-mktpl" "$_PR_REPO" + +_pr_extract_owner_repo "git@github.com:nsheaps/ai-mktpl.git" +assert_eq "SSH .git URL owner" "nsheaps" "$_PR_OWNER" +assert_eq "SSH .git URL repo" "ai-mktpl" "$_PR_REPO" + +_pr_extract_owner_repo "git@github.com:nsheaps/ai-mktpl" +assert_eq "SSH URL owner" "nsheaps" "$_PR_OWNER" +assert_eq "SSH URL repo" "ai-mktpl" "$_PR_REPO" + +_pr_extract_owner_repo "http://127.0.0.1:8080/git/nsheaps/ai-mktpl" +assert_eq "Web proxy URL owner" "nsheaps" "$_PR_OWNER" +assert_eq "Web proxy URL repo" "ai-mktpl" "$_PR_REPO" + +assert_fail "Invalid URL returns failure" _pr_extract_owner_repo "https://gitlab.com/foo/bar" + +# ============================================================ +# Test: _pr_validate_identifier +# ============================================================ + +echo "--- _pr_validate_identifier ---" + +assert_ok "Valid owner" _pr_validate_identifier "nsheaps" +assert_ok "Valid repo with dots" _pr_validate_identifier "ai-mktpl.test" +assert_ok "Valid with underscore" _pr_validate_identifier "my_repo" +assert_fail "Path traversal" _pr_validate_identifier "../etc/passwd" +assert_fail "Slash in name" _pr_validate_identifier "owner/repo" +assert_fail "Empty string" _pr_validate_identifier "" +assert_fail "Spaces" _pr_validate_identifier "my repo" +assert_fail "Semicolon injection" _pr_validate_identifier "repo;rm -rf /" + +# ============================================================ +# Test: _pr_state_diff — no changes +# ============================================================ + +echo "--- _pr_state_diff ---" + +CACHE_DIR="$(mktemp -d)" +pr_state_init "$CACHE_DIR" + +state_a='{"pr":{"title":"Test PR","body":"hello","draft":false,"state":"open","merged":false,"mergeable":true,"mergeable_state":"clean","labels":["bug"],"head_sha":"abc123","updated_at":"2026-01-01"},"reviews":[],"comments":[],"review_comments":[],"checks":{"total_count":1,"checks":[{"name":"ci","status":"completed","conclusion":"success"}]},"fetched_at":"2026-01-01T00:00:00Z"}' + +_pr_state_diff "$state_a" "$state_a" "owner" "repo" "1" +assert_eq "Identical states — no changes" "0" "${#PR_STATE_CHANGES[@]}" + +# ============================================================ +# Test: _pr_state_diff — title change +# ============================================================ + +state_b="$(echo "$state_a" | jq '.pr.title = "Updated PR"')" +_pr_state_diff "$state_a" "$state_b" "owner" "repo" "1" +assert_eq "Title change detected" "1" "${#PR_STATE_CHANGES[@]}" + +# ============================================================ +# Test: _pr_state_diff — body change +# ============================================================ + +state_c="$(echo "$state_a" | jq '.pr.body = "new body"')" +_pr_state_diff "$state_a" "$state_c" "owner" "repo" "1" +assert_eq "Body change detected" "1" "${#PR_STATE_CHANGES[@]}" + +# ============================================================ +# Test: _pr_state_diff — draft toggle +# ============================================================ + +state_d="$(echo "$state_a" | jq '.pr.draft = true')" +_pr_state_diff "$state_a" "$state_d" "owner" "repo" "1" +assert_eq "Draft change count" "1" "${#PR_STATE_CHANGES[@]}" +echo "${PR_STATE_CHANGES[0]}" | grep -q "converted to draft" +assert_eq "Draft change message" "0" "$?" + +# ============================================================ +# Test: _pr_state_diff — new comment +# ============================================================ + +state_e="$(echo "$state_a" | jq '.comments = [{"user":"alice","body":"looks good","created_at":"2026-01-02","id":1}]')" +_pr_state_diff "$state_a" "$state_e" "owner" "repo" "1" +assert_eq "New comment detected" "1" "${#PR_STATE_CHANGES[@]}" + +# ============================================================ +# Test: _pr_state_diff — CI status change +# ============================================================ + +state_f="$(echo "$state_a" | jq '.checks.checks[0].conclusion = "failure"')" +_pr_state_diff "$state_a" "$state_f" "owner" "repo" "1" +assert_eq "CI change detected" "1" "${#PR_STATE_CHANGES[@]}" +echo "${PR_STATE_CHANGES[0]}" | grep -q "CI on" +assert_eq "CI change message prefix" "0" "$?" + +# ============================================================ +# Test: _pr_state_diff — label change (human-readable) +# ============================================================ + +state_g="$(echo "$state_a" | jq '.pr.labels = ["bug", "enhancement"]')" +_pr_state_diff "$state_a" "$state_g" "owner" "repo" "1" +assert_eq "Label change detected" "1" "${#PR_STATE_CHANGES[@]}" +echo "${PR_STATE_CHANGES[0]}" | grep -q '\[bug\]' +assert_eq "Label output is human-readable (not raw JSON)" "0" "$?" + +# ============================================================ +# Test: _pr_state_diff — merged PR +# ============================================================ + +state_h="$(echo "$state_a" | jq '.pr.state = "closed" | .pr.merged = true')" +_pr_state_diff "$state_a" "$state_h" "owner" "repo" "1" +local_merged=0 +for change in "${PR_STATE_CHANGES[@]}"; do + echo "$change" | grep -q "MERGED" && local_merged=1 +done +assert_eq "Merged PR detected" "1" "$local_merged" + +# ============================================================ +# Test: _pr_state_diff — new review +# ============================================================ + +state_i="$(echo "$state_a" | jq '.reviews = [{"user":"bob","state":"APPROVED","submitted_at":"2026-01-02","body":"lgtm"}]')" +_pr_state_diff "$state_a" "$state_i" "owner" "repo" "1" +assert_eq "New review detected" "1" "${#PR_STATE_CHANGES[@]}" +echo "${PR_STATE_CHANGES[0]}" | grep -q "bob APPROVED" +assert_eq "Review includes user and state" "0" "$?" + +# ============================================================ +# Test: _pr_state_diff — new review comment +# ============================================================ + +state_j="$(echo "$state_a" | jq '.review_comments = [{"user":"carol","body":"nit: fix typo","path":"src/main.sh","created_at":"2026-01-02","id":1}]')" +_pr_state_diff "$state_a" "$state_j" "owner" "repo" "1" +assert_eq "New review comment detected" "1" "${#PR_STATE_CHANGES[@]}" +echo "${PR_STATE_CHANGES[0]}" | grep -q "carol on src/main.sh" +assert_eq "Review comment includes user and path" "0" "$?" + +# ============================================================ +# Test: _pr_state_diff — multiple simultaneous changes +# ============================================================ + +state_k="$(echo "$state_a" | jq '.pr.title = "New title" | .comments = [{"user":"dave","body":"hi","created_at":"2026-01-02","id":1}] | .checks.checks[0].conclusion = "failure"')" +_pr_state_diff "$state_a" "$state_k" "owner" "repo" "1" +assert_eq "Multiple changes detected (>=3)" "1" "$([ ${#PR_STATE_CHANGES[@]} -ge 3 ] && echo 1 || echo 0)" + +# --- Cleanup --- +rm -rf "$CACHE_DIR" + +# --- Summary --- +echo "" +echo "=== Test Results ===" +echo "Passed: ${PASS}" +echo "Failed: ${FAIL}" +echo "Total: $((PASS + FAIL))" + +if [ "$FAIL" -gt 0 ]; then + exit 1 +fi + +echo "All tests passed." +exit 0 From 7fc4162abcd453d3c4fe7dc60512e3c87109fe59 Mon Sep 17 00:00:00 2001 From: nsheaps <1282393+nsheaps@users.noreply.github.com> Date: Wed, 25 Mar 2026 00:57:38 +0000 Subject: [PATCH 07/11] chore: auto-bump plugin versions and update marketplace --- .claude-plugin/marketplace.json | 11 ++++++++--- plugins/github/.claude-plugin/plugin.json | 2 +- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 9b1536692..9e9c247b5 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -213,8 +213,8 @@ }, { "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.13", + "description": "GitHub CLI installation, authentication, PR state tracking, and workflow skill for Claude Code sessions. Monitors comments, reviews, CI status, and merge readiness across multi-project sessions.", + "version": "0.1.14", "author": { "name": "Nathan Heaps" }, @@ -233,7 +233,12 @@ "issues", "session-start", "web-session", - "auto-install" + "auto-install", + "pr-state-tracking", + "ci-status", + "reviews", + "async-hooks", + "multi-project" ] }, { diff --git a/plugins/github/.claude-plugin/plugin.json b/plugins/github/.claude-plugin/plugin.json index 4061ba7f3..aea111a4d 100644 --- a/plugins/github/.claude-plugin/plugin.json +++ b/plugins/github/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "github", - "version": "0.1.13", + "version": "0.1.14", "description": "GitHub CLI installation, authentication, PR state tracking, and workflow skill for Claude Code sessions. Monitors comments, reviews, CI status, and merge readiness across multi-project sessions.", "author": { "name": "Nathan Heaps", From db9e70c66b77ee5a167abd62dc0bbba472392f4d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 25 Mar 2026 00:58:16 +0000 Subject: [PATCH 08/11] docs: add PR #306 review reports from iterate-until-good https://claude.ai/code/session_01HJDTfa1KwAnxW1oFVgHqvc --- .../ai-mktpl/306/1774399314/OVERALL-REPORT.md | 81 +++++++ .../306/1774399314/best-practices/REPORT.md | 205 ++++++++++++++++++ .../306/1774399314/documentation/REPORT.md | 69 ++++++ .../306/1774399314/flexibility/REPORT.md | 81 +++++++ .../306/1774399314/qa-engineering/REPORT.md | 118 ++++++++++ .../306/1774399314/repo-patterns/REPORT.md | 81 +++++++ .../306/1774399314/security/REPORT.md | 154 +++++++++++++ .../306/1774399314/simplicity/REPORT.md | 114 ++++++++++ .../306/1774399314/usability/REPORT.md | 82 +++++++ 9 files changed, 985 insertions(+) create mode 100644 .claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/OVERALL-REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/best-practices/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/documentation/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/flexibility/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/qa-engineering/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/repo-patterns/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/security/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/simplicity/REPORT.md create mode 100644 .claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/usability/REPORT.md diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/OVERALL-REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/OVERALL-REPORT.md new file mode 100644 index 000000000..de80554db --- /dev/null +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/OVERALL-REPORT.md @@ -0,0 +1,81 @@ +# 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. diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/best-practices/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/best-practices/REPORT.md new file mode 100644 index 000000000..caa91b956 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/best-practices/REPORT.md @@ -0,0 +1,205 @@ +# 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) diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/documentation/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/documentation/REPORT.md new file mode 100644 index 000000000..d8516b147 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/documentation/REPORT.md @@ -0,0 +1,69 @@ +# Documentation Review — PR #306 +Score: 87/100 + +## Summary + +The documentation for this PR is thorough and well-structured across all layers. Shell scripts have clear file-level headers with purpose, usage, environment variable, and return value documentation. The README and SKILL.md are comprehensive, accurate, and complementary without being redundant. The settings YAML is exceptionally well-commented — arguably the best-documented file in the set. The PR body is clearly written but contains one stale reference to `lib/hook-output.sh`, a file that was removed during the review feedback cycle. There are minor discoverability gaps and a few inline comment accuracy issues that keep this from being a near-perfect documentation score. + +## Findings + +### Shell Script Headers and Function Documentation + +**pr-state-check.sh** (lines 1–18): Header clearly states purpose, the three hook events it serves, and all relevant environment variables (`HOOK_EVENT`, `CLAUDE_PROJECT_DIR`, `CLAUDE_PLUGIN_ROOT`). The `set -euo pipefail` declaration is visible and unambiguous. No issues. + +**pr-state.sh** (lines 1–15): Header documents purpose, usage pattern (source + init + fetch_and_compare), return value semantics (`0`/`1`), and the array side-effect (`PR_STATE_CHANGES`). Requirements section lists `gh` and `jq` explicitly. Function-level comments follow consistently throughout the file. One minor note: `_pr_state_diff_checks` (line 296) documents its output as "human-readable diff lines via stdout" but does not mention that it emits nothing when there are no changes — callers have to infer this from the surrounding code. + +**pr-discover.sh** (lines 1–9): Header is correct and succinct. The guard-against-double-sourcing pattern and the global `_PR_OWNER`/`_PR_REPO` side-effect variables are both noted inline. The note on the internal output variables (lines 75–77) is a helpful contract signal for maintainers. + +One inline comment accuracy issue: `pr-discover.sh` line 61 comments `[ "$branch" = "main" ] || [ "$branch" = "master" ] && return 0` — this is documented as "skip default branches" but the bash operator precedence here means only `master` is guarded by the `&&`. The `main` check is a standalone condition that always returns 0 if the branch is `main`. The logic works by accident (both OR branches evaluate before `&&` in practice for simple string comparisons) but the comment does not flag this fragility, and a reader relying on the comment alone could be misled. + +### README.md + +The README is the strongest documentation artifact in this PR. It: + +- Adds a clear `### GitHub CLI Installation` section header to separate the existing feature from the new one (previously the "How It Works" section jumped directly into numbered steps with no subheading). +- Provides a complete change-detection table with real-world example values. +- Correctly describes the three hook events and their throttle behavior. +- Accurately documents the cache path structure including the `//pr-state/` suffix appended by the plugin. +- The "Future: Claude Code Channels" section is clearly marked as speculative (`> **Planned feature**`) and does not overstate what the current code does. +- The `## Local Sessions` section is updated to note that PR state tracking runs on both local and web sessions, which matches the actual script behavior. + +Minor gap: The README does not document what happens when `CLAUDE_PROJECT_DIR` is unset (the script falls back to `project_slug="default"`). This edge case is handled gracefully in code but is invisible to users reading the docs. + +### SKILL.md + +The skill file is well-suited for consumption by AI agents. The frontmatter `description` field is concise and trigger-word-rich, appropriate for skill matching. The body covers all hook events, configuration keys with defaults, cache structure, requirements, and the future channels roadmap. Content is accurate and consistent with both the README and the implementation. + +The one structural gap: SKILL.md documents `prStateCacheDir: ""` (empty string as the shown default), while the README shows it commented-out with the actual default path as a comment. These are technically consistent (empty string means "use default") but a reader comparing the two docs could be confused about whether to set the key at all. + +### github.settings.yaml + +The settings file is the most thoroughly commented in the diff. The `--- PR State Tracking ---` block header, inline description of what the feature does, the throttle/debounce semantics note on `prStateCheckInterval`, the path-suffix clarification on `prStateCacheDir`, and the `~`/`$HOME` support note are all present and accurate. The commented-out `prStateCacheDir` key with its suggested structure is a good affordance. No issues. + +### Inline Comment Accuracy + +Beyond the `pr-discover.sh` operator-precedence issue noted above, all other inline comments are accurate. The comment in `pr-state.sh` at line 82 ("Note: reviewDecision is only available via GraphQL, not REST API") is a genuinely useful caveat for future maintainers. The throttle section comment in `pr-state-check.sh` (lines 71–74) correctly describes both the mechanism and the exceptions (SessionStart/Stop always run). + +### PR Body / Description + +The PR body is well-written: the summary bullet list accurately describes the feature, the changes table is complete and maps to real files, and the test plan checkboxes are specific and actionable. + +**Stale reference (confirmed defect)**: The Changes section lists `lib/hook-output.sh — Symlink to shared hook-output library` as a changed file. Inspecting the branch (`a5e2afe`), `plugins/github/lib/` contains `add-permission.sh`, `hook-logging.sh`, `log.sh`, `plugin-config-read.sh`, `safe-settings-write.sh`, and `tool-install.sh` — there is no `hook-output.sh` present. The file diff also shows no such entry. This entry was removed from the implementation following review feedback but was never removed from the PR body. It should be deleted from the Changes list. + +### Discoverability + +The plugin's `plugin.json` and `marketplace.json` keywords were expanded to include `pr-state-tracking`, `ci-status`, `reviews`, `async-hooks`, and `multi-project` — good for search-based discovery. The README feature list bullet is visible near the top of the document. + +One gap: there is no mention in the README of the `SKILL.md` file or the `skills/pr-state-tracking/` directory. Users browsing the README would not know the skill document exists. A one-line pointer under `## Skills` (e.g., "See `skills/pr-state-tracking/SKILL.md` for full agent-facing documentation") would close this. + +A secondary discoverability concern: the `prStateTracking: true` default means the feature is on by default for all users of this plugin, but the README only notes how to disable it in the context of project-specific overrides, not as a standalone "to opt out, set `prStateTracking: false`" statement. New users may not realize the feature is running. + +## References + +- PR: https://github.com/nsheaps/ai-mktpl/pull/306 +- `plugins/github/README.md` — https://github.com/nsheaps/ai-mktpl/blob/claude/github-async-hooks-1z1aZ/plugins/github/README.md +- `plugins/github/skills/pr-state-tracking/SKILL.md` — https://github.com/nsheaps/ai-mktpl/blob/claude/github-async-hooks-1z1aZ/plugins/github/skills/pr-state-tracking/SKILL.md +- `plugins/github/github.settings.yaml` — https://github.com/nsheaps/ai-mktpl/blob/claude/github-async-hooks-1z1aZ/plugins/github/github.settings.yaml +- `plugins/github/hooks/scripts/pr-state-check.sh` — https://github.com/nsheaps/ai-mktpl/blob/claude/github-async-hooks-1z1aZ/plugins/github/hooks/scripts/pr-state-check.sh +- `plugins/github/hooks/scripts/lib/pr-state.sh` — https://github.com/nsheaps/ai-mktpl/blob/claude/github-async-hooks-1z1aZ/plugins/github/hooks/scripts/lib/pr-state.sh +- `plugins/github/hooks/scripts/lib/pr-discover.sh` — https://github.com/nsheaps/ai-mktpl/blob/claude/github-async-hooks-1z1aZ/plugins/github/hooks/scripts/lib/pr-discover.sh diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/flexibility/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/flexibility/REPORT.md new file mode 100644 index 000000000..92fbd08da --- /dev/null +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/flexibility/REPORT.md @@ -0,0 +1,81 @@ +# Flexibility Review — PR #306 +Score: 78/100 + +## Summary + +This PR is well-designed for flexibility across its primary use cases. The three-setting configuration surface (enable/disable, interval, cache directory), combined with environment-adaptive behavior and thoughtful graceful degradation, gives users meaningful control without requiring code changes. The library separation between `pr-state.sh` and `pr-discover.sh` makes the feature modular and extensible. The score is held back by a small set of hardwired behaviors that cannot be overridden via config — notably the sibling-directory discovery strategy, the GitHub-only remote assumption, and the absence of any per-PR or per-repo filtering controls. These are not architectural flaws but rather gaps that will likely surface as soon as the feature is adopted at scale. + +## Findings + +### Strengths + +**1. Clean enable/disable with silent exit** +`pr-state-check.sh` lines 32–37 check `prStateTracking: false` and exit 0 immediately, consuming stdin to avoid a broken pipe. This is the correct pattern for a no-op hook — the agent never sees noise from a disabled feature. + +**2. Configurable throttle interval** +`prStateCheckInterval` (default 60s) is read from plugin config and applied to PostToolUse via a `.last-check` timestamp file (`pr-state-check.sh` lines 71–82). SessionStart and Stop bypass the throttle, which is the right behavior: baseline must always establish, and the final check on Stop should always run. Users needing a quieter experience can push this to 300 or higher in their `plugins.settings.yaml` without touching any script. + +**3. Three-tier cache directory resolution** +`pr-state-check.sh` lines 47–62 implement a clean fallback chain: user-configured `prStateCacheDir` > `$HOME/.claude/plugin-cache/github`. Tilde and `$HOME` are both expanded. The project slug appended from `CLAUDE_PROJECT_DIR` ensures multiple projects never collide in a shared cache base. This is flexible enough for shared-home environments and project-specific overrides alike. + +**4. Environment-adaptive gh flag** +Both `pr-state.sh` and `pr-discover.sh` check `CLAUDE_CODE_REMOTE` and conditionally apply `--hostname github.com` to all `gh api` calls. This transparently handles the web session proxy-remote pattern without any user configuration required. + +**5. Graceful degradation for missing dependencies** +`pr-state-check.sh` lines 39–48 guard against missing `gh` or `jq` with silent exits. `_pr_discover_for_dir` uses `|| return 0` at every fallible step (git branch, remote URL, PR lookup). `_pr_state_fetch` falls back to `"{}"` and `"[]"` on API errors. The feature fails open — no output is worse than crashing the hook. + +**6. Multi-project sibling discovery is automatic** +`pr-discover.sh` scans `$(dirname CLAUDE_PROJECT_DIR)/*/` for `.git` directories and adds them without requiring the user to enumerate repos. This zero-config multi-project support is a genuine flexibility win for the primary target environment (Claude Code web sessions with co-cloned repos). + +**7. Library architecture supports extension** +The split between `pr-state.sh` (fetch/cache/diff) and `pr-discover.sh` (enumerate repos) allows either to be extended or replaced independently. The `PR_STATE_CHANGES` array is a clean, inspectable output contract. A future channels integration would only need to replace the output section of `pr-state-check.sh` — the detection engine is already reusable. + +**8. Channels future-proofing is explicitly documented** +`README.md` and `SKILL.md` both document the planned channels integration pattern. The architectural note that "state diffs would become channel triggers" gives any future contributor a clear upgrade path without requiring a rewrite. + +--- + +### Limitations and Gaps + +**1. Discovery strategy is not configurable — no way to opt in to strict single-project mode** +`pr-discover.sh:pr_discover_all` always scans sibling directories when `CLAUDE_PROJECT_DIR` is set. There is no config key to say "only track the primary project" or "track these specific repos." In a dense home directory where many sibling folders happen to be git repos, this can produce a large and unintended PR list. A `prStateDiscoveryMode: auto|primary-only` or `prStateAdditionalDirs` config key would address this. + +**2. Default-on with no per-project override hint in the settings file** +`github.settings.yaml` sets `prStateTracking: true` as the default and comments out `prStateCacheDir`. The README documents project-level override correctly, but there is no example of turning off tracking for a specific project only (e.g., `prStateTracking: false` in a project's `.claude/plugins.settings.yaml`). Users inheriting this at the user level may not realize they can override it per project. + +**3. Branch filter is hardcoded to skip `main` and `master` only** +`pr-discover.sh` line 63: `[ "$branch" = "main" ] || [ "$branch" = "master" ] && return 0`. Repositories using `develop`, `trunk`, or a different default branch name will still emit a lookup attempt for that branch, burning an API call and potentially returning unexpected results if an old PR exists for it. There is no `prStateDefaultBranch` config or a `gh repo view --json defaultBranchRef` lookup to detect the actual default branch. + +**4. `_PR_OWNER` and `_PR_REPO` are global variables set by side effect** +`pr-discover.sh` lines 80–81 declare `_PR_OWNER=""` and `_PR_REPO=""` at script scope, written by `_pr_extract_owner_repo`. If `pr_discover_all` is ever called concurrently or sourced in a context with subshells, these globals are not safe. More importantly, if the function is ever tested or reused, the caller must know to read `_PR_OWNER`/`_PR_REPO` rather than a return value. A local output-via-stdout or nameref pattern would be more flexible. + +**5. PostToolUse timeout is 15 seconds, but the fetch makes 4–5 sequential API calls per PR** +`hooks.json` sets `"timeout": 15` for PostToolUse. `_pr_state_fetch` makes up to 5 sequential `gh api` calls (PR metadata, reviews, issue comments, review comments, check runs). On a session tracking 3–4 PRs across multiple repos, this can exceed 15 seconds on a slow connection. There is no `prStateFetchTimeout` config. The user has no way to tune the per-hook timeout without editing `hooks.json` directly. + +**6. No mechanism to exclude specific PRs or repos from tracking** +Once a sibling repo is discovered and has an open PR on the current branch, it is always tracked. There is no deny-list (`prStateIgnoreRepos`) or allow-list (`prStateTrackRepos`) config. A user who has a long-running draft PR in a sibling repo they are not actively working on will receive repeated "no changes" noise (or worse, frequent change notifications for unrelated work). + +**7. Cache invalidation has no TTL or manual clear mechanism** +Cache files grow indefinitely and are never pruned. A PR that is merged and its cache file never cleaned up will be re-compared on the next session if the branch is somehow recreated. There is no `prStateCacheMaxAge` config or a documented `rm -rf ~/.claude/plugin-cache/github//pr-state/` step in the README. + +**8. GitHub-only; no GitLab or Bitbucket support path** +`_pr_extract_owner_repo` explicitly matches `github.com` or the web session proxy format and returns 1 for all other remotes. This is fine for the current scope, but there is no abstraction layer (e.g., a `pr-provider.sh` interface) that would allow a non-GitHub remote to be supported without rewriting the discovery and fetch logic. Given the plugin is named "github" this is expected, but it is a ceiling worth noting. + +--- + +### Minor Observations + +- `pr-state-check.sh` line 66 reads stdin into `hook_input` but never uses it. This is correct (stdin must be consumed) but the variable assignment creates a mild confusion — a comment explaining why the value is intentionally discarded would help. +- The `pr_state_changes_summary` function in `pr-state.sh` (lines 310–320) is defined but never called from the main entry point, which instead formats its own output inline. The function is available for future use but is dead code today. +- The `all_checks` variable in `_pr_state_diff_checks` is populated via `<<<` here-string from `jq -r ... unique[]`. If `jq` returns an empty string (no checks), the `while` loop receives one empty line and the `[ -z "$check_name" ] && continue` guard on line 281 handles it. The guard is correct but the empty-string-from-heredoc edge case is non-obvious. + +## References + +- `plugins/github/hooks/scripts/pr-state-check.sh` — main entry point, config reading, throttle logic +- `plugins/github/hooks/scripts/lib/pr-state.sh` — fetch, cache, and diff engine +- `plugins/github/hooks/scripts/lib/pr-discover.sh` — multi-project PR enumeration +- `plugins/github/hooks/hooks.json` — hook registration and timeouts +- `plugins/github/github.settings.yaml` — config key definitions and defaults +- `plugins/github/README.md` — user-facing documentation including channels roadmap +- `plugins/github/skills/pr-state-tracking/SKILL.md` — skill-level documentation +- PR: https://github.com/nsheaps/ai-mktpl/pull/306 diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/qa-engineering/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/qa-engineering/REPORT.md new file mode 100644 index 000000000..8fac49cd4 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/qa-engineering/REPORT.md @@ -0,0 +1,118 @@ +# QA & Engineering Review — PR #306 +Score: 47/100 + +## Summary + +This PR adds async hook infrastructure for PR state tracking in the github plugin. The core idea is sound and the implementation is thorough in documentation, but at time of review the PR had seven open review threads — all of which were addressed in a subsequent fix commit (`862fbafeb43ca0ff9c63bafdf22b3b8540f74767`). However, the PR body was never updated to remove the stale `lib/hook-output.sh` entry from the Changes section, and the branch has a `mergeable_state` of "dirty" (requires rebase against main). There are no automated tests for any of the new shell scripts, and the test plan consists entirely of manual verification steps with no evidence of execution. The 5 sequential `gh` API calls per PR have no partial-failure recovery, meaning a single transient network failure mid-fetch results in silently stale cache state. The PostToolUse hook's `*` matcher fires on every tool use, addressed only by a timestamp throttle — not a targeted matcher — which could still cause rate-limit pressure in active sessions. + +## Findings + +### 1. PR Body Inaccuracy — Stale File Reference +The Changes section of the PR body still lists: +> `lib/hook-output.sh` — Symlink to shared hook-output library + +This file was removed in commit `862fbafeb` per the "Remove unused hook-output.sh symlink (YAGNI)" fix. The PR body was never updated to reflect this removal. The file does not appear in any changed-files listing, confirming it is absent from the final branch state. The test plan and summary sections are otherwise accurate. + +### 2. Dirty Merge State — Rebase Required +`mergeable_state: "dirty"` at head SHA `a5e2afe8dd87220f2c3685bcd3edb5ac5f09c0b3`. The branch needs a rebase onto `main` before merge. Since the PR is still draft, this is expected but must be resolved before promotion. + +### 3. No Automated Tests +Zero test files exist for any of the three new shell scripts: +- `plugins/github/hooks/scripts/pr-state-check.sh` +- `plugins/github/hooks/scripts/lib/pr-state.sh` +- `plugins/github/hooks/scripts/lib/pr-discover.sh` + +The test plan is 7 manual checkbox items, none marked as completed. There is no bats, shellspec, or similar framework invocation. CI passes only `lint` and `validate` checks — neither exercises the new functionality. Given that the core logic (JSON diffing, cache file management, throttle state, multi-repo discovery) is all testable in isolation, the absence of tests is a significant gap for production reliability. + +### 4. Partial-Failure State Corruption in `_pr_state_fetch` +`plugins/github/hooks/scripts/lib/pr-state.sh` (lines ~85–155) makes 5 sequential `gh api` calls. The failure handling is asymmetric: + +- Call 1 (PR core data): `|| { echo "{}" ; return 1; }` — returns 1, and the caller (`pr_state_fetch_and_compare`) traps this with `|| return 0`, so no cache write occurs. This is correct. +- Calls 2–4 (reviews, comments, review comments): `|| review_json="[]"` / `|| comment_json="[]"` / `|| review_comment_json="[]"` — silently substitutes empty arrays on failure and continues. +- Call 5 (check runs): `|| checks_json='{"total_count":0,"checks":[]}'` — same pattern. + +The problem: if call 1 succeeds but call 2 (reviews) fails transiently, the function continues with `review_json="[]"`, combines all state into JSON, and **writes the incomplete snapshot to the cache file**. On the next invocation, the old (complete) cache is replaced by the new (incomplete) one. The diff logic then reports "all reviews disappeared" — generating false-positive change notifications — or worse, silently misses real changes because the baseline is now wrong. + +A correct approach would either: +- Track whether any secondary call failed and skip the cache write, or +- Record a `fetch_error` flag in the JSON to suppress diffs for that invocation. + +### 5. Uncaught Failure Modes: `jq` Combining Step +After all 5 API calls, the function runs a `jq -n` to combine results (pr-state.sh, ~lines 157–170). This step has no `||` guard. If `jq` fails (e.g., malformed JSON from one of the `|| fallback` paths), the function exits non-zero, but `pr_state_fetch_and_compare` has already passed the point where it guards on fetch failure. Depending on bash `set -e` propagation through sourced libs, this may silently exit the entire hook rather than returning a controlled error. + +### 6. Comment Count Diffing — Assumes Append-Only +`plugins/github/hooks/scripts/lib/pr-state.sh` (lines ~210–240) detects new reviews, comments, and review comments by comparing array lengths and slicing from `[$old_count:]`. This assumes comments are only ever added, never deleted. If a comment is deleted externally (GitHub allows this), the new count may be lower, the slice index becomes negative or out-of-range, and `jq` emits nothing — the deletion is silently ignored with no notification. The current implementation explicitly does not detect deletions. + +### 7. Review Deduplication Not Handled +The reviews diff (pr-state.sh ~lines 195–210) compares total review count. GitHub's reviews endpoint returns a full history including superseded reviews (e.g., a reviewer who approved, then requested changes, then approved again will appear 3 times). Count-based diffing will report a new review event even if the reviewer merely re-submitted the same state, and will miss review state _changes_ within the same reviewer's review record if no new review object is appended. + +### 8. Shell Logic Bug: Operator Precedence in Branch Skip +`plugins/github/hooks/scripts/lib/pr-discover.sh` (line ~63): +```bash +[ "$branch" = "main" ] || [ "$branch" = "master" ] && return 0 +``` +Due to bash's left-to-right evaluation with equal precedence for `||` and `&&`, this parses as: +```bash +([ "$branch" = "main" ]) || ([ "$branch" = "master" ] && return 0) +``` +If `branch` is `main`, the first test is true, the `||` short-circuits, and `return 0` is NOT executed — the function continues processing the main branch as if it had a PR. The fix commit (`862fbafeb`) does not appear to have addressed this. The correct form is: +```bash +{ [ "$branch" = "main" ] || [ "$branch" = "master" ]; } && return 0 +``` + +### 9. Unquoted Variable in `gh api` Call +`plugins/github/hooks/scripts/lib/pr-discover.sh` (line ~75) and `pr-state.sh` (lines ~90, ~100, etc.): +```bash +gh api ${gh_hostname_flag} \ +``` +`${gh_hostname_flag}` is unquoted. When empty, this is fine. When set to `--hostname github.com`, it expands to two words correctly. However, with `set -u` active (via `set -euo pipefail` in `pr-state-check.sh`), if `gh_hostname_flag` were ever unset (not just empty) this would error. The variable is always initialized to `""` so this is low risk but is a style violation under `set -u` conventions. + +### 10. PostToolUse Timeout May Be Too Short +`plugins/github/hooks/hooks.json` (line 27): PostToolUse timeout is 15 seconds. With 5 sequential API calls per PR (each with network latency), a multi-repo session tracking 3 PRs could require 15+ API calls. On a slow network or GitHub API slowdown, the hook will time out mid-fetch, leaving partial state. The throttle ensures this doesn't happen every tool call, but the first execution after the 60s window may still exceed 15s for multi-PR sessions. + +### 11. CI Status: All Checks Pass +- `lint`: completed/success +- `validate`: completed/success +- `auto-version-bump`: completed/success +- `bump-and-update-marketplace`: skipped (expected for draft) +- `claude-review`: skipped (expected for draft) + +No CI failures. The lint check validates the new shell scripts pass shellcheck (implicitly, given the lint job passes). + +### 12. Commit Quality +The 5 commits on the branch are: +1. `e1c5f3ab` — `feat(github): add async hooks for PR state tracking` — well-scoped initial implementation +2. `35642ca6` — `chore: mise run lint` — atomic, appropriate +3. `8f7ca2f9` — `chore: auto-bump plugin versions and update marketplace` — automation bot, appropriate +4. `121d91b2` — merged commit from PR #300 (mise fix + scm-utils rename) — **this commit is unrelated to the PR's stated purpose** and includes work from a separate PR; it is a history artifact from rebasing/squashing +5. `862fbafeb` — `fix(github): address PR review feedback on async hooks` — addresses 6 of 7 review threads; well-described + +Commit 4 is the most notable: it includes the mise absolute-path fix and the auto-pr-to-making-great-prs rename, both of which are logically separate features. This inflates the PR's diff and makes the history harder to bisect. + +### 13. Open Review Threads Not All Resolved +Of the 7 original review threads from `henry-nsheaps`: +- Threads marked `is_outdated: true` (5 of 7): addressed in the fix commit and superseded by code changes +- `hooks.json` PostToolUse matcher thread: `is_outdated: false`, **not resolved** — the throttle was added, but the `*` matcher remains; the thread author's suggestion to use a targeted matcher was not implemented +- `README.md` tilde expansion thread: `is_outdated: false`, **not resolved** — tilde expansion was added to the script (`pr-state-check.sh:52`) but the thread was not marked resolved + +### 14. Test Plan Gaps +The test plan does not cover: +- Behavior when `gh` API returns an error mid-sequence (partial fetch) +- Comment/review deletion handling +- The `main`/`master` branch skip logic +- Behavior with repos that have no remote or a non-GitHub remote +- Rate limit handling (HTTP 403/429 from `gh`) +- Cache file corruption or invalid JSON in cache +- The `$ARGUMENTS`-less path in `/fix-pr` command (unrelated but included in this PR) + +## References + +- PR: https://github.com/nsheaps/ai-mktpl/pull/306 +- Head commit: https://github.com/nsheaps/ai-mktpl/commit/a5e2afe8dd87220f2c3685bcd3edb5ac5f09c0b3 +- Fix commit (review feedback): https://github.com/nsheaps/ai-mktpl/commit/862fbafeb43ca0ff9c63bafdf22b3b8540f74767 +- `pr-state.sh`: `plugins/github/hooks/scripts/lib/pr-state.sh` +- `pr-discover.sh`: `plugins/github/hooks/scripts/lib/pr-discover.sh` +- `pr-state-check.sh`: `plugins/github/hooks/scripts/pr-state-check.sh` +- `hooks.json`: `plugins/github/hooks/hooks.json` +- Open review thread (PostToolUse matcher): https://github.com/nsheaps/ai-mktpl/pull/306#discussion_r2983942600 +- Open review thread (tilde expansion): https://github.com/nsheaps/ai-mktpl/pull/306#discussion_r2983944156 diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/repo-patterns/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/repo-patterns/REPORT.md new file mode 100644 index 000000000..e1cd51935 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/repo-patterns/REPORT.md @@ -0,0 +1,81 @@ +# Repo Patterns Review — PR #306 +Score: 84/100 + +## Summary + +The PR introduces async PR state tracking hooks to the github plugin and is largely well-aligned with existing repo patterns. The hooks.json format, double-source guard pattern in library files, settings YAML structure, and SKILL.md frontmatter all match established conventions. The most notable deviations are: the new hook scripts bypass the repo's `hook-logging.sh` / `hook_respond` / `hook_log_cleanup` infrastructure in favour of a lighter-weight approach using `log.sh` directly with plain `echo` output, and the new libraries live under `hooks/scripts/lib/` rather than the plugin-level `lib/` where shared shell libraries normally reside. One item from the PR description (a `lib/hook-output.sh` symlink) is listed as a change but does not appear on the branch at all, suggesting either an incomplete commit or stale description. These are genuine but non-blocking deviations from established patterns; nothing is broken, but the approach is inconsistent with how existing hooks are structured. + +## Findings + +### hooks.json format — MATCHES + +The updated `plugins/github/hooks/hooks.json` follows the exact schema used by all other plugins: a top-level `description` string, a `hooks` object keyed by event name, each event containing an array of matcher objects with `matcher`, `hooks[]`, `type`, `command`, and `timeout` fields. The `matcher: "*"` wildcard is consistent with `plugins/mise/hooks/hooks.json` and `plugins/scm-utils/hooks/hooks.json`. Timeouts (30 s for SessionStart, 15 s for PostToolUse/Stop) are reasonable and distinct from the 60 s install hook, matching the convention of differentiating heavy vs. lightweight operations. + +### Script structure — PARTIAL DEVIATION + +Existing hook scripts (`install-gh.sh`, `install-mise.sh`) follow a strict pattern: +1. `PLUGIN_NAME=""` +2. `source "${CLAUDE_PLUGIN_ROOT}/lib/plugin-config-read.sh"` +3. `source "${CLAUDE_PLUGIN_ROOT}/lib/tool-install.sh"` +4. `source "${CLAUDE_PLUGIN_ROOT}/lib/hook-logging.sh"` +5. Guards using `plugin_is_enabled` and `tool_is_web_session` helper functions +6. A named `do_*` function wrapping all logic +7. `tool_run_install do_*` then `hook_log_cleanup` then `hook_respond` as the final exit sequence + +The new `pr-state-check.sh` deviates from this in several ways: + +- `plugins/github/hooks/scripts/pr-state-check.sh` line 3: sources `lib/log.sh` directly instead of `hook-logging.sh`, and uses bare `echo` to stdout for output rather than `hook_log` / `hook_respond`. The existing infrastructure accumulates messages in a temp file and emits them in a structured way via `hook_respond`; the new script bypasses this entirely. +- The guards use inline `command -v` checks and bare `exit 0` instead of `plugin_is_enabled` / `hook_log "... skipping"; hook_respond; exit 0` as seen in `install-gh.sh` lines 16–17. +- There is no `hook_log_cleanup` call at the end. +- The stdin consume (`hook_input="$(cat)"`) at line 48 is novel — existing hooks do not explicitly consume stdin. This is not wrong, but it is not a repo pattern; it is an introduced pattern. The comment says it avoids broken pipe, which is reasonable. + +The new library files `hooks/scripts/lib/pr-state.sh` and `hooks/scripts/lib/pr-discover.sh` are placed under `hooks/scripts/lib/` rather than the plugin-level `plugins/github/lib/`. All existing shared shell libraries (`log.sh`, `hook-logging.sh`, `plugin-config-read.sh`, etc.) live in `plugins//lib/` and are symlinked identically across plugins. Putting hook-specific libraries one level deeper under `hooks/scripts/lib/` is a new sub-pattern not established elsewhere. It is internally consistent within this PR but diverges from the repo's convention of keeping all shell libraries at `lib/`. + +### Double-source guard — MATCHES + +Both new library files use the repo's established guard idiom: +```bash +if [ "${_PR_STATE_LOADED:-}" = "true" ]; then + return 0 2>/dev/null || true +fi +_PR_STATE_LOADED="true" +``` +This exactly matches the pattern in `plugins/github/lib/log.sh` (`_LOG_SH_LOADED`) and `plugins/github/lib/hook-logging.sh` (`_HOOK_LOGGING_LOADED`). The guard variable naming convention (`__LOADED`) is consistent. + +### Settings YAML — MATCHES + +`plugins/github/github.settings.yaml` follows the existing pattern: a single top-level key matching the plugin name (`github:`), settings as indented scalar values, commented-out optional settings, and inline comments explaining each key. This matches `plugins/mise/mise.settings.yaml` structurally. The new keys (`prStateTracking`, `prStateCheckInterval`, `prStateCacheDir`) use the same camelCase convention as existing keys (`autoInstall`, `autoAuthCheck`, `backgroundInstall`). + +### SKILL.md frontmatter and structure — MATCHES + +`plugins/github/skills/pr-state-tracking/SKILL.md` uses the correct YAML frontmatter with `name:`, `description:` (multi-line `>` block), and `allowed-tools:` fields, matching the format of `plugins/github/skills/gh/SKILL.md` and `plugins/mise/skills/mise/SKILL.md`. The skill directory name matches the `name:` frontmatter field (`pr-state-tracking`). The document body uses H2 headings, code blocks, and a configuration table consistent with other skills. + +### Missing lib/hook-output.sh symlink — INCONSISTENCY WITH PR DESCRIPTION + +The PR description under "Changes" lists: `lib/hook-output.sh — Symlink to shared hook-output library`. However, on the PR branch (`a5e2afe`), `plugins/github/lib/` contains the same six files as on `main` — no `hook-output.sh` appears. The hook scripts reference `lib/log.sh` (which does exist) rather than any `hook-output.sh`. This is either a stale description item or an accidentally dropped commit. It is not a functional problem since the scripts work without it, but the PR description is misleading. + +### Plugin.json and marketplace.json version bumps — MATCHES + +Both `plugins/github/.claude-plugin/plugin.json` and `.claude-plugin/marketplace.json` are bumped from `0.1.12` to `0.1.13`, consistent with the repo's semver patch increment convention for additive changes. + +### hook_respond / stdout contract — DEVIATION + +The existing contract (documented at length in `hook-logging.sh`) is that hooks must always `exit 0` and must call `hook_respond` exactly once as the last statement, which writes accumulated messages to stdout. `pr-state-check.sh` writes directly to stdout with `echo` and exits with bare `exit 0` without calling `hook_respond`. This works at the Claude hook runtime level, but it is not following the repo's established stdout/stderr separation contract. Specifically, the existing pattern ensures that stray `echo` calls from called functions cannot pollute hook output, by redirecting function stdout to stderr inside `hook_run`. The new script has no such protection. + +## References + +- `plugins/github/hooks/hooks.json` — base pattern for hooks.json schema +- `plugins/mise/hooks/hooks.json` — reference for SessionStart-only hook +- `plugins/scm-utils/hooks/hooks.json` — reference for Stop hook +- `plugins/github/hooks/scripts/install-gh.sh` — reference for hook script structure and guard pattern +- `plugins/mise/hooks/scripts/install-mise.sh` — reference for hook script structure +- `plugins/github/lib/hook-logging.sh` — defines `hook_respond`, `hook_log`, `hook_log_cleanup` contract +- `plugins/github/lib/log.sh` — double-source guard reference +- `plugins/github/github.settings.yaml` — settings YAML pattern +- `plugins/mise/mise.settings.yaml` — settings YAML pattern +- `plugins/github/skills/gh/SKILL.md` — SKILL.md frontmatter reference +- `plugins/github/skills/pr-state-tracking/SKILL.md` — new skill (PR branch) +- `plugins/github/hooks/scripts/pr-state-check.sh` — new entry point (PR branch) +- `plugins/github/hooks/scripts/lib/pr-state.sh` — new library (PR branch) +- `plugins/github/hooks/scripts/lib/pr-discover.sh` — new library (PR branch) +- https://github.com/nsheaps/ai-mktpl/pull/306 diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/security/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/security/REPORT.md new file mode 100644 index 000000000..4cb071786 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/security/REPORT.md @@ -0,0 +1,154 @@ +# Security Review — PR #306 +Score: 62/100 + +## Summary + +PR #306 introduces async shell-based hooks for PR state tracking. The scripts are well-structured and use `set -euo pipefail` in the entry point, pass API data through `jq` rather than raw shell parsing, and prefer quoted variables throughout. However, there are several notable security issues: git remote URLs are parsed with unvalidated `sed` patterns whose output is used directly in filesystem paths and API URLs (path traversal risk); user-supplied and API-derived values are interpolated into filenames without sanitization; the `check_interval` config value is used in a bash arithmetic expression without integer validation; cache files and directories are created without explicit restrictive permissions; and a TOCTOU race in the throttle logic allows duplicate concurrent API calls. None of these rise to remote code execution given the threat model (local user running a dev tool), but several can cause data written outside the intended directory or unintended API calls, and the terminal-output injection via raw PR content is a low-severity concern. + +## Findings + +### F1 — Path Traversal via Parsed Remote URL (Medium) +**File:** `plugins/github/hooks/scripts/lib/pr-discover.sh`, lines 88–100 + +`_pr_extract_owner_repo` extracts `_PR_OWNER` and `_PR_REPO` from the git remote URL using `sed` with no sanitization or character-class restriction. The captured groups allow `/`, `..`, `~`, and other filesystem-significant characters. Both values flow directly into the cache filename in `pr-state.sh`: + +``` +local cache_file="${_PR_STATE_CACHE_DIR}/${owner}_${repo}_${pr_number}.json" +``` + +A crafted remote URL such as `https://github.com/owner/../../../tmp/evil/x` would set `_PR_REPO` to `../../../tmp/evil/x` (after `.git` stripping), causing the cache file to be written at `~/.claude/plugin-cache/github//pr-state/owner_../../../tmp/evil/x_N.json`, which resolves outside the cache tree. The same values are interpolated into the `gh api` URL path: + +``` +pr_number="$(gh api ${gh_hostname_flag} \ + "repos/${_PR_OWNER}/${_PR_REPO}/pulls?head=..." +``` + +Mitigation: validate that `_PR_OWNER` and `_PR_REPO` match `^[A-Za-z0-9_.-]+$` before use. Validate `pr_number` matches `^[0-9]+$`. + +--- + +### F2 — `head_sha` from API Response Used in URL Without Validation (Low-Medium) +**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines 118–124 + +`head_sha` is extracted from the GitHub API JSON response via `jq -r '.head_sha // empty'` and immediately used in a subsequent API call: + +``` +checks_json="$(gh api ${gh_hostname_flag} \ + "repos/${owner}/${repo}/commits/${head_sha}/check-runs" \ + ...)" +``` + +A SHA returned by the API should be a 40-character hex string. If the API proxy or a MITM returns a crafted value (e.g., `../../actions/runners`), it becomes an unintended path in the GitHub API URL. While exploitability requires a compromised API endpoint, it is good practice to validate that `head_sha` matches `^[0-9a-f]{40}$` before use. + +--- + +### F3 — Unquoted `$gh_hostname_flag` (Low) +**Files:** `plugins/github/hooks/scripts/lib/pr-discover.sh` line ~72; `plugins/github/hooks/scripts/lib/pr-state.sh` lines ~77, 83, 88, 94, 119 + +`gh_hostname_flag` is used unquoted in all `gh api` invocations: + +``` +gh api ${gh_hostname_flag} "repos/..." +``` + +The value is either empty or the literal string `--hostname github.com`, so word-splitting yields the correct two-word expansion and is not currently exploitable. However, if this variable's source ever changes, this pattern becomes a command-injection vector. The correct approach is to use a bash array: `local gh_flags=(); [[ ... ]] && gh_flags=(--hostname github.com); gh api "${gh_flags[@]}" ...`. + +--- + +### F4 — `check_interval` Arithmetic Expansion Without Integer Validation (Low) +**File:** `plugins/github/hooks/scripts/pr-state-check.sh`, lines ~86–93 + +```bash +check_interval="$(plugin_get_config "prStateCheckInterval" "60")" +... +elapsed=$((now - last_check)) +if [ "$elapsed" -lt "$check_interval" ]; then +``` + +`check_interval` comes from user YAML config without integer validation. The `[ "$elapsed" -lt "$check_interval" ]` test uses `-lt` which requires an integer; a non-integer value (e.g., `"abc"`) will cause a fatal error under `set -e`, exiting the hook early and disabling throttling for the session. More critically, while bash arithmetic `$((…))` does not execute shell commands, a value like `a[$(malicious_cmd)]` would be evaluated as an array subscript in some bash versions (indirect execution via `(( ))` arithmetic). The value should be validated with `[[ "$check_interval" =~ ^[0-9]+$ ]]` before use. + +--- + +### F5 — Cache Files Created Without Explicit Permissions (Low-Medium) +**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, line ~43; `pr-state-check.sh` line ~62 + +```bash +mkdir -p "$_PR_STATE_CACHE_DIR" +... +echo "$new_state" > "$cache_file" +``` + +Neither the directory nor files are created with an explicit restrictive mode. The default umask determines permissions. On a shared system where `~/.claude/` is world-readable (or group-readable), PR metadata — including PR bodies, reviewer names, comment text, and CI results — would be readable by other users. Recommendation: `mkdir -p -m 700 "$_PR_STATE_CACHE_DIR"` or `umask 077` at the top of the script. + +--- + +### F6 — TOCTOU Race in Throttle Logic (Low) +**File:** `plugins/github/hooks/scripts/pr-state-check.sh`, lines ~86–95 + +The throttle pattern reads `.last-check`, evaluates elapsed time, then writes the updated timestamp — a classic check-then-act race: + +```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" +``` + +If multiple hook invocations run concurrently (e.g., a user triggers many tool uses in rapid succession in a multi-process setup), all can pass the throttle gate simultaneously, each making full API requests and potentially reporting duplicate change notifications. Mitigation: use `flock` for atomic read-update (`flock "$last_check_file" bash -c '...'`). + +The same race applies in `pr_state_fetch_and_compare`: old state is read, API is called, new state is written — concurrent invocations can each read the same baseline and emit the same changes twice. + +--- + +### F7 — Raw PR Content Interpolated into Change Strings (Low) +**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines ~178, 184 and throughout `_pr_state_diff` + +PR titles, labels, and the first 100 characters of comment/review bodies from the GitHub API are interpolated directly into the `PR_STATE_CHANGES` array strings and echoed to stdout: + +```bash +PR_STATE_CHANGES+=("PR title changed: '${old_title}' -> '${new_title}' on ...") +... +echo " - ${change}" +``` + +A PR title containing terminal escape sequences (e.g., `\033[2J` to clear the terminal, or hyperlink sequences) would be emitted verbatim to the terminal. This does not allow command execution but can corrupt terminal state. Recommendation: strip or escape non-printable characters from API-derived values before including them in output. `jq`'s `@sh` or `@base64` filters could be used for sanitization, or the output could be passed through `cat -v`. + +--- + +### F8 — `set -euo pipefail` Absent from Sourced Library Files (Informational) +**Files:** `plugins/github/hooks/scripts/lib/pr-state.sh` line 1; `plugins/github/hooks/scripts/lib/pr-discover.sh` line 1 + +The library files are sourced (not executed), so they inherit the entry point's `set -euo pipefail`. This is correct behavior. However, the absence of the directive in the library headers means if a future caller sources these libraries without strict mode, silent failure propagation becomes possible (e.g., a failed `git` command silently returning empty string). A comment noting this inherited dependency would help prevent future misuse. + +--- + +### F9 — `project_slug` Safe; No Path Traversal (Informational — Positive Finding) +**File:** `plugins/github/hooks/scripts/pr-state-check.sh`, line ~60 + +```bash +project_slug="$(basename "$CLAUDE_PROJECT_DIR")" +``` + +`basename` correctly strips path components, so a `CLAUDE_PROJECT_DIR` of `/workspace/../../etc` would yield `etc` as the slug rather than traversing outside the cache root. This is correctly handled. + +--- + +### F10 — No Validation That Discovered Sibling Directories Are Trustworthy (Low) +**File:** `plugins/github/hooks/scripts/lib/pr-discover.sh`, lines ~30–43 + +The multi-project discovery scans `$(dirname "$CLAUDE_PROJECT_DIR")/*/` for sibling `.git` repos. In a shared or adversarially constructed filesystem, a symlinked directory or an attacker-planted git repo in the parent directory could be discovered and have its remote URL parsed, potentially causing `gh api` calls for attacker-controlled repositories. This is a low-severity concern in the intended single-user workstation context but worth noting for web-session deployments on shared infrastructure. + +## References + +- `plugins/github/hooks/scripts/pr-state-check.sh` (entry point, throttle logic, cache resolution) +- `plugins/github/hooks/scripts/lib/pr-state.sh` (API fetching, cache write, diff logic) +- `plugins/github/hooks/scripts/lib/pr-discover.sh` (URL parsing, sibling discovery) +- `plugins/github/hooks/hooks.json` (hook registration) +- [CWE-22: Path Traversal](https://cwe.mitre.org/data/definitions/22.html) +- [CWE-190: Integer Overflow / Improper Input Validation in Arithmetic](https://cwe.mitre.org/data/definitions/190.html) +- [CWE-362: TOCTOU Race Condition](https://cwe.mitre.org/data/definitions/362.html) +- [CWE-116: Improper Encoding / Escaping of Output](https://cwe.mitre.org/data/definitions/116.html) +- [bash arithmetic injection via array subscript](https://www.google.com/search?q=bash+arithmetic+injection+array+subscript+security) +- [flock(1) man page — for atomic file locking in shell](https://man7.org/linux/man-pages/man1/flock.1.html) diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/simplicity/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/simplicity/REPORT.md new file mode 100644 index 000000000..eb259cfe9 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/simplicity/REPORT.md @@ -0,0 +1,114 @@ +# Simplicity Review — PR #306 +Score: 58/100 + +## Summary + +The PR introduces a real, useful feature — async PR state tracking — and the overall intent is clean: fetch, cache, diff, report. However, the implementation carries a meaningful complexity tax at several layers. The `_pr_state_diff` function spawns a separate `echo "$json" | jq` subprocess for every individual field it inspects (roughly 20 separate jq invocations against the same two JSON blobs), when a single jq call could produce all diff output at once. The 3-file library split is slightly over-engineered for the volume of code involved: `pr-discover.sh` (114 lines) and `pr-state.sh` (330 lines) are reasonable in isolation, but their combined caller `pr-state-check.sh` is thin enough (145 lines, much of which is scaffolding) that collapsing all three into a single well-commented script would reduce sourcing overhead and make the execution path easier to follow. The `PostToolUse "*"` matcher with an in-script throttle works correctly but introduces a subtle ordering constraint: the throttle state file is written *after* the interval check passes, meaning the script must always spin up just to re-read the cache and exit — a more targeted matcher or a dedicated debounce hook type would eliminate that per-tool-use bash invocation entirely. These are real costs but none are blockers; the code is correct and well-commented throughout. + +## Findings + +### 1. `_pr_state_diff`: ~20 redundant jq subprocesses (high impact) + +**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines 152–285 + +The function compares old and new state by calling `echo "$old" | jq -r '...'` and `echo "$new" | jq -r '...'` once per field, in sequence. Counting the individual subshell invocations: + +- body: 2 jq calls +- title: 2 +- draft: 2 +- state + merged: 4 +- mergeable + mergeable_state: 4 +- labels: 2 +- review count: 2 (plus a conditional third for new review text) +- comment count: 2 (plus conditional) +- review comment count: 2 (plus conditional) +- CI checks comparison: 2 (plus `_pr_state_diff_checks` which spawns N additional calls per check name) + +That is roughly 22–30 forks just for the diff of a single PR. Each call re-parses the same JSON string. A single jq invocation can compare all scalar fields and produce structured output: + +```bash +jq -r -n \ + --argjson old "$old" \ + --argjson new "$new" \ + '{ + body_changed: ($old.pr.body != $new.pr.body), + title_changed: ($old.pr.title != $new.pr.title), + old_title: $old.pr.title, + new_title: $new.pr.title, + draft_changed: ($old.pr.draft != $new.pr.draft), + new_draft: $new.pr.draft, + state_changed: ($old.pr.state != $new.pr.state), + ... + } | to_entries[] | select(.value == true or (.key | endswith("_changed") | not)) | ...' +``` + +This would cut subprocess count from ~25 to 1–2, with real performance impact since the function runs on every throttle-passing PostToolUse event across multiple PRs. + +### 2. `_pr_state_diff_checks`: N jq subprocesses per check name (high impact) + +**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines 288–323 + +The function first calls jq once to get all check names, then for each check name spawns 4 more jq calls (old_status, old_conclusion, new_status, new_conclusion). For a repo with 10 CI checks this is 41 jq invocations for a single diff. This entire function could be replaced with a single jq call that joins old and new check arrays by name and emits only the changed rows: + +```bash +jq -r -n \ + --argjson old "$(echo "$old" | jq '.checks.checks // []')" \ + --argjson new "$(echo "$new" | jq '.checks.checks // []')" \ + '($old | map({(.name): {status,conclusion}}) | add // {}) as $om | + ($new | map({(.name): {status,conclusion}}) | add // {}) as $nm | + ([$old[].name, $new[].name] | unique[]) as $name | + select($om[$name] != $nm[$name]) | + "\($name): \($om[$name].status // "missing")/\($om[$name].conclusion // "pending") -> \($nm[$name].status // "missing")/\($nm[$name].conclusion // "pending")"' +``` + +This reduces the check diff from O(N×4)+1 calls to 3 (two to extract subfields before passing `--argjson`, or 1 if old/new are passed as top-level objects). + +### 3. 3-file library split is slightly over-engineered for the code volume + +**Files:** `pr-state-check.sh` (145 lines), `pr-state.sh` (330 lines), `pr-discover.sh` (114 lines) + +The split is principled — orchestration vs. state-fetch/diff vs. discovery — and would be appropriate if each library were reused independently. However, `pr-discover.sh` is sourced only by `pr-state-check.sh`, and `pr-state.sh` is sourced only by `pr-state-check.sh`. With no other consumers, the indirection adds `source` overhead and reader navigation cost without reuse benefit. Collapsing all three into a single `pr-state-check.sh` with internal `_` prefixed functions (following the pattern already used) would make the execution path linear and self-contained, reducing cognitive overhead from ~590 lines across 3 files to one cohesive script. + +The split is not wrong and is forward-compatible if reuse is anticipated, but there is no current evidence of that intent. + +### 4. `PostToolUse "*"` matcher launches a bash process on every tool call + +**File:** `plugins/github/hooks/hooks.json`, line 14 + +The matcher `"*"` means `pr-state-check.sh` is invoked after every single tool use. The script then reads the throttle file to decide whether to exit early. This is correct behavior, but it means the full bash startup + `source` of 4 library files + config reads occurs on every tool use — only to exit after the interval check in most cases. A more efficient approach would be to place the throttle check *before* the library sources, or to use a dedicated hook event that fires less frequently if the platform supports it. Currently the early-exit guard at line 98 in `pr-state-check.sh` comes after config reads, `source` calls, and guard checks — which is already reasonably fast but not minimal. + +A minor structural improvement: move the `hook_input="$(cat)"` stdin drain above the throttle check, which is already required before any early exit to avoid breaking the pipe, but the current ordering already does this correctly. + +### 5. Global mutation via `_PR_OWNER` / `_PR_REPO` in `pr-discover.sh` + +**File:** `plugins/github/hooks/scripts/lib/pr-discover.sh`, lines 72–74 + +`_pr_extract_owner_repo` communicates its results by setting module-level global variables `_PR_OWNER` and `_PR_REPO`. In a sourced library this is a surprising side effect. The function could instead echo `owner repo` to stdout and be called as `read -r owner repo < <(_pr_extract_owner_repo "$url")`, which is idiomatic shell and eliminates the global state. This is a minor style issue but adds to complexity when reasoning about the library. + +### 6. `cat > /dev/null` pattern for stdin drain on early exit + +**File:** `plugins/github/hooks/scripts/pr-state-check.sh`, lines 48, 52, 56 + +Each early-exit guard uses `cat > /dev/null` before `exit 0` to drain stdin. This pattern is correct for Claude Code hooks (which pipe stdin), but: +- The `hook_input="$(cat)"` drain at line 82 already handles this once stdin is needed. +- The early-exit guards before any stdin read are correct to drain, but `cat > /dev/null` is an unusual idiom. The simpler `exec > /dev/null 2>&1` or `read -r -d '' _` would be more standard. This is a minor clarity issue, not a correctness problem. + +### 7. `pr_state_changes_summary` is defined but never called + +**File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines 325–335 + +The function `pr_state_changes_summary` provides a formatted summary of `PR_STATE_CHANGES`, but `pr-state-check.sh` iterates the array directly to build its own output. The summary function is dead code in the current implementation. Either the function should be used by the caller, or it should be removed. Its presence adds a small navigation burden without benefit. + +### 8. `[ "$branch" = "main" ] || [ "$branch" = "master" ]` filtering in discover + +**File:** `plugins/github/hooks/scripts/lib/pr-discover.sh`, line 63 + +Filtering out `main` and `master` as a proxy for "no PR open" is a reasonable heuristic but not correct in all cases: some workflows use `main` or `master` as feature branches, and some repos use different default branch names (e.g., `trunk`, `develop`). The correct check — which the code already performs two lines later — is to query the API for an open PR and get an empty result. The branch-name filter is an unnecessary pre-check that adds fragility. Removing it simplifies the function to just "try to find a PR; if none, return 0." + +## References + +- `plugins/github/hooks/scripts/lib/pr-state.sh` — `_pr_state_diff` (lines 152–285), `_pr_state_diff_checks` (lines 288–323) +- `plugins/github/hooks/scripts/lib/pr-discover.sh` — `_pr_extract_owner_repo` (lines 72–114), branch filter (line 63) +- `plugins/github/hooks/scripts/pr-state-check.sh` — PostToolUse throttle (lines 98–109), early-exit guards (lines 46–56) +- `plugins/github/hooks/hooks.json` — PostToolUse `"*"` matcher (line 14) +- PR #306: https://github.com/nsheaps/ai-mktpl/pull/306 diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/usability/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/usability/REPORT.md new file mode 100644 index 000000000..328c67bb8 --- /dev/null +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/usability/REPORT.md @@ -0,0 +1,82 @@ +# Usability Review — PR #306 +Score: 81/100 + +## Summary + +This PR adds async PR state tracking hooks to the github plugin. The overall usability is strong: the feature is zero-config (enabled by default with sensible defaults), the change messages are specific and actionable, and the documentation is thorough across three locations (README, SKILL.md, and the settings YAML). The main usability gaps are a non-obvious dependency on `jq` that is silently swallowed, a PR discovery approach whose single-branch-per-repo constraint is undocumented from the user's perspective, a review comparison that only detects new reviews (not withdrawn or re-submitted ones), and the 60-second PostToolUse throttle creating a window where the agent could act on stale information. + +## Findings + +### Zero-config / works out of the box + +`prStateTracking` defaults to `true` and the cache directory auto-resolves to `~/.claude/plugin-cache/github`. On first invocation the cache directory is created automatically. The feature requires `gh` and `jq` on PATH; both guards exit cleanly and silently if they are missing. + +- `plugins/github/hooks/scripts/pr-state-check.sh` lines 38-50: both guards exit 0, which means the feature simply does nothing when requirements are missing. This is good for silent degradation, but there is no visible indication to the user that tracking is inactive because `jq` is absent. A one-time warning to the agent (e.g., written to the hook's stdout on first invocation only) would let users know what to install. + +### Change notification quality + +The messages are notably specific and actionable. Examples from `plugins/github/hooks/scripts/lib/pr-state.sh`: + +- Line ~195: `"New review on ${owner}/${repo}#${pr_number}: ${review_line}"` — includes reviewer username and state (e.g., `nsheaps APPROVED`), immediately actionable. +- Line ~215: `"New comment on ${owner}/${repo}#${pr_number}: ${comment_line}"` — includes the first 100 characters of the comment body, enough context for the agent to decide whether to read more. +- Line ~227: `"New review comment on ${owner}/${repo}#${pr_number}: ${rc_line}"` — includes the file path and first 100 characters, aligning with the documented example `"nsheaps on src/main.ts: Consider using..."`. +- Lines ~244-252 (`_pr_state_diff_checks`): CI messages include the check name and the exact old/new status/conclusion transition, e.g., `"lint: in_progress/pending -> completed/success"`. This is better than a generic "CI changed" message. + +**Gap — body change message is vague**: `"PR body updated on ${owner}/${repo}#${pr_number}"` (line ~162) reports that the body changed but provides no diff or summary. The agent knows something changed but must make another API call to learn what. All other change types surface at least the new value inline. + +**Gap — review comparison is count-based only**: Lines 185-197 detect new reviews by comparing array length. If a reviewer dismisses and re-submits a review (same count, different state), or if a review is dismissed, the change is invisible. This is a latent correctness issue that may confuse agents when they see a stale APPROVED state after a CHANGES_REQUESTED dismissal. + +**Gap — label message surfaces raw JSON**: Line ~179: `"Labels changed on ${owner}/${repo}#${pr_number}: ${old_labels} -> ${new_labels}"` where `${old_labels}` and `${new_labels}` are compact JSON arrays (e.g., `["bug","enhancement"]`). This is readable but inconsistent with the prose style of the other messages. Showing it as `bug, enhancement -> bug` would be cleaner. + +### Configuration documentation + +The settings file (`plugins/github/github.settings.yaml`) is the primary reference and is well-written. Each option has a multi-line comment explaining its purpose, units, defaults, and interaction with other options. The README has a full example YAML block and a separate subsection explaining the cache directory. The SKILL.md repeats the configuration in a condensed, skill-appropriate format. + +- The `prStateCacheDir` option is commented out in the defaults file, which correctly signals "this is an override, not a required value". +- The 3-tier config resolution (plugin default → user level → project level) is mentioned in the README but not explicitly described as 3-tier; users who haven't read the broader plugin system docs might not know the precedence order. + +### Graceful degradation + +- Missing `gh`: exits 0 silently. Good. +- Missing `jq`: exits 0 silently. Good for non-interruption, but silent. Same concern as above. +- `CLAUDE_PROJECT_DIR` unset: falls back to `project_slug="default"`. Acceptable. +- No active PRs found: logs `"No active PRs found for tracked projects"` to the log (not to the agent's context window) and exits 0. Clean. +- `pr_state_fetch_and_compare` failure: returns 0 (treats as no-change). The API call returns `{}` on failure (line ~88 of pr-state.sh), which will then silently skip diffing. This is safe but means a transient API error could cause a missed change notification. + +### PR discovery constraints (user comprehension) + +`plugins/github/hooks/scripts/lib/pr-discover.sh` line 58 skips branches named `main` or `master` unconditionally. This is a reasonable heuristic to avoid tracking the base branch, but if a user's default branch is named `trunk` or `develop` it would be tracked (and likely find no PR). The README mentions multi-project discovery and the sibling-directory scan but does not mention the main/master exclusion or the single-open-PR-per-branch assumption. Users working on stacked PRs or repos with multiple open PRs from the same branch would only see one tracked. + +The sibling directory scan has a usability implication that is undocumented: the plugin will also track PRs in any adjacent git repo under the parent directory, even repos unrelated to the current session. In a developer's home directory where multiple projects sit as siblings, this could produce surprising noise. The README acknowledges multi-project sessions positively but does not warn about this edge case. + +### PostToolUse throttle and staleness + +The 60-second default interval (`prStateCheckInterval`) means the agent could make decisions for up to a minute without seeing changes that arrived during that window. The output message `"Review these changes and determine if any action is needed."` (pr-state-check.sh, line ~141) is a useful prompt, but does not tell the agent how stale the information might be (i.e., when the last check actually ran). Adding a `"Last checked: "` line to the output would help the agent calibrate urgency. + +### SessionStart output + +On SessionStart with no changes, the script outputs `"github: PR state baseline established for ${pr_count} PR(s)"` to stdout. This is clear and confirms the feature is working. On subsequent PostToolUse runs with no changes, the script exits silently — good, no noise when nothing happens. + +### Output format + +The bulleted list format for changes is clean: +``` +github: PR state changes detected since last check: + + - New review on owner/repo#123: nsheaps APPROVED + - CI on owner/repo#123: lint: completed/pending -> completed/success + +Review these changes and determine if any action is needed. +``` +This is readable, correctly prefixed with the plugin name, and ends with an explicit action prompt. The trailing blank lines and the closing call-to-action improve agent comprehension over a raw list. + +## References + +- `plugins/github/hooks/scripts/pr-state-check.sh` — main hook entry point, guards, throttle logic, output formatting +- `plugins/github/hooks/scripts/lib/pr-state.sh` — fetch, cache, diff logic; change message construction +- `plugins/github/hooks/scripts/lib/pr-discover.sh` — multi-project PR discovery, branch filtering +- `plugins/github/github.settings.yaml` — config schema and inline documentation +- `plugins/github/README.md` — change detection table, cache structure, configuration examples +- `plugins/github/skills/pr-state-tracking/SKILL.md` — skill-level documentation, requirements, future roadmap +- `plugins/github/hooks/hooks.json` — hook registrations (SessionStart, PostToolUse, Stop) +- PR: https://github.com/nsheaps/ai-mktpl/pull/306 From ce6392b4771ec8a272947327a8b4beb9eeaf1c72 Mon Sep 17 00:00:00 2001 From: nsheaps <1282393+nsheaps@users.noreply.github.com> Date: Wed, 25 Mar 2026 00:59:16 +0000 Subject: [PATCH 09/11] chore: auto-bump plugin versions and update marketplace --- .../ai-mktpl/306/1774399314/OVERALL-REPORT.md | 31 ++++++++++++------- .../306/1774399314/best-practices/REPORT.md | 28 +++++++++++++++++ .../306/1774399314/documentation/REPORT.md | 1 + .../306/1774399314/flexibility/REPORT.md | 1 + .../306/1774399314/qa-engineering/REPORT.md | 28 +++++++++++++++++ .../306/1774399314/repo-patterns/REPORT.md | 4 +++ .../306/1774399314/security/REPORT.md | 11 +++++++ .../306/1774399314/simplicity/REPORT.md | 6 ++-- .../306/1774399314/usability/REPORT.md | 3 ++ 9 files changed, 100 insertions(+), 13 deletions(-) diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/OVERALL-REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/OVERALL-REPORT.md index de80554db..acf17cedf 100644 --- a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/OVERALL-REPORT.md +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/OVERALL-REPORT.md @@ -5,6 +5,7 @@ ## 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 @@ -19,42 +20,47 @@ Initial review identified 4 hard-block categories (<70%). An iteration commit (` ## 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** | ⚠️ | +| 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 @@ -62,15 +68,18 @@ Initial review identified 4 hard-block categories (<70%). An iteration commit (` - 🔕 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) diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/best-practices/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/best-practices/REPORT.md index caa91b956..bf9c80d1c 100644 --- a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/best-practices/REPORT.md +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/best-practices/REPORT.md @@ -1,4 +1,5 @@ # Best Practices Review — PR #306 + Score: 62/100 ## Summary @@ -10,6 +11,7 @@ This PR introduces a well-structured async hook system for tracking PR state cha ## Findings ### F1 — CRITICAL: Operator-precedence logic bug in branch skip guard + **File:** `plugins/github/hooks/scripts/lib/pr-discover.sh`, line ~65 ```bash @@ -17,18 +19,23 @@ This PR introduces a well-structured async hook system for tracking PR state cha ``` 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 @@ -38,12 +45,15 @@ 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" ``` @@ -51,9 +61,11 @@ 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 // ""')" @@ -62,27 +74,33 @@ 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 @@ -101,6 +119,7 @@ There is a check-then-act gap between reading the file and writing the new times --- ### 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 @@ -110,6 +129,7 @@ pr_json="$(gh api ${gh_hostname_flag} \ ``` `${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 @@ -117,11 +137,13 @@ if [ "${CLAUDE_CODE_REMOTE:-}" = "true" ]; then 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. @@ -131,6 +153,7 @@ At minimum, the fetch should log a warning rather than silently discarding rate- --- ### F8 — MEDIUM: `cat > /dev/null` idiom for stdin drain is misleading + **File:** `plugins/github/hooks/scripts/pr-state-check.sh`, lines ~36–44 ```bash @@ -145,6 +168,7 @@ The early-exit guards drain stdin before exiting using `cat > /dev/null`. This i --- ### 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 @@ -157,6 +181,7 @@ _PR_REPO="" --- ### F10 — LOW: `jq` labels filter has incorrect array comprehension + **File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, line ~192 ```bash @@ -164,6 +189,7 @@ 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 @@ -173,6 +199,7 @@ 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. @@ -180,6 +207,7 @@ jq -c '(.pr.labels // []) | sort' --- ### F12 — LOW: `PLUGIN_NAME` variable is declared but never used + **File:** `plugins/github/hooks/scripts/pr-state-check.sh`, line ~18 ```bash diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/documentation/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/documentation/REPORT.md index d8516b147..7be9fd4b3 100644 --- a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/documentation/REPORT.md +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/documentation/REPORT.md @@ -1,4 +1,5 @@ # Documentation Review — PR #306 + Score: 87/100 ## Summary diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/flexibility/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/flexibility/REPORT.md index 92fbd08da..3d487eee6 100644 --- a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/flexibility/REPORT.md +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/flexibility/REPORT.md @@ -1,4 +1,5 @@ # Flexibility Review — PR #306 + Score: 78/100 ## Summary diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/qa-engineering/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/qa-engineering/REPORT.md index 8fac49cd4..60fdf8501 100644 --- a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/qa-engineering/REPORT.md +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/qa-engineering/REPORT.md @@ -1,4 +1,5 @@ # QA & Engineering Review — PR #306 + Score: 47/100 ## Summary @@ -8,16 +9,21 @@ This PR adds async hook infrastructure for PR state tracking in the github plugi ## Findings ### 1. PR Body Inaccuracy — Stale File Reference + The Changes section of the PR body still lists: + > `lib/hook-output.sh` — Symlink to shared hook-output library This file was removed in commit `862fbafeb` per the "Remove unused hook-output.sh symlink (YAGNI)" fix. The PR body was never updated to reflect this removal. The file does not appear in any changed-files listing, confirming it is absent from the final branch state. The test plan and summary sections are otherwise accurate. ### 2. Dirty Merge State — Rebase Required + `mergeable_state: "dirty"` at head SHA `a5e2afe8dd87220f2c3685bcd3edb5ac5f09c0b3`. The branch needs a rebase onto `main` before merge. Since the PR is still draft, this is expected but must be resolved before promotion. ### 3. No Automated Tests + Zero test files exist for any of the three new shell scripts: + - `plugins/github/hooks/scripts/pr-state-check.sh` - `plugins/github/hooks/scripts/lib/pr-state.sh` - `plugins/github/hooks/scripts/lib/pr-discover.sh` @@ -25,6 +31,7 @@ Zero test files exist for any of the three new shell scripts: The test plan is 7 manual checkbox items, none marked as completed. There is no bats, shellspec, or similar framework invocation. CI passes only `lint` and `validate` checks — neither exercises the new functionality. Given that the core logic (JSON diffing, cache file management, throttle state, multi-repo discovery) is all testable in isolation, the absence of tests is a significant gap for production reliability. ### 4. Partial-Failure State Corruption in `_pr_state_fetch` + `plugins/github/hooks/scripts/lib/pr-state.sh` (lines ~85–155) makes 5 sequential `gh api` calls. The failure handling is asymmetric: - Call 1 (PR core data): `|| { echo "{}" ; return 1; }` — returns 1, and the caller (`pr_state_fetch_and_compare`) traps this with `|| return 0`, so no cache write occurs. This is correct. @@ -34,43 +41,58 @@ The test plan is 7 manual checkbox items, none marked as completed. There is no The problem: if call 1 succeeds but call 2 (reviews) fails transiently, the function continues with `review_json="[]"`, combines all state into JSON, and **writes the incomplete snapshot to the cache file**. On the next invocation, the old (complete) cache is replaced by the new (incomplete) one. The diff logic then reports "all reviews disappeared" — generating false-positive change notifications — or worse, silently misses real changes because the baseline is now wrong. A correct approach would either: + - Track whether any secondary call failed and skip the cache write, or - Record a `fetch_error` flag in the JSON to suppress diffs for that invocation. ### 5. Uncaught Failure Modes: `jq` Combining Step + After all 5 API calls, the function runs a `jq -n` to combine results (pr-state.sh, ~lines 157–170). This step has no `||` guard. If `jq` fails (e.g., malformed JSON from one of the `|| fallback` paths), the function exits non-zero, but `pr_state_fetch_and_compare` has already passed the point where it guards on fetch failure. Depending on bash `set -e` propagation through sourced libs, this may silently exit the entire hook rather than returning a controlled error. ### 6. Comment Count Diffing — Assumes Append-Only + `plugins/github/hooks/scripts/lib/pr-state.sh` (lines ~210–240) detects new reviews, comments, and review comments by comparing array lengths and slicing from `[$old_count:]`. This assumes comments are only ever added, never deleted. If a comment is deleted externally (GitHub allows this), the new count may be lower, the slice index becomes negative or out-of-range, and `jq` emits nothing — the deletion is silently ignored with no notification. The current implementation explicitly does not detect deletions. ### 7. Review Deduplication Not Handled + The reviews diff (pr-state.sh ~lines 195–210) compares total review count. GitHub's reviews endpoint returns a full history including superseded reviews (e.g., a reviewer who approved, then requested changes, then approved again will appear 3 times). Count-based diffing will report a new review event even if the reviewer merely re-submitted the same state, and will miss review state _changes_ within the same reviewer's review record if no new review object is appended. ### 8. Shell Logic Bug: Operator Precedence in Branch Skip + `plugins/github/hooks/scripts/lib/pr-discover.sh` (line ~63): + ```bash [ "$branch" = "main" ] || [ "$branch" = "master" ] && return 0 ``` + Due to bash's left-to-right evaluation with equal precedence for `||` and `&&`, this parses as: + ```bash ([ "$branch" = "main" ]) || ([ "$branch" = "master" ] && return 0) ``` + If `branch` is `main`, the first test is true, the `||` short-circuits, and `return 0` is NOT executed — the function continues processing the main branch as if it had a PR. The fix commit (`862fbafeb`) does not appear to have addressed this. The correct form is: + ```bash { [ "$branch" = "main" ] || [ "$branch" = "master" ]; } && return 0 ``` ### 9. Unquoted Variable in `gh api` Call + `plugins/github/hooks/scripts/lib/pr-discover.sh` (line ~75) and `pr-state.sh` (lines ~90, ~100, etc.): + ```bash gh api ${gh_hostname_flag} \ ``` + `${gh_hostname_flag}` is unquoted. When empty, this is fine. When set to `--hostname github.com`, it expands to two words correctly. However, with `set -u` active (via `set -euo pipefail` in `pr-state-check.sh`), if `gh_hostname_flag` were ever unset (not just empty) this would error. The variable is always initialized to `""` so this is low risk but is a style violation under `set -u` conventions. ### 10. PostToolUse Timeout May Be Too Short + `plugins/github/hooks/hooks.json` (line 27): PostToolUse timeout is 15 seconds. With 5 sequential API calls per PR (each with network latency), a multi-repo session tracking 3 PRs could require 15+ API calls. On a slow network or GitHub API slowdown, the hook will time out mid-fetch, leaving partial state. The throttle ensures this doesn't happen every tool call, but the first execution after the 60s window may still exceed 15s for multi-PR sessions. ### 11. CI Status: All Checks Pass + - `lint`: completed/success - `validate`: completed/success - `auto-version-bump`: completed/success @@ -80,7 +102,9 @@ gh api ${gh_hostname_flag} \ No CI failures. The lint check validates the new shell scripts pass shellcheck (implicitly, given the lint job passes). ### 12. Commit Quality + The 5 commits on the branch are: + 1. `e1c5f3ab` — `feat(github): add async hooks for PR state tracking` — well-scoped initial implementation 2. `35642ca6` — `chore: mise run lint` — atomic, appropriate 3. `8f7ca2f9` — `chore: auto-bump plugin versions and update marketplace` — automation bot, appropriate @@ -90,13 +114,17 @@ The 5 commits on the branch are: Commit 4 is the most notable: it includes the mise absolute-path fix and the auto-pr-to-making-great-prs rename, both of which are logically separate features. This inflates the PR's diff and makes the history harder to bisect. ### 13. Open Review Threads Not All Resolved + Of the 7 original review threads from `henry-nsheaps`: + - Threads marked `is_outdated: true` (5 of 7): addressed in the fix commit and superseded by code changes - `hooks.json` PostToolUse matcher thread: `is_outdated: false`, **not resolved** — the throttle was added, but the `*` matcher remains; the thread author's suggestion to use a targeted matcher was not implemented - `README.md` tilde expansion thread: `is_outdated: false`, **not resolved** — tilde expansion was added to the script (`pr-state-check.sh:52`) but the thread was not marked resolved ### 14. Test Plan Gaps + The test plan does not cover: + - Behavior when `gh` API returns an error mid-sequence (partial fetch) - Comment/review deletion handling - The `main`/`master` branch skip logic diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/repo-patterns/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/repo-patterns/REPORT.md index e1cd51935..93a7cc31a 100644 --- a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/repo-patterns/REPORT.md +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/repo-patterns/REPORT.md @@ -1,4 +1,5 @@ # Repo Patterns Review — PR #306 + Score: 84/100 ## Summary @@ -14,6 +15,7 @@ The updated `plugins/github/hooks/hooks.json` follows the exact schema used by a ### Script structure — PARTIAL DEVIATION Existing hook scripts (`install-gh.sh`, `install-mise.sh`) follow a strict pattern: + 1. `PLUGIN_NAME=""` 2. `source "${CLAUDE_PLUGIN_ROOT}/lib/plugin-config-read.sh"` 3. `source "${CLAUDE_PLUGIN_ROOT}/lib/tool-install.sh"` @@ -34,12 +36,14 @@ The new library files `hooks/scripts/lib/pr-state.sh` and `hooks/scripts/lib/pr- ### Double-source guard — MATCHES Both new library files use the repo's established guard idiom: + ```bash if [ "${_PR_STATE_LOADED:-}" = "true" ]; then return 0 2>/dev/null || true fi _PR_STATE_LOADED="true" ``` + This exactly matches the pattern in `plugins/github/lib/log.sh` (`_LOG_SH_LOADED`) and `plugins/github/lib/hook-logging.sh` (`_HOOK_LOGGING_LOADED`). The guard variable naming convention (`__LOADED`) is consistent. ### Settings YAML — MATCHES diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/security/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/security/REPORT.md index 4cb071786..8866a2756 100644 --- a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/security/REPORT.md +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/security/REPORT.md @@ -1,4 +1,5 @@ # Security Review — PR #306 + Score: 62/100 ## Summary @@ -8,6 +9,7 @@ PR #306 introduces async shell-based hooks for PR state tracking. The scripts ar ## Findings ### F1 — Path Traversal via Parsed Remote URL (Medium) + **File:** `plugins/github/hooks/scripts/lib/pr-discover.sh`, lines 88–100 `_pr_extract_owner_repo` extracts `_PR_OWNER` and `_PR_REPO` from the git remote URL using `sed` with no sanitization or character-class restriction. The captured groups allow `/`, `..`, `~`, and other filesystem-significant characters. Both values flow directly into the cache filename in `pr-state.sh`: @@ -28,6 +30,7 @@ Mitigation: validate that `_PR_OWNER` and `_PR_REPO` match `^[A-Za-z0-9_.-]+$` b --- ### F2 — `head_sha` from API Response Used in URL Without Validation (Low-Medium) + **File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines 118–124 `head_sha` is extracted from the GitHub API JSON response via `jq -r '.head_sha // empty'` and immediately used in a subsequent API call: @@ -43,6 +46,7 @@ A SHA returned by the API should be a 40-character hex string. If the API proxy --- ### F3 — Unquoted `$gh_hostname_flag` (Low) + **Files:** `plugins/github/hooks/scripts/lib/pr-discover.sh` line ~72; `plugins/github/hooks/scripts/lib/pr-state.sh` lines ~77, 83, 88, 94, 119 `gh_hostname_flag` is used unquoted in all `gh api` invocations: @@ -56,6 +60,7 @@ The value is either empty or the literal string `--hostname github.com`, so word --- ### F4 — `check_interval` Arithmetic Expansion Without Integer Validation (Low) + **File:** `plugins/github/hooks/scripts/pr-state-check.sh`, lines ~86–93 ```bash @@ -70,6 +75,7 @@ if [ "$elapsed" -lt "$check_interval" ]; then --- ### F5 — Cache Files Created Without Explicit Permissions (Low-Medium) + **File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, line ~43; `pr-state-check.sh` line ~62 ```bash @@ -83,6 +89,7 @@ Neither the directory nor files are created with an explicit restrictive mode. T --- ### F6 — TOCTOU Race in Throttle Logic (Low) + **File:** `plugins/github/hooks/scripts/pr-state-check.sh`, lines ~86–95 The throttle pattern reads `.last-check`, evaluates elapsed time, then writes the updated timestamp — a classic check-then-act race: @@ -103,6 +110,7 @@ The same race applies in `pr_state_fetch_and_compare`: old state is read, API is --- ### F7 — Raw PR Content Interpolated into Change Strings (Low) + **File:** `plugins/github/hooks/scripts/lib/pr-state.sh`, lines ~178, 184 and throughout `_pr_state_diff` PR titles, labels, and the first 100 characters of comment/review bodies from the GitHub API are interpolated directly into the `PR_STATE_CHANGES` array strings and echoed to stdout: @@ -118,6 +126,7 @@ A PR title containing terminal escape sequences (e.g., `\033[2J` to clear the te --- ### F8 — `set -euo pipefail` Absent from Sourced Library Files (Informational) + **Files:** `plugins/github/hooks/scripts/lib/pr-state.sh` line 1; `plugins/github/hooks/scripts/lib/pr-discover.sh` line 1 The library files are sourced (not executed), so they inherit the entry point's `set -euo pipefail`. This is correct behavior. However, the absence of the directive in the library headers means if a future caller sources these libraries without strict mode, silent failure propagation becomes possible (e.g., a failed `git` command silently returning empty string). A comment noting this inherited dependency would help prevent future misuse. @@ -125,6 +134,7 @@ The library files are sourced (not executed), so they inherit the entry point's --- ### F9 — `project_slug` Safe; No Path Traversal (Informational — Positive Finding) + **File:** `plugins/github/hooks/scripts/pr-state-check.sh`, line ~60 ```bash @@ -136,6 +146,7 @@ project_slug="$(basename "$CLAUDE_PROJECT_DIR")" --- ### F10 — No Validation That Discovered Sibling Directories Are Trustworthy (Low) + **File:** `plugins/github/hooks/scripts/lib/pr-discover.sh`, lines ~30–43 The multi-project discovery scans `$(dirname "$CLAUDE_PROJECT_DIR")/*/` for sibling `.git` repos. In a shared or adversarially constructed filesystem, a symlinked directory or an attacker-planted git repo in the parent directory could be discovered and have its remote URL parsed, potentially causing `gh api` calls for attacker-controlled repositories. This is a low-severity concern in the intended single-user workstation context but worth noting for web-session deployments on shared infrastructure. diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/simplicity/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/simplicity/REPORT.md index eb259cfe9..2348b4c1d 100644 --- a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/simplicity/REPORT.md +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/simplicity/REPORT.md @@ -1,9 +1,10 @@ # Simplicity Review — PR #306 + Score: 58/100 ## Summary -The PR introduces a real, useful feature — async PR state tracking — and the overall intent is clean: fetch, cache, diff, report. However, the implementation carries a meaningful complexity tax at several layers. The `_pr_state_diff` function spawns a separate `echo "$json" | jq` subprocess for every individual field it inspects (roughly 20 separate jq invocations against the same two JSON blobs), when a single jq call could produce all diff output at once. The 3-file library split is slightly over-engineered for the volume of code involved: `pr-discover.sh` (114 lines) and `pr-state.sh` (330 lines) are reasonable in isolation, but their combined caller `pr-state-check.sh` is thin enough (145 lines, much of which is scaffolding) that collapsing all three into a single well-commented script would reduce sourcing overhead and make the execution path easier to follow. The `PostToolUse "*"` matcher with an in-script throttle works correctly but introduces a subtle ordering constraint: the throttle state file is written *after* the interval check passes, meaning the script must always spin up just to re-read the cache and exit — a more targeted matcher or a dedicated debounce hook type would eliminate that per-tool-use bash invocation entirely. These are real costs but none are blockers; the code is correct and well-commented throughout. +The PR introduces a real, useful feature — async PR state tracking — and the overall intent is clean: fetch, cache, diff, report. However, the implementation carries a meaningful complexity tax at several layers. The `_pr_state_diff` function spawns a separate `echo "$json" | jq` subprocess for every individual field it inspects (roughly 20 separate jq invocations against the same two JSON blobs), when a single jq call could produce all diff output at once. The 3-file library split is slightly over-engineered for the volume of code involved: `pr-discover.sh` (114 lines) and `pr-state.sh` (330 lines) are reasonable in isolation, but their combined caller `pr-state-check.sh` is thin enough (145 lines, much of which is scaffolding) that collapsing all three into a single well-commented script would reduce sourcing overhead and make the execution path easier to follow. The `PostToolUse "*"` matcher with an in-script throttle works correctly but introduces a subtle ordering constraint: the throttle state file is written _after_ the interval check passes, meaning the script must always spin up just to re-read the cache and exit — a more targeted matcher or a dedicated debounce hook type would eliminate that per-tool-use bash invocation entirely. These are real costs but none are blockers; the code is correct and well-commented throughout. ## Findings @@ -75,7 +76,7 @@ The split is not wrong and is forward-compatible if reuse is anticipated, but th **File:** `plugins/github/hooks/hooks.json`, line 14 -The matcher `"*"` means `pr-state-check.sh` is invoked after every single tool use. The script then reads the throttle file to decide whether to exit early. This is correct behavior, but it means the full bash startup + `source` of 4 library files + config reads occurs on every tool use — only to exit after the interval check in most cases. A more efficient approach would be to place the throttle check *before* the library sources, or to use a dedicated hook event that fires less frequently if the platform supports it. Currently the early-exit guard at line 98 in `pr-state-check.sh` comes after config reads, `source` calls, and guard checks — which is already reasonably fast but not minimal. +The matcher `"*"` means `pr-state-check.sh` is invoked after every single tool use. The script then reads the throttle file to decide whether to exit early. This is correct behavior, but it means the full bash startup + `source` of 4 library files + config reads occurs on every tool use — only to exit after the interval check in most cases. A more efficient approach would be to place the throttle check _before_ the library sources, or to use a dedicated hook event that fires less frequently if the platform supports it. Currently the early-exit guard at line 98 in `pr-state-check.sh` comes after config reads, `source` calls, and guard checks — which is already reasonably fast but not minimal. A minor structural improvement: move the `hook_input="$(cat)"` stdin drain above the throttle check, which is already required before any early exit to avoid breaking the pipe, but the current ordering already does this correctly. @@ -90,6 +91,7 @@ A minor structural improvement: move the `hook_input="$(cat)"` stdin drain above **File:** `plugins/github/hooks/scripts/pr-state-check.sh`, lines 48, 52, 56 Each early-exit guard uses `cat > /dev/null` before `exit 0` to drain stdin. This pattern is correct for Claude Code hooks (which pipe stdin), but: + - The `hook_input="$(cat)"` drain at line 82 already handles this once stdin is needed. - The early-exit guards before any stdin read are correct to drain, but `cat > /dev/null` is an unusual idiom. The simpler `exec > /dev/null 2>&1` or `read -r -d '' _` would be more standard. This is a minor clarity issue, not a correctness problem. diff --git a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/usability/REPORT.md b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/usability/REPORT.md index 328c67bb8..1ee7815b6 100644 --- a/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/usability/REPORT.md +++ b/.claude/pr-reviews/nsheaps/ai-mktpl/306/1774399314/usability/REPORT.md @@ -1,4 +1,5 @@ # Usability Review — PR #306 + Score: 81/100 ## Summary @@ -60,6 +61,7 @@ On SessionStart with no changes, the script outputs `"github: PR state baseline ### Output format The bulleted list format for changes is clean: + ``` github: PR state changes detected since last check: @@ -68,6 +70,7 @@ github: PR state changes detected since last check: Review these changes and determine if any action is needed. ``` + This is readable, correctly prefixed with the plugin name, and ends with an explicit action prompt. The trailing blank lines and the closing call-to-action improve agent comprehension over a raw list. ## References From 022cc8e707dcb5d01a75b2bc1359afeee3657e0a Mon Sep 17 00:00:00 2001 From: "jack-nsheaps[bot]" Date: Sun, 5 Apr 2026 19:36:49 -0400 Subject: [PATCH 10/11] chore: merge main --- .claude-plugin/marketplace.json | 133 +++++++++++++++++++++++++++++--- 1 file changed, 123 insertions(+), 10 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 9e9c247b5..78fcf1166 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -11,7 +11,7 @@ { "name": "1pass", "description": "Install and manage 1Password CLI (op) and op-exec in Claude Code sessions. Provides session-start auto-install for web sessions and comprehensive workflow skills.", - "version": "0.2.0", + "version": "0.3.0", "author": { "name": "Nathan Heaps" }, @@ -41,6 +41,72 @@ "tags": ["utility"], "keywords": ["tmux", "iterm2", "tab-title", "agent-teams", "pane-title"] }, + { + "name": "arcane", + "description": "Skills for deploying docker-compose stacks via Arcane GitOps — directory conventions, secrets management with 1Password, and GitHub Actions CI/CD.", + "version": "0.1.0", + "author": { + "name": "Nathan Heaps" + }, + "source": "./plugins/arcane", + "category": "utility", + "tags": ["utility", "skill"], + "keywords": [ + "arcane", + "gitops", + "docker-compose", + "deployment", + "1password", + "secrets", + "docker", + "containers", + "self-hosted", + "homelab" + ] + }, + { + "name": "brain", + "description": "Git-backed memory and prompt tracking. Auto-saves prompts to history, syncs memory files to a configurable git repo, and provides self-checking reminders inspired by the Serena MCP Ralph loop pattern.", + "version": "0.1.0", + "author": { + "name": "Nathan Heaps" + }, + "source": "./plugins/brain", + "category": "utility", + "tags": ["utility", "skill"], + "keywords": ["memory", "history", "git", "brain", "prompt-tracking", "self-check"] + }, + { + "name": "cloudflare", + "description": "Skills for managing Cloudflare developer platform components including Workers, R2, D1, AI Gateway, Pages, Tunnels, and more — with Pulumi IaC setup for each.", + "version": "0.1.0", + "author": { + "name": "Nathan Heaps" + }, + "source": "./plugins/cloudflare", + "category": "utility", + "tags": ["utility", "skill"], + "keywords": [ + "cloudflare", + "workers", + "r2", + "d1", + "kv", + "pages", + "tunnels", + "ai-gateway", + "zero-trust", + "durable-objects", + "queues", + "vectorize", + "dns", + "pulumi", + "infrastructure-as-code", + "iac", + "serverless", + "edge-computing" + ] + }, { "name": "code-simplifier", "description": "Simplify and refine code for clarity, consistency, and maintainability. Requires pr-review-toolkit plugin.", @@ -213,8 +279,8 @@ }, { "name": "github", - "description": "GitHub CLI installation, authentication, PR state tracking, and workflow skill for Claude Code sessions. Monitors comments, reviews, CI status, and merge readiness across multi-project sessions.", - "version": "0.1.14", + "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", "author": { "name": "Nathan Heaps" }, @@ -234,17 +300,16 @@ "session-start", "web-session", "auto-install", - "pr-state-tracking", - "ci-status", - "reviews", - "async-hooks", - "multi-project" + "pr-feedback", + "code-review", + "ci-failures", + "review-comments" ] }, { "name": "github-app", "description": "Automatic GitHub App token lifecycle for Claude Code sessions. Generates installation tokens on session start, monitors expiry via PreToolUse hook, and refreshes transparently before commands that need authentication.", - "version": "0.1.11", + "version": "0.1.12", "author": { "name": "Nathan Heaps" }, @@ -431,6 +496,28 @@ "product-management" ] }, + { + "name": "proxmox", + "description": "Skills for managing Proxmox VE hosts and LXC containers — creating containers, running Docker in LXC, and hosting services like cloudflared.", + "version": "0.1.0", + "author": { + "name": "Nathan Heaps" + }, + "source": "./plugins/proxmox", + "category": "utility", + "tags": ["utility", "skill"], + "keywords": [ + "proxmox", + "pve", + "lxc", + "containers", + "virtualization", + "homelab", + "self-hosted", + "docker", + "infrastructure" + ] + }, { "name": "remote-config", "description": "Sync upstream Claude config repo on session start. Pulls latest from a configured git repo to ~/.claude-remote/ and reports update status.", @@ -544,7 +631,7 @@ { "name": "skill-required", "description": "Enforces that specified skills are loaded before certain tools can be used. Tracks skill reads via PostToolUse and blocks tool use via PreToolUse if the required skill hasn't been recently loaded.", - "version": "0.1.3", + "version": "0.1.4", "author": { "name": "Nathan Heaps" }, @@ -706,6 +793,32 @@ "workflow", "hooks" ] + }, + { + "name": "zai-glm", + "description": "Skills for using z.ai (Zhipu AI) GLM models and configuring Claude Code to access them via the Anthropic-compatible endpoint or API gateways.", + "version": "0.1.0", + "author": { + "name": "Nathan Heaps" + }, + "source": "./plugins/zai-glm", + "category": "utility", + "tags": ["utility", "skill"], + "keywords": [ + "zai", + "z-ai", + "zhipu", + "glm", + "glm-5", + "glm-4.7", + "glm-4.5", + "chatglm", + "ai-models", + "openai-compatible", + "anthropic-compatible", + "china-ai", + "llm-provider" + ] } ] } From a0af2e0df082373ab56fcd434abae78da0c3f283 Mon Sep 17 00:00:00 2001 From: "jack-nsheaps[bot]" <254347511+jack-nsheaps[bot]@users.noreply.github.com> Date: Sun, 5 Apr 2026 23:40:08 +0000 Subject: [PATCH 11/11] chore: auto-bump plugin versions and update marketplace --- .claude-plugin/marketplace.json | 2 +- plugins/github/.claude-plugin/plugin.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 78fcf1166..a96d51ac1 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -280,7 +280,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" }, diff --git a/plugins/github/.claude-plugin/plugin.json b/plugins/github/.claude-plugin/plugin.json index cdcba576a..7632647a8 100644 --- a/plugins/github/.claude-plugin/plugin.json +++ b/plugins/github/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "github", - "version": "0.1.15", + "version": "0.1.16", "description": "GitHub CLI installation, authentication, and workflow skill for Claude Code sessions. Consolidates gh-tool and github-auth-skill into a single plugin.", "author": { "name": "Nathan Heaps",