diff --git a/.claude/skills/port-pr/SKILL.md b/.claude/skills/port-pr/SKILL.md index dcd3eb04..ceb47c20 100644 --- a/.claude/skills/port-pr/SKILL.md +++ b/.claude/skills/port-pr/SKILL.md @@ -99,8 +99,9 @@ poetry run pytest ``` `black` and `isort` rewrite files in place; `flake8` and `pyright` are gates that must pass -clean. Fix any formatting, lint, type, or test failures before proceeding. Report the test -results plainly (pass/fail counts); don't claim success if anything failed. +clean. Fix any formatting, lint, type, or test failures before proceeding. Finish with +`./local_check.sh --agent-strict` and report its last line, `local_check: PASSED` or +`local_check: FAILED ()`. A pytest count is not a pass: pyright can fail above it. ## Step 6 — Commit, push, open PR (pause first) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index adb2dd53..6ee6077c 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -66,6 +66,11 @@ Every earlier finding has a status when this is done. Anchor it on the line. Two problems on one line are two comments. A reviewer scrolling the diff should meet each point where it applies. +A defect the PR shares with the `sillsdev/machine` code it ports or matches is +an **upstream** defect, not a finding: it is fixed in C# first and ported +back, apart from this PR. Confirm it in the C# source, post no comment on the +diff, and name it in the summary's upstream line. + Number findings `F1`, `F2`, ... in the order you post them, and put the number right after the keyword, so a reply or a later review can refer to one without quoting it. Not `#1`: GitHub links that to issue 1. Numbers are stable - a @@ -76,6 +81,10 @@ withdrawn finding keeps its number, and a later round continues the sequence. After the keyword and number, the first sentence names the defect. Evidence second, fix third, if it fits. +Write for an author who has not traced the code you traced. The first sentence +says, in plain words, what a caller sees go wrong; then the smallest input, +with what comes out against what should. + ``` Major: F1. Verse range collapses to one row: `_advance_rows` keeps only the last match, so a `\v 1-2` matched by two rows loses the first row's metadata. @@ -121,6 +130,10 @@ confirm `Unverified`; an unverified concern is never Critical. - Reproduce before you report. The environment can run the code: a finding you tried and failed to reproduce is worth more than one you only reasoned about. - A coverage percentage is not evidence that a changed line is tested. +- Lint, type, and test status come from the CI build: cite + `gh pr checks `, and call a check still running pending. `local_check.sh` + rewrites files, so the review leaves it to CI and does not count it as + unverified. ## 5. Close with five lines @@ -130,9 +143,14 @@ confirm `Unverified`; an unverified concern is never Critical. 4. What you ran, and its result. 5. What you could not verify. +Each line is a sentence or two. The candidates you dropped stay out of it. + Say which public API, optional dependency, published-wheel surface, or parity contract with `sillsdev/machine` changed, or `None verified`. +When there are upstream defects, add one line, `Upstream in sillsdev/machine, +not this PR:`, naming each with its C# file and the visible effect. + 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 @@ -152,4 +170,9 @@ containing `Reviewed at`; leave its other comments, such as replies to an gh api graphql -F id= -f query='mutation($id: ID!) { minimizeComment(input: {subjectId: $id, classifier: OUTDATED}) { minimizedComment { isMinimized } } }' ``` +A round with no new finding and no finding whose status changed is quiet. In +place of the five lines its summary is the single line +`No new findings. Reviewed at `, and the earlier summary stays +visible. + 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 534c8bcc..3996071e 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -20,13 +20,55 @@ env: PYTHON_VERSION: "3.12" jobs: + # A rebase or a merge from main that keeps the PR's changes needs no new round. It + # compares with the last summary's commit: a quick rebase can cancel the round before it. + changes: + if: github.event.pull_request.head.repo.full_name == github.repository + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + issues: read + outputs: + review: ${{ steps.compare.outputs.review }} + steps: + - name: Checkout repository + uses: actions/checkout@v7 + with: + fetch-depth: 0 + + - name: Compare with the last reviewed commit + id: compare + env: + GH_TOKEN: ${{ github.token }} + PR: ${{ github.event.pull_request.number }} + BASE: ${{ github.event.pull_request.base.ref }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + reviewed=$(gh api --paginate "repos/$GITHUB_REPOSITORY/issues/$PR/comments" \ + --jq '.[] | select(.user.login == "claude[bot]") | .body' \ + | grep -oE 'Reviewed at [0-9a-f]{40}' | tail -n 1 | cut -d' ' -f3) + pr_patch_id() { + git diff "$(git merge-base "origin/$BASE" "$1")" "$1" | git patch-id --stable + } + if [ -n "$reviewed" ] && git fetch --quiet origin "$reviewed" \ + && [ "$(pr_patch_id "$reviewed")" = "$(pr_patch_id "$HEAD_SHA")" ]; then + echo "The changes at $HEAD_SHA match those reviewed at $reviewed; skipping the review." + echo "review=false" >> "$GITHUB_OUTPUT" + else + echo "review=true" >> "$GITHUB_OUTPUT" + fi + claude-review: + needs: changes # Secrets are not available to workflows triggered by pull requests from forked # repositories, so only review pull requests from branches in this repository. # This also keeps `poetry install` below from running build hooks authored by # someone without push access, and keeps AGENTS.md trusted: CLAUDE.md is # restored from main, but the AGENTS.md it imports comes from the PR head. - if: github.event.pull_request.head.repo.full_name == github.repository + if: >- + github.event.pull_request.head.repo.full_name == github.repository && + needs.changes.outputs.review == 'true' runs-on: ubuntu-latest permissions: @@ -34,6 +76,7 @@ jobs: pull-requests: read issues: read id-token: write + actions: read # Lets the reviewer cite CI results in place of running local_check.sh steps: - name: Checkout repository @@ -75,6 +118,8 @@ jobs: # A push by the @claude workflow makes claude[bot] the actor, which the action # rejects unless allowed. The review never pushes, so it cannot retrigger itself. allowed_bots: claude + additional_permissions: | + actions: read # pr-review has the built-in code-review skill find and verify at "high" # effort, then sets what gets posted. The code-review plugin this replaces # dropped anything under 80 on its confidence rubric. @@ -84,7 +129,21 @@ jobs: # behavior and cost can change with no commit here. claude_args: >- --model opus - --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr comment:*),Bash(gh pr list:*),Bash(gh api:*),Bash(gh search:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*),Bash(git diff:*),Bash(poetry run:*),Bash(.venv/bin/python:*)" + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr comment:*),Bash(gh pr list:*),Bash(gh pr checks:*),Bash(gh api:*),Bash(gh search:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*),Bash(git diff:*),Bash(poetry run:*),Bash(.venv/bin/python:*)" + --append-system-prompt "Allowed commands match by prefix, so run poetry run and .venv/bin/python from the repository root as they are, not after cd or inside a wrapper such as timeout." # 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 + # The action hides Claude's output, so the log does not say which calls were denied. + # Only the command or target is printed, never file content, and @json keeps it on one + # line so a denied command cannot issue a "::" workflow command. + - name: Show denied tool calls + if: always() && steps.claude-review.outputs.execution_file != '' + continue-on-error: true + env: + EXECUTION_FILE: ${{ steps.claude-review.outputs.execution_file }} + run: >- + jq -r '.[] | select(.type == "result") | .permission_denials[]? + | "\(.tool_name): \((.tool_input.command? // .tool_input.file_path? // .tool_input.url? + // .tool_input.pattern? // "") | @json | .[:300])"' + "$EXECUTION_FILE" diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index 6895c9fe..23891cce 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -94,7 +94,7 @@ jobs: claude_args: >- --add-dir ../machine --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(git -C ../machine show:*),Bash(git -C ../machine cat-file:*),Bash(git -C ../machine log:*),Bash(git -C ../machine blame:*),Bash(poetry run:*),Bash(./local_check.sh:*),Bash(tail:*),Bash(head:*)" - --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. Allowed commands match by prefix, so run poetry run and ./local_check.sh from the repository root as they are, not after cd or inside a wrapper such as timeout. When the issue is labeled porting, port it with the port-pr skill." + --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 commit the files black and isort rewrite. Whenever you report local_check, in a PR body or a comment, quote its last line, local_check: PASSED or local_check: FAILED (...), and never a pytest count in its place; if it did not run to that line, say it did not run. 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. Allowed commands match by prefix, so run poetry run and ./local_check.sh from the repository root as they are, not after cd or inside a wrapper such as timeout. When the issue is labeled porting, port it with the port-pr skill." # 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 @@ -111,3 +111,10 @@ jobs: | "\(.tool_name): \((.tool_input.command? // .tool_input.file_path? // .tool_input.url? // .tool_input.pattern? // "") | @json | .[:300])"' "$EXECUTION_FILE" + + # Two Reviewable reviews that began with @claude passed the job filter, yet the action + # found no trigger and skipped. Print what it was given, so the next miss shows why. + - name: Show review body of a skipped mention + if: always() && github.event_name == 'pull_request_review' && steps.claude.outputs.execution_file == '' + continue-on-error: true + run: jq '.review.body // "" | .[:300]' "$GITHUB_EVENT_PATH" diff --git a/local_check.sh b/local_check.sh index c606be14..1308c548 100755 --- a/local_check.sh +++ b/local_check.sh @@ -25,13 +25,23 @@ if [ "$agent_strict" = true ]; then fi fi +# Every step runs even after one fails, and the last line names the failures, so +# output read through tail cannot pass off a pyright failure as a pytest pass. +failed=() + echo "======================= black ======================" -poetry run black . +poetry run black . || failed+=(black) echo "======================= flake8 ======================" -poetry run flake8 . +poetry run flake8 . || failed+=(flake8) echo "======================= isort ======================" -poetry run isort . +poetry run isort . || failed+=(isort) echo "======================= pyright ======================" -poetry run pyright +poetry run pyright || failed+=(pyright) echo "======================= pytest ======================" -poetry run pytest +poetry run pytest || failed+=(pytest) + +if [ "${#failed[@]}" -ne 0 ]; then + echo "local_check: FAILED (${failed[*]})" + exit 1 +fi +echo "local_check: PASSED"