Skip to content

docs(review): Consolidate PR review guidance across skill and guides#17696

Open
kKPulla wants to merge 1 commit into
mainfrom
consolidate-pr-review-guidance
Open

docs(review): Consolidate PR review guidance across skill and guides#17696
kKPulla wants to merge 1 commit into
mainfrom
consolidate-pr-review-guidance

Conversation

@kKPulla
Copy link
Copy Markdown
Contributor

@kKPulla kKPulla commented Jun 2, 2026

Summary

  • Make the /pr-review skill the single orchestrator for PR reviews, loading human-readable guides (REVIEW_GUIDE.md, FUNCTION_PR_GUIDE.md) as content rather than duplicating them.
  • Rewrite the skill as a staged pipeline (detect mode, fetch, load content, analyze, draft, approve, post) that works consistently across local CLI and CI invocations.
  • Fold three net-new principles into REVIEW_GUIDE.md (assume competence, permission to be quiet, focus on what CI cannot check) and move local-mode drafts from /tmp/ to ~/.claude/review-drafts/.

Test plan

  • Verified the skill runs end-to-end on a live PR (/pr-review https://github.com/facebookincubator/velox/pull/17665) — fetched via fetch.py, loaded CODING_STYLE.md + REVIEW_GUIDE.md, drafted to ~/.claude/review-drafts/pr-17665-r1-v1.md.
  • CI passes (markdown-only change, no build impact).

The `/pr-review` skill and `scripts/review/REVIEW_GUIDE.md` previously carried overlapping and occasionally contradictory guidance — different output formats, different fetch instructions, different sets of principles. A reviewer using the skill on a function PR also missed `FUNCTION_PR_GUIDE.md` entirely because the skill never referenced it. Make the skill the single point of entry: it orchestrates the review and loads the human-readable guides for content, so AI and human reviews stay consistent.

The skill is now a staged pipeline: detect mode, fetch, load content, analyze, draft, approve, post. Substantive review content (rigor, tone, what to check) moves out of the skill; the skill now points to `REVIEW_GUIDE.md` as the source of truth. `FUNCTION_PR_GUIDE.md` loads automatically when the diff touches `velox/functions/`, with a subtree guard so the function checklist doesn't apply to tests/benchmarks/fuzzer-only PRs. Three net-new principles fold into `REVIEW_GUIDE.md`: assume competence, permission to be quiet, focus on what CI cannot check.

Local-mode drafts move from `/tmp/` to `~/.claude/review-drafts/pr-NNNNN-rN-vN.md` to match `REVIEW_GUIDE.md`'s existing procedure. The skill creates the directory on first use. `rN` is the re-review round; `vN` is the iteration within a round.
@netlify
Copy link
Copy Markdown

netlify Bot commented Jun 2, 2026

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 0742e62
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/6a1e82f07cb9330008f2cec4

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 2, 2026
@kKPulla kKPulla requested review from kgpai and mbasmanova June 3, 2026 14:17
@pratikpugalia
Copy link
Copy Markdown
Contributor

pratikpugalia commented Jun 3, 2026

I meant to streamline this too. Looks neat! Thanks @kKPulla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants