diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index b7041d3f..dca0f646 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -8,13 +8,58 @@ user-invocable: true # Writing a machine.py review Post one short comment per finding, anchored on the line it is about, then one -summary comment. A review is read-only: do not edit, commit, push, or resolve -threads. +summary comment. A review leaves the code alone: do not edit, commit, or push. +The only thread it resolves is its own finding, once addressed or withdrawn. Unless verified findings are already in hand, get them first with `/code-review high `, without `--comment`: it finds and verifies, and this skill decides what gets posted. +## Start from the last round + +A PR is reviewed again on every push. Each round is a follow-up: settle the +open findings first, then add only what is new. + +1. Load the review threads, which carry the resolved state REST lacks: + + ``` + gh api graphql -F owner={owner} -F repo={repo} -F n= -f query=' + query($owner: String!, $repo: String!, $n: Int!) { + repository(owner: $owner, name: $repo) { pullRequest(number: $n) { + reviewThreads(first: 100) { nodes { id isResolved path line + comments(first: 50) { nodes { databaseId author { login } body } } } } } } }' + ``` + + Load summaries from + `gh api --paginate repos/{owner}/{repo}/issues//comments`. An earlier + finding is a thread whose first comment's author is `claude`, as GraphQL + spells `claude[bot]`, numbered `F` or not. It is open while unresolved, + even when `line` is null because the diff moved past it. The latest summary + names the commit it reviewed. +2. Check every open finding against the head commit and reply in its thread, + using its first comment's `databaseId`, with + `gh api repos/{owner}/{repo}/pulls//comments//replies -f body=...`: + - Fixed: `F3 addressed in :` and what fixed it, then resolve the + thread by its `id`: + `gh api graphql -F id= -f query='mutation($id: ID!) { resolveReviewThread(input: {threadId: $id}) { thread { isResolved } } }'` + - Author gave a reason: weigh it. If it holds, `F3 withdrawn:` and why, then + resolve the thread. If not, answer once with evidence; a point already + answered stays answered. + - Author chose to keep it, e.g. deferred to an issue: record it as accepted. + - Still applies and its code changed: `F3 still applies at :` and why. + - Still applies and its code is untouched: stay silent; the summary counts + it. +3. Treat each finding `/code-review` returns as a duplicate when any thread, + open or resolved, from anyone, already raises the same defect, even if the + line has moved. Drop duplicates: a resolved thread is a settled one. +4. Post a new finding when it is on code changed since the reviewed commit, or + when it is Critical. Diff with `git diff ` if + `gh api repos/{owner}/{repo}/compare/... --jq .status` + prints `ahead`; otherwise history was rewritten, so treat the whole PR as + changed. Number new findings on from the highest `F` in the thread. + +Every earlier finding has a status when this is done. + ## 1. One finding, one comment Anchor it on the line. Two problems on one line are two comments. A reviewer @@ -87,8 +132,14 @@ confirm `Unverified`; an unverified concern is never Critical. Say which public API, optional dependency, published-wheel surface, or parity contract with `sillsdev/machine` changed, or `None verified`. -Then mark each finding, by number, **changed**, **accepted**, or -**unverified**. Leave nothing implicit: a thread with no follow-up leaves nobody +Then mark every finding in the thread, earlier rounds included, by number: +**new**, **open**, **addressed**, **accepted** (the author keeps it knowingly), +or **withdrawn**, adding **unverified** where +it applies. Leave nothing implicit: a thread with no follow-up leaves nobody able to tell which findings mattered. +End with `Reviewed at `, the PR head from +`gh pr view --json headRefOid`, not the merge commit checked out, so the +next round knows where this one stopped. + For an adversarial second pass, apply `docs/review/devils-advocate.md`. diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 92a9884f..b0cd2712 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -10,6 +10,12 @@ on: # - "src/**/*.js" # - "src/**/*.jsx" +# Parallel reviews of one PR cannot see each other's comments and post duplicates. +# A cancelled run may leave a partial round; the next one picks up its threads. +concurrency: + group: claude-review-${{ github.event.pull_request.number }} + cancel-in-progress: true + env: PYTHON_VERSION: "3.12" diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index 580e80a9..3fe1ebf6 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -10,13 +10,23 @@ on: pull_request_review: types: [submitted] +env: + PYTHON_VERSION: "3.12" + jobs: claude: + # Skips outsiders before the install; the action still checks write access. The issue or PR + # body reaches the prompt whoever wrote it, and an outsider's fork PR would run its code. if: | - (github.event_name == 'issue_comment' && contains(github.event.comment.body, '@claude')) || + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), + github.event.comment.author_association || github.event.review.author_association || + github.event.issue.author_association) && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), + github.event.issue.author_association || github.event.pull_request.author_association) && + ((github.event_name == 'issue_comment' && contains(github.event.comment.body, '@claude')) || (github.event_name == 'pull_request_review_comment' && contains(github.event.comment.body, '@claude')) || (github.event_name == 'pull_request_review' && contains(github.event.review.body, '@claude')) || - (github.event_name == 'issues' && (contains(github.event.issue.body, '@claude') || contains(github.event.issue.title, '@claude'))) + (github.event_name == 'issues' && (contains(github.event.issue.body, '@claude') || contains(github.event.issue.title, '@claude')))) runs-on: ubuntu-latest permissions: contents: read @@ -28,7 +38,31 @@ jobs: - name: Checkout repository uses: actions/checkout@v7 with: - fetch-depth: 1 + # Full history so git log and blame can find the commits AGENTS.md cites. + fetch-depth: 0 + + - name: Set up Python ${{ env.PYTHON_VERSION }} + uses: actions/setup-python@v6 + with: + python-version: ${{ env.PYTHON_VERSION }} + + - name: Install Poetry + uses: snok/install-poetry@v1 + with: + version: 2.4.1 + virtualenvs-create: true + virtualenvs-in-project: true + installer-parallel: true + + - name: Restore virtualenv + uses: actions/cache@v4 + with: + path: .venv + key: claude-venv-${{ runner.os }}-py${{ env.PYTHON_VERSION }}-${{ hashFiles('poetry.lock') }} + + # A change made from an issue is validated before its pull request is opened. + - name: Install dependencies + run: poetry install --no-interaction --all-extras - name: Run Claude Code id: claude @@ -40,11 +74,10 @@ jobs: additional_permissions: | actions: read - # Optional: Give a custom prompt to Claude. If this is not specified, Claude will perform the instructions specified in the comment that tagged it. - # prompt: 'Update the pull request description to include a summary of changes.' - - # Optional: Add claude_args to customize behavior and configuration + # The action's own prompt ends an issue run with a "Create a PR" link; the + # appended prompt has Claude open the pull request itself. + claude_args: >- + --allowedTools "Bash(gh pr create:*),Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh issue view:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*),Bash(git diff:*),Bash(git status:*),Bash(poetry run:*),Bash(./local_check.sh:*)" + --append-system-prompt "When invoked on an issue and you change code, open the pull request yourself once your commits are pushed: gh pr create --base main --head . Post its link in your comment in place of a Create a PR link. Write the title and body with the pr-authoring skill, and put Closes # in the body. Before opening it, run ./local_check.sh --agent-strict and report its result in the body. Dependencies are already installed, and you work on the branch this action created: where a skill such as port-pr says to install, create or switch branches, or push, commit on the current branch and push it the way these instructions say." # See https://github.com/anthropics/claude-code-action/blob/main/docs/usage.md # or https://code.claude.com/docs/en/cli-reference for available options - # claude_args: '--allowed-tools Bash(gh pr *)' -