--- name: pr-review description: Review a GitHub pull request using multiple expert personas. Takes a PR URL as input, analyzes the changes, and generates comprehensive review feedback from different perspectives (Merge Specialist, Frontend, Backend, Security, DevOps, AI/Agent, SRE, Chief Architect). license: Apache-2.0 metadata: author: mcp-gateway-registry version: "1.1" --- # PR Review Skill Use this skill to review GitHub pull requests comprehensively using multiple expert personas. Each persona brings specialized knowledge to identify issues from different perspectives. ## Input The skill takes a GitHub PR URL as input: - Format: `https://github.com/{owner}/{repo}/pull/{number}` - Example: `https://github.com/agentic-community/mcp-gateway-registry/pull/123` ## Output Creates review documentation in `.scratchpad/pr-{pr-number}/` containing: - `review.md` - Comprehensive review from all personas ## Workflow ### Step 1: Parse PR URL and Fetch PR Details 1. Extract the PR number from the URL 2. Use `gh pr view {number}` to get PR details 3. Use `gh pr diff {number}` to get the changes 4. Identify which files are changed and their types (frontend, backend, etc.) ### Step 2: Determine Relevant Personas Based on the files changed, determine which personas should review: | Changed Files | Personas to Engage | |---------------|-------------------| | `/frontend/**` | Merge Specialist, Frontend Developer, Chief Architect | | `/registry/**` | Merge Specialist, Backend Developer, Security Engineer, SRE, Chief Architect | | `/registry/core/config.py`, `/registry/api/config_routes.py` | Merge Specialist, Backend Developer, **DevOps Engineer**, Security Engineer, Chief Architect | | `/auth_server/**` | Merge Specialist, Backend Developer, Security Engineer, Chief Architect | | `/terraform/**`, `/charts/**`, `/docker/**` | Merge Specialist, DevOps Engineer, SRE, Chief Architect | | `/agents/**`, `/servers/**` | Merge Specialist, AI/Agent Developer, Backend Developer, Chief Architect | | `/metrics-service/**` | Merge Specialist, SRE Engineer, Backend Developer, Chief Architect | | `*.md`, `docs/**` | Merge Specialist, Chief Architect | | `pyproject.toml`, `requirements*.txt` | Merge Specialist, DevOps Engineer, Security Engineer, Chief Architect | | `tests/**` | Merge Specialist, Backend Developer, Chief Architect | | `.env.example` | Merge Specialist, **DevOps Engineer**, Chief Architect | **Note:** Merge Specialist and Chief Architect always participate in every review. ### Step 2.5: Detect New or Modified Configuration Parameters (CRITICAL) Before running any reviews, determine whether this PR introduces or modifies any configuration parameters across the three deployment surfaces. If it does, the unified parameter reference must be updated in the same PR. A missing update is a **blocker**. **Detection command:** ```bash # Any diff that touches one of the canonical parameter-carrying files triggers this check gh pr diff {pr-number} --name-only | grep -E \ -e '^\.env\.example$' \ -e '^docker-compose(\.|$)' \ -e '^terraform/aws-ecs/terraform\.tfvars\.example$' \ -e '^terraform/aws-ecs/variables\.tf$' \ -e '^terraform/aws-ecs/modules/.+/(variables|ecs-services)\.tf$' \ -e '^charts/.+/values\.yaml$' \ -e '^charts/.+/templates/(deployment|secret)\.yaml$' \ -e '^registry/core/config\.py$' \ -e '^registry/api/config_routes\.py$' ``` **If any files match**, every new or renamed parameter must be reflected in `docs/unified-parameter-reference.md`. Verify with: ```bash # For each new parameter name, confirm the reference file mentions it for PARAM in $(gh pr diff {pr-number} | grep -E '^\+[A-Z_]{3,}=' | sed 's/^+//;s/=.*//' | sort -u); do if ! grep -q "$PARAM" docs/unified-parameter-reference.md; then echo "MISSING from unified-parameter-reference.md: $PARAM" fi done ``` **Merge-blocking checks (add to the Merge Specialist review section):** - [ ] `docs/unified-parameter-reference.md` is included in the diff whenever any parameter-carrying file is touched. - [ ] Every new `.env` variable has a row, with Docker / Terraform / Helm columns filled (or explicitly blank with a justification in the PR description). - [ ] Every new Terraform variable appears in the reference. - [ ] Every new Helm value appears in the reference. - [ ] Secrets are flagged with **(secret)**. - [ ] New rows live in an existing logical group, or the PR adds a new group with a clear rationale. - [ ] Renamed parameters have the old row updated in place (not a duplicate). - [ ] Deleted parameters have their row removed (not left stale). - [ ] `registry/api/config_routes.py` `CONFIG_GROUPS` is updated so the parameter surfaces in `GET /api/config/full` and the System Config UI. If any of the above is missing, the review verdict is **REQUEST CHANGES** with a blocker titled "Unified parameter reference not updated". ### Step 2.6: Detect New or Changed API Endpoints (CRITICAL) `api/openapi.json` is the published API contract, and it is **hand-refreshed**: no script generates it and no CI job checks it. A PR that adds a route therefore passes every gate while leaving the new endpoint invisible to API consumers. This has already happened, more than once in a row, so the spec shipped missing endpoints from several merged PRs at the same time. **Detection:** does the diff add or change a route decorator? ```bash # Matches ANY router object, not just `router`/`app`: real routes are declared on # names like `cimd_router`, `wellknown_router`, and a narrower pattern misses them # (verified: it missed #1711's `@cimd_router.get(...)` entirely). gh pr diff {pr-number} | grep -E '^\+\s*@[A-Za-z_][A-Za-z0-9_]*\.(get|post|put|patch|delete)\(' ``` **If that matches**, check whether the spec was refreshed: ```bash gh pr diff {pr-number} --name-only | grep -q '^api/openapi\.json$' \ && echo "spec refreshed" || echo "SPEC NOT REFRESHED" ``` And confirm the new path actually landed in it, rather than the file being touched for an unrelated reason: ```bash git show {pr-branch}:api/openapi.json | python3 -c " import json, sys print(sorted(json.load(sys.stdin)['paths'])) " | tr ',' '\n' | grep -i '' ``` **Merge-blocking checks:** - [ ] Every route the diff adds appears in `api/openapi.json`. - [ ] Every route the diff removes is gone from it. - [ ] `info.version` is a clean release semver, not the app's git-describe development string (`1.30.0-46-g...`). - [ ] The diff to `api/openapi.json` is confined to the affected paths. A wholesale reformat usually means it was regenerated with `ensure_ascii=False`, which rewrites every non-ASCII character and hides the real change. - [ ] Nothing was removed unexpectedly. Unexplained removals usually mean the container was built with a feature flag off or the wrong `DEPLOYMENT_MODE`, so a whole router never registered. A route added behind a default-off feature flag still belongs in the spec: FastAPI registers the route regardless, and the flag only changes the response at request time. If a route was added and the spec was not refreshed, the verdict is **REQUEST CHANGES** with a blocker titled "OpenAPI spec not refreshed for new endpoint". The procedure is in [AGENTS.md](../../../AGENTS.md#regenerating-apiopenapijson); refreshing it can also be offered as a follow-up PR when the author would rather not rebuild locally. ### Step 3: Run Tests and Quality Checks Before reviewing, run the test suite to verify the PR doesn't break anything: ```bash # Checkout the PR gh pr checkout {pr-number} # Run tests uv run pytest tests/ -n 8 --tb=short # Run linting uv run ruff check . && uv run ruff format --check . # Run security scan (if applicable) uv run bandit -r registry/ auth_server/ -q # Return to main branch when done git checkout main ``` ### Step 4: Create Review Folder Create the folder structure: ``` .scratchpad/pr-{pr-number}/ └── review.md ``` ### Step 5: Conduct Multi-Persona Review For each relevant persona, adopt that perspective and review the changes. Reference the persona definition files: > **Theory check (always):** the Chief Architect persona must read > [Theory of the System](../../../docs/design/theory-of-the-system.md) and walk the diff against its > "how to change this system without breaking its theory" checklist. If the PR violates a core > invariant (control-plane/data-plane split, generic gateway, A2A peer-to-peer, mode axes, config > parity, fail-closed admission, IdP-agnosticism, MCP spec compliance) without explicitly arguing > for the change, **flag it to the user as a blocker.** - [Merge Specialist](personas/merge-specialist.md) - Always included - [Frontend Developer](personas/frontend-developer.md) - For frontend changes - [Backend Developer](personas/backend-developer.md) - For backend/API changes - [Security Engineer](personas/security-engineer.md) - For auth/security changes - [DevOps Engineer](personas/devops-engineer.md) - For infrastructure changes - [AI/Agent Developer](personas/ai-agent-developer.md) - For agent/MCP changes - [SRE Engineer](personas/sre-engineer.md) - For observability/metrics changes - [Chief Architect](personas/chief-architect.md) - Always included (final synthesis) ### Step 6: Write Comprehensive Review (review.md) Generate the review document using this structure: ```markdown # PR Review: #{pr-number} - {pr-title} *Review Date: {date}* *PR URL: {pr-url}* *Author: {author}* ## PR Summary {Brief description of what the PR does based on PR description and changes} ### Files Changed | File | Type | Lines Added | Lines Removed | |------|------|-------------|---------------| | {file} | {type} | +{n} | -{n} | ### Test Results | Check | Status | Details | |-------|--------|---------| | Unit Tests | {PASS/FAIL} | {summary} | | Integration Tests | {PASS/FAIL} | {summary} | | Linting | {PASS/FAIL} | {summary} | | Security Scan | {PASS/FAIL} | {summary} | ### API Spec Check *Only required when the diff adds or changes a route decorator (see Step 2.6). Mark "Not Applicable" otherwise.* | Check | Status | Details | |-------|--------|---------| | Every added route appears in `api/openapi.json` | {PASS/FAIL/N/A} | {the paths} | | Every removed route is gone from it | {PASS/FAIL/N/A} | n/a | | `info.version` is a release semver, not a dev string | {PASS/FAIL/N/A} | n/a | | Spec diff confined to the affected paths | {PASS/FAIL/N/A} | n/a | ### Configuration Parameter Surface Check *Only required when the PR touches any parameter-carrying file (see Step 2.5). Mark "Not Applicable" if the detection command returned no matches.* | Check | Status | Details | |-------|--------|---------| | Unified parameter reference updated (`docs/unified-parameter-reference.md`) | {PASS/FAIL/N/A} | {list of new/renamed/removed parameter names and which rows were added} | | Docker column populated (`.env.example`, `docker-compose*.yml`) | {PASS/FAIL/N/A} |, | | Terraform column populated (`variables.tf`, `terraform.tfvars.example`, module wiring) | {PASS/FAIL/N/A} |, | | Helm column populated (`charts/.../values.yaml`, stack values, templates) | {PASS/FAIL/N/A} |, | | `registry/api/config_routes.py` `CONFIG_GROUPS` updated | {PASS/FAIL/N/A} |, | | Secrets flagged with **(secret)** and wired through Secrets Manager / `secretKeyRef` | {PASS/FAIL/N/A} |, | --- ## Review Panel | Role | Reviewer | Verdict | |------|----------|---------| | Merge Specialist | Gatekeeper | {verdict} | | {Role} | {Name} | {verdict} | | Chief Architect | Atlas | {verdict} | --- {Include each relevant persona's review section using the format from their persona file} --- ## Review Summary | Reviewer | Verdict | Blockers | Key Concerns | |----------|---------|----------|--------------| | {Reviewer} | {verdict} | {count} | {summary} | ### Blockers (Must Fix) 1. {Blocker description} - Raised by: {persona} - File: `{file:line}` - Fix: {suggested fix} ### Should Fix (Important) 1. {Issue description} - Raised by: {persona} - File: `{file:line}` - Recommendation: {suggestion} ### Consider (Nice to Have) 1. {Suggestion} - Raised by: {persona} --- ## Final Recommendation **Overall Verdict: {APPROVE / APPROVE WITH CHANGES / REQUEST CHANGES}** ### Required Actions Before Merge - [ ] {Action 1} - [ ] {Action 2} - [ ] (If config params changed) `docs/unified-parameter-reference.md` updated and all three surface columns consistent with the diff ### Post-Merge Actions - [ ] {Action 1} ``` ### Step 7: Render the review as HTML A review document is long and heavily tabular, which reads badly as raw markdown in a terminal. Render it so the reader can open a formatted page instead: ```bash uv run python scripts/render-doc-html.py .scratchpad/pr-NNNN/review.md ``` That writes `review.html` beside the markdown, self-contained (all CSS inline, no CDN, no JavaScript, renders from `file://`), using the same stylesheet as the explainer skill so every generated document looks the same. Title and byline come from the document's H1 and the italic lines under it, and a section nav is built from the H2 headings. Useful flags: - `--footer-html '...'` for provenance, such as the commit the review was performed against. - `--diagrams DIR` to inline SVG from `DIR/.svg` wherever the markdown fences a block as ` ```svg: Optional caption `. Keep the ASCII inside the fence: it is what a terminal reader sees, and a missing `.svg` file falls back to it rather than losing the diagram. A review rarely needs this; a design document often does. - `--code-style invert` for the template's dark code blocks. The default (`match`) makes code blocks follow the page surface, which suits a document that is mostly code. Then check the output, because an unparsed HTML file is worse than none: ```bash uv run python scripts/prose-scan.py --strict .scratchpad/pr-NNNN/review.md .scratchpad/pr-NNNN/review.html ``` The renderer already warns about unfilled placeholders, broken in-page anchors, and `svg:` fences with no matching file. Fix anything it reports and re-run. Re-render after every edit to the markdown. The markdown is the source; the HTML is a build artifact, and the two drift the moment you hand-edit the HTML. Open it for the reader rather than starting a server: ```bash code -r .scratchpad/pr-NNNN/review.html ``` Do not start a server. The HTML is self-contained, so the editor's preview or a downloaded copy is enough, and a process the user did not ask for is one they have to hunt down later. If they want HTTP, offer this and let them run it in a VS Code integrated terminal, which is what makes VS Code forward the port: ```bash python3 -m http.server 8112 --bind 127.0.0.1 --directory /abs/path/to/.scratchpad/pr-NNNN ``` Then the URL is `http://127.0.0.1:8112/review.html`, or drop the filename for a directory listing. Point `--directory` at the single document's folder, never at `.scratchpad/` itself: that folder holds credential files and `http.server` serves everything below its root. #### Trust model for the generated HTML The renderer treats the markdown body, the byline derived from it, and any inlined SVG as untrusted, because this skill summarizes GitHub-fetched content into that markdown. Raw HTML in the markdown is disabled, the byline is escaped with an href scheme allowlist, and an SVG carrying a script, an event handler, or an external reference aborts the render. A `