github-pr-review

name	github:pr-review
description	Automated PR review for Kagenti - conventions, security, CI status, inline comments

flowchart TD
    START([PR Review]) --> GATHER["Phase 1: Gather"]:::github
    GATHER --> ANALYZE["Phase 2: Analyze"]:::github
    ANALYZE --> REVIEW["Phase 3: Review"]:::github
    REVIEW --> DRAFT["Phase 4: Draft"]:::github
    DRAFT -->|User approves| SUBMIT["Phase 5: Submit"]:::github
    DRAFT -->|User edits| DRAFT

    classDef github fill:#E91E63,stroke:#333,color:white

Follow this diagram as the workflow.

PR Review

Automated code review workflow. Gathers PR data, analyzes the diff, checks against repo conventions and CI, drafts inline review comments, and posts a GitHub review after user approval.

Variables

Set at session start:

export OWNER=<org-or-user>
export REPO=<repo-name>

Table of Contents

• Variables
• When to Use
• Context-Safe Execution
• Phase 1: Gather PR Data
  • 1.6 Author Trust Level
• Phase 2: Analyze Changes
• Phase 3: Review Checklist
• Phase 4: Draft Review
• Phase 5: Submit Review
• Troubleshooting
• Related Skills

When to Use

• Reviewing a PR before approving or requesting changes
• Checking a PR against Kagenti conventions before merge
• Providing structured, inline feedback on PRs
• Invoked as /github:pr-review <PR-number>

Context-Safe Execution (MANDATORY)

PR diffs can be very large. Always redirect diff output to files and analyze with subagents.

export LOG_DIR="${LOG_DIR:-${WORKSPACE_DIR:-/tmp}/kagenti-review}"
mkdir -p "$LOG_DIR"

Small output OK inline: gh pr checks, gh pr view --json (metadata only).

Large output MUST redirect: gh pr diff, commit logs, file contents.

Phase 1: Gather PR Data

Collect all PR metadata, diff, CI status, and commit history.

1.1 PR Metadata

gh pr view <number> --json number,title,body,author,baseRefName,headRefName,commits,files,reviews,reviewDecision,labels,createdAt,updatedAt

1.2 PR Diff

gh pr diff <number> > $LOG_DIR/pr-<number>.diff 2>&1; echo "EXIT:$?"

1.3 CI Status

gh pr checks <number>

If checks are failing, delegate to ci:status for detailed analysis.

1.4 Commit History

gh pr view <number> --json commits --jq '.commits[] | "\(.oid[:7]) \(.messageHeadline)"'

1.5 Sync Local Repo (CRITICAL)

Before verifying ANY PR claims against local source files, fetch the latest upstream:

# Determine the target repo and sync
cd <local-clone-path>
git fetch upstream main

Anti-pattern: Reading local files without fetching first. The PR diff comes from GitHub (up-to-date), but local files may be days behind. This mismatch causes false negatives — flagging correct version claims as wrong.

When verifying claims (versions, file existence, code patterns), always use:

# Verify against upstream/main, NOT local working tree
git show upstream/main:<path-to-file>

1.6 Author Trust Level

The PR author's relationship to the project is itself a risk signal. Supply-chain attacks usually arrive through PRs from people new to the repo. Gather the author's GitHub association on every review. The gh pr view --json form does not expose this field — use the REST API:

gh api repos/$OWNER/$REPO/pulls/<number> --jq '{author: .user.login, association: .author_association}'

Classify the result:

author_association	Trust	Review posture
OWNER, MEMBER, COLLABORATOR	Maintainer	Normal review
CONTRIBUTOR	Returning external	Elevated scrutiny
FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, NONE	New / untrusted	Highest scrutiny

For elevated or highest scrutiny:

• Read every changed file in full — no summary-only passes.
• Pay special attention to build, CI, dependency-manifest, and any config files (see §3.5 and §3.5a).
• Never auto-approve. Always route through the §4.4 confirmation gate, even when the diff is otherwise clean. First-time contributors get the loudest warning.

