docs: add a contributing guide covering the pre-PR review workflow - #321
Draft
kkozik-amplify wants to merge 3 commits into
Draft
docs: add a contributing guide covering the pre-PR review workflow#321kkozik-amplify wants to merge 3 commits into
kkozik-amplify wants to merge 3 commits into
Conversation
The README pointed contributors at "open a PR" with no mention of how to check their work first. This parser is bidirectional, so a change that looks right in isolation can break round-tripping somewhere else in the pipeline; the guide front-loads that. Documents the intended sequence: get the suite and pre-commit green locally, open a draft PR, verify it with /review-pr, then mark it ready. Draft first because the skill reviews a PR number rather than a working tree, and because it signals no human attention is wanted yet. Also records two things that reliably confuse people: no-commit-to-branch failing by design during --all-files on main, and fork PRs not running CI until a maintainer approves the workflow, which is why the local run carries the weight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two additions. Under Pre-commit Checks, the staged-files invocation and the no-commit-to-branch caveat, both of which cost time to rediscover. Under Keeping Docs Current, CONTRIBUTING.md joins the list of docs that have to move when the workflow does -- a stale command in the contributor entry point is worse than a stale line in CLAUDE.md, since a contributor has no way to know it is wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two problems with the first draft. A bare `/review-pr` means nothing to someone who has never used Claude Code, and the README mentioned it with no indication of what it even was. Worse, it sat as step 4 of a five-step workflow, which read as gating contribution on a specific paid tool. This repo takes PRs from outside contributors; that is not a bar we want. The requirement is now the checklist itself, written so it can be worked by hand. The skill is presented as an automated shortcut for people who happen to have it, with an explicit note that nothing here needs Claude Code and no PR is held up for lacking it. The checklist is drawn from what has actually slipped through on this repo, led by "does your test fail without your source change" -- that one alone has caught a test asserting an unreachable value and another that never exercised the code path it named. Also notes that maintainers may run the skill during review, so a contributor can see up front what it will look for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The README's Contributing section said "open a GitHub issue for anything sizable, then create a pull request" and nothing else. There was no statement of what a contributor should have checked before asking for review.
That matters more here than in most repos. The parser is bidirectional, so a change that looks correct in isolation can quietly break round-tripping in a different module — and fork PRs don't run CI until a maintainer approves the workflow, so for a first-time contributor the local run is often the only signal for a while.
What
Adds
CONTRIBUTING.mdand points the README at it. The workflow it documents:/review-pr <number>and act on the findingsDraft first for two reasons:
/review-prworks from a PR number rather than a working tree, so there has to be a PR; and draft status signals that human attention isn't wanted yet.The guide describes what the skill actually checks (issue reproduction, CLAUDE.md compliance, per-area code review, tests + derived edge cases) and frames it as a first reviewer rather than a verdict — it's good at the mechanical checks and it can also be wrong.
It also records two things that reliably confuse people:
no-commit-to-branchfails duringpre-commit run --all-fileswhenevermainis checked out. That hook exists to block commits tomain, so failing there is intended, not a defect.One correction worth noting
I initially documented
pre-commit run --files $(git diff --name-only origin/main). Running it against this very PR showed it checked onlyREADME.md—git diffdoesn't list untracked files, so the brand-newCONTRIBUTING.mdwas silently skipped. The guide now says togit add -Aand run barepre-commit run, which checks staged files, and explains why the--filesform is a trap.Test plan
python -m unittest discover -s test -p "test_*.py"— 1504 passing (docs-only change, run for completeness)git add -A && pre-commit run— all hooks pass, includingmdformaton both changed filestest-requirements.txtexists as referenced, and that the documented pre-commit invocation covers a newly added fileDocs only; no code paths touched.