Record the author and trust level — it appears in the Phase 4 summary (§4.2) and drives the verdict (§4.3).

Phase 2: Analyze Changes

Use a subagent to categorize the diff by area and produce a summary.

Task(subagent_type='Explore'):
  "Read $LOG_DIR/pr-<number>.diff. Categorize changed files into these areas:
   Python, Helm/K8s, Shell, YAML, Dockerfile, CI/GitHub Actions, Docs, Frontend,
   Agent/IDE config (.claude/ or .vscode/), Other.
   For each area return: files changed, lines added/removed.
   ALWAYS return, verbatim, the full list of any changed paths under .claude/ or
   .vscode/ (added, modified, or renamed) — even if there are no other findings.
   Return a brief summary of what the PR does overall (2-3 sentences).
   Do NOT return the full diff content."

The summary tells us which review criteria to apply in Phase 3 (only check areas the PR touches). The verbatim .claude//.vscode/ path list ensures the §3.5a supply-chain gate is never silently skipped just because the diff was summarized.

Phase 3: Review Checklist

Apply Kagenti-specific review criteria only for areas the PR touches.

Reminder: When verifying version numbers, file paths, or code claims in the PR, use git show upstream/main:<file> — never trust the local working tree without fetching first (see §1.5).

3.1 Commit Conventions

Check	Criteria
Signed-off	Every commit has Signed-off-by: line (DCO — enforced, blocking)
Subject prefix	Conventional prefix, e.g. fix:, Feat:, Docs: (advisory, case-insensitive) — same set as the PR title list in §3.2
Imperative mood	"Add feature" not "Added feature" (advisory)
Length	Subject line under 72 characters (advisory)

Note: the git:commit skill documents an emoji-prefix style, but actual practice across the org (and the pr-verifier-required workflow) is the conventional prefixes above. Matching is case-insensitive (fix:, Fix:, and FIX: are all valid). Treat any prefix deviation as a nit, not a blocker. Only DCO sign-off is blocking.

# Check sign-off on all PR commits
gh pr view <number> --json commits --jq '.commits[].messageBody' | grep -c 'Signed-off-by'

3.2 PR Format

Check against repo:pr conventions:

Check	Criteria
Title prefix	Starts with a conventional prefix, under 72 chars (advisory)
Summary section	Body contains ## Summary
Issue linking	Fixes #N or Closes #N if applicable (optional)

Title prefix must be one of (case-insensitive, per kagenti/.github pr-verifier-required.yml): Build, Chore, CI, Docs, Feat, Fix, Perf, Refactor, Revert, Style, Test, Feature, Bug fix, Proposal, Breaking change, Other, Other/Misc.

This is advisory only — the title check is not a required/blocking status check, and only runs in repos that invoke pr-verifier.yml. Flag a non-conforming title as a nit, not a blocker. Do not flag it at all in repos that do not run the verifier workflow.

3.3 Python Changes

If the PR touches .py files:

Check	What
Formatting	Would ruff format --check pass?
Linting	Any pylint/ruff issues in changed files?
Security	No bandit HIGH severity issues in changed code
Imports	Clean imports, no unused

3.4 Helm / Kubernetes Changes

If the PR touches charts/, K8s manifests, or values files:

Check	What
Chart lint	helm lint would pass
Labels	Uses kagenti.io/* labels where appropriate
Resource limits	Containers have resource requests/limits
Values	New values documented or self-explanatory

3.5 Security

Always check regardless of area:

Check	What
Secrets	No hardcoded secrets, tokens, passwords in diff
Actions	GitHub Action versions pinned to SHA (not @main or @v1)
Dependencies	New dependencies reviewed for supply-chain risk
Dockerfiles	Non-root user, pinned base images

3.5a Agent / IDE Config Files (supply-chain gate)

Why this exists: Recent supply-chain attacks smuggle agent/IDE config files into PRs so that merely opening, building, or reviewing the branch runs attacker code on the maintainer's machine. Treat these files as hostile until proven safe.

Scan every PR for changes under .claude/ or .vscode/ (added, modified, or renamed):

# Added / modified files appear as +++ b/ headers.
# Pure renames have NO +++ b/ line — git emits `rename to <path>` instead — so
# grep both, or a config file slipped in via a rename would go undetected.
grep -nE '^\+\+\+ b/(.*/)?\.(claude|vscode)/|^rename to (.*/)?\.(claude|vscode)/' $LOG_DIR/pr-<number>.diff

If there is any match, this is a must-fix supply-chain finding. Do the following:

1. Add a must-fix inline comment on each flagged file telling the reviewer to read the full file contents closely — these are not routine config tweaks.

2. Read every flagged file in full and call out the high-risk payloads:

   File / pattern	What to inspect
   .claude/settings.json, .claude/settings.local.json	hooks, any command, runOptions, auto-run / permission grants
   .claude/*.mjs, *.js, *.sh, *.py	Any executable referenced from config; obfuscated or base64 content
   .vscode/tasks.json, .vscode/launch.json	runOptions.runOn: folderOpen, command/args, pre-launch tasks
   .vscode/settings.json, extensions.json	Auto-run settings, recommended extensions from untrusted publishers

3. The verdict cannot be APPROVE while these changes are present unless the user explicitly confirms inspection (see §4.3 and §4.4).

🚨 CONFIRMED MALWARE — STOP IMMEDIATELY 🚨

If a .claude/settings.json in the diff contains "command": "node .claude/setup.mjs", this is a known malware signature.

• Do NOT check out the branch, run any command, or open the referenced .claude/setup.mjs (or any other file it points to).
• Halt the review. Do not draft or submit a GitHub review.
• Report it immediately to the user: state that the PR is confirmed malware, and recommend closing the PR and alerting the org / security contact.
• Take no further action on the PR content.

⚠️ The exact string above is illustrative, not the safeguard. It is a fast-path "loud halt" for one observed payload — a trivially renamed file (setup2.mjs), a different command, or any other obfuscation slips past this literal match. The actual protection is the general §3.5a gate: full inspection of every .claude//.vscode/ file, with no APPROVE permitted until the reviewer confirms inspection. Treat the signature as one known-bad example on top of that gate — never as the whole defense.

3.6 Shell Scripts

If the PR touches .sh files:

Check	What
Shellcheck	Would shellcheck pass?
Error handling	Uses set -euo pipefail or equivalent
Quoting	Variables properly quoted

3.7 YAML

If the PR touches .yaml/.yml files:

Check	What
yamllint	Would yamllint pass?
Indentation	Consistent indentation

3.8 Tests

Check	What
Coverage	New features have corresponding tests
Quality	Tests are assertive, no hidden skips (delegate to test:review)
E2E	User-facing changes have E2E coverage

3.9 Documentation

Check	What
Updated	User-facing changes have docs/README updates
Accurate	Docs match the actual behavior

3.10 Feature Gate / Dual-Mode Code

If the PR introduces two code paths behind a feature gate (e.g. legacy vs resolved, ValueFrom vs literal):

Check	What
Source parity	Both paths read config from the same ConfigMaps/keys
Env var parity	Both paths inject the same env var names
Fallback parity	Missing-resource behavior is equivalent across paths

Cross-file search: For each key constant or ConfigMap name introduced in new code, grep across ALL files in the diff to verify both paths reference the same source. Dual-mode bugs often appear as a value read from CM-A in the new path but CM-B in the legacy path — the inconsistency is only visible by comparing both files side-by-side.

# Example: find all ConfigMap name references in the diff (substring match)
grep -E 'authbridge-config|environments|spiffe-helper' $LOG_DIR/pr-<number>.diff

This check catches a class of bugs where a new code path was written against a planned data layout that differs from the actual deployment (e.g. design doc says key X lives in CM-A, but existing deployments have it in CM-B).

Phase 4: Draft Review

Present proposed review to user for approval before posting.

4.1 Inline Comments Table

## Proposed Review Comments

### Inline Comments
| # | File | Line | Severity | Comment |
|---|------|------|----------|---------|
| 1 | path/to/file.py | 42 | must-fix | Description of issue... |
| 2 | charts/values.yaml | 15 | suggestion | Consider adding... |
| 3 | scripts/deploy.sh | 8 | nit | Minor style issue... |

Severity levels:

• must-fix - Blocks merge. Security issues, broken functionality, missing sign-off.
• suggestion - Should fix but not blocking. Better patterns, missing tests.
• nit - Trivial. Style, naming, minor improvements.

Do NOT include praise comments — they clutter the review without adding value.

4.2 Summary Comment

### Summary
[2-3 sentence overview of the review findings]

**Author**: <login> (<association> — <trust tier>)
**Areas reviewed**: Python, Helm, CI (list areas actually checked)
**Agent/IDE config (.claude/.vscode)**: none / FLAGGED (list paths)
**Commits**: N commits, all signed-off: yes/no
**CI status**: passing/failing/pending

Map <association> to its §1.6 trust tier so the line is self-explanatory: MEMBER/OWNER/COLLABORATOR → maintainer, CONTRIBUTOR → returning external, FIRST_TIME_CONTRIBUTOR/FIRST_TIMER/NONE → first-time (e.g. **Author**: alice (MEMBER — maintainer)).

4.3 Verdict

Decision tree (follow in order):

1. Confirmed malware signature present? (.claude/settings.json with "command": "node .claude/setup.mjs", see §3.5a) → HALT. Do not submit any review. Report to the user immediately.
2. Any .claude/ or .vscode/ change? (see §3.5a) → verdict cannot be APPROVE. Default to REQUEST_CHANGES with the must-fix supply-chain comment, unless the user has explicitly confirmed in the current turn that they inspected and validated the changes (see §4.4).
3. Non-maintainer author? (CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, NONE, see §1.6) → may still APPROVE on a clean diff, but never auto-approve — route through the §4.4 confirmation gate first.
4. Any other must-fix issues? → REQUEST_CHANGES.
5. Otherwise (trusted maintainer, clean diff) → APPROVE.

Suggestions and nits are included as inline comments within the APPROVE review — they do NOT downgrade the verdict. Never use COMMENT as the event; it withholds approval unnecessarily when there are no blocking issues.

4.4 User Approval

Present the full draft and ask user to approve, edit, or cancel:

AskUserQuestion:
  "Review draft ready. Approve to submit, or edit first?"
  Options: ["Submit as-is", "Edit comments first", "Cancel review"]

Inspection gate (MANDATORY). When the diff touches .claude//.vscode/ (§3.5a) or the author is a non-maintainer (§1.6), the normal prompt is replaced by an explicit inspection gate. The skill will not approve without the user's explicit confirmation in this turn:

AskUserQuestion:
  "⚠️ This PR requires manual inspection before approval.
   Author: <login> (<association>).
   Flagged agent/IDE config files: <list, or 'none'>.
   I will NOT approve until you confirm you inspected and validated these changes."
  Options:
    - "I inspected these and they are safe — proceed to APPROVE"
    - "Request changes / do not approve"
    - "Cancel review"

For a first-time contributor, make the warning the loudest item in the prompt. A confirmed-malware signature (§3.5a) never reaches this gate — it halts in §4.3 step 1.

Phase 5: Submit Review

After user approves, post the review via GitHub API.

5.1 Post Review with Inline Comments

# Build the review payload as JSON (gh api does NOT support array params via -f)
# For each inline comment: path, line (in the file on HEAD side), body
# event: APPROVE (default when no must-fix) or REQUEST_CHANGES (when must-fix exists)

cat <<'EOF' | gh api repos/{owner}/{repo}/pulls/<number>/reviews --method POST --input -
{
  "event": "APPROVE",
  "body": "Review summary text...",
  "comments": [
    {"path": "path/to/file.py", "line": 42, "body": "Comment text..."},
    {"path": "charts/values.yaml", "line": 15, "body": "Another comment..."}
  ]
}
EOF

Note: Use "event": "APPROVE" when there are no must-fix issues (even if there are suggestions/nits). Only use "event": "REQUEST_CHANGES" when must-fix issues exist.

Note: gh api is NOT auto-approved. The user will be prompted to approve the review submission. This is intentional — reviews are write operations.

5.2 Confirm Submission

After posting, confirm with a link:

Review submitted on PR #<number>: https://github.com/$OWNER/$REPO/pull/<number>
- Verdict: APPROVE / REQUEST_CHANGES / COMMENT
- Inline comments: N

Troubleshooting

Line numbers in diff vs file

The line parameter in the review API refers to the line number in the diff hunk, not the file. Use the diff output to determine correct line numbers. The line field should be the line number in the file on the HEAD side of the diff.

Review already exists

GitHub allows multiple reviews. A new review will be added alongside existing ones.

Large PRs (> 500 changed lines)

For very large PRs, focus the review on:

1. Security issues (always)
2. Architecture / design concerns
3. Convention violations
4. Skip nit-level comments to avoid noise

PR from fork

For PRs from forks, gh pr diff still works but branch checkout may not. Use the diff file for all analysis.

Cross-repo reviews

When verifying cross-repo dependency files (e.g. checking whether kagenti-extensions already handles something being removed from $OWNER/$REPO), always fetch directly from GitHub — never trust a local clone:

# Fetch a specific file from the latest main of another repo
gh api repos/{owner}/{repo}/contents/{path/to/file} --jq '.content' \
  | base64 -d > /tmp/file-from-upstream.go

This is more reliable than a local clone, which may be stale or have no upstream remote configured. Use the GitHub API for targeted single-file verification; a local clone is only appropriate for broad multi-file exploration.

Anti-pattern — stale cross-repo local clone:

git show upstream/main:<file> in a sibling repo can silently return stale content if the repo hasn't been fetched this session, or if there is no upstream remote. A fetch you didn't explicitly run this session is a stale fetch.

The --repo flag works with gh pr commands, but cross-repo source verification should go through the API, not the local filesystem.

Stale local checkout (anti-pattern)

Symptom: PR claims a version/fact. You check the local file and it disagrees. You flag it as wrong. The PR author says it's correct.

Cause: Your local main is behind upstream/main. The PR was based on the latest upstream, which has newer dependency versions.

Fix: Always run git fetch upstream main and use git show upstream/main:<file> before cross-referencing PR claims against source files. See §1.5.

Subagent model access denied (401)

If the Explore subagent fails with team not allowed to access model or similar 401 error, fall back to reading the diff directly in the main context.

When reading large diffs without subagent help, explicitly cross-reference related files — don't read them sequentially and assume consistency:

1. After reading each new function/struct, search for its key constants/field names across the entire diff: grep "ConstantName\|fieldName" $LOG_DIR/pr-<number>.diff
2. For any new config source (ConfigMap names, API paths), verify both the new code AND any legacy/fallback code use the same source name.
3. For dual-mode PRs: compare the two paths side-by-side rather than reading each path file-by-file in sequence.

gh api array parameters

The gh api CLI does not support array parameters via -f 'comments[0][path]=...'. Use JSON input instead:

cat <<'EOF' | gh api repos/{owner}/{repo}/pulls/<number>/reviews --method POST --input -
{
  "event": "APPROVE",
  "body": "Review summary...",
  "comments": [
    {"path": "file.py", "line": 42, "body": "Comment text..."}
  ]
}
EOF

Related Skills

• ci:status - Detailed CI check analysis for failing PRs
• test:review - Deep test quality review
• repo:pr - PR format conventions
• git:commit - Commit format conventions
• github:prs - PR health overview (batch analysis)
• rca:ci - Root cause analysis when CI fails