diff --git a/.claude/skills/port-pr/SKILL.md b/.claude/skills/port-pr/SKILL.md index fa9e67a4a..5622b5a68 100644 --- a/.claude/skills/port-pr/SKILL.md +++ b/.claude/skills/port-pr/SKILL.md @@ -94,8 +94,9 @@ dotnet csharpier format . `local_check.sh` restores, checks CSharpier formatting, builds Release, and tests; `--agent-strict` adds the blocking comment-hygiene scan. Fix any failure before -proceeding. Report the test results plainly (pass/fail counts); don't claim success if -anything failed. +proceeding, then report its last line, `local_check: PASSED` or +`local_check: FAILED ()`. A test count is not a pass: the build or CSharpier 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 4d9e1e491..d439f32ee 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -69,6 +69,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.py` code it ports or matches +is an **upstream** defect, not a finding: it gets its own fix, ported between +the repos, apart from this PR. Confirm it in the Python 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 @@ -79,6 +84,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. + ``` Minor: F1. Ordinal comparison missing: `marker.IndexOf(":")` is culture-sensitive, so tr-TR splits this marker differently. Pass @@ -126,6 +135,9 @@ confirm `Unverified`; an unverified concern is never Critical. - Reproduce before you report. The environment can build and test: 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. +- Build, format, and test status come from the CI build: cite + `gh pr checks `, and call a check still running pending. The review leaves + `local_check.sh` to CI and does not count it as unverified. ## 5. Close with five lines @@ -135,9 +147,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, target framework, package, or parity contract with `sillsdev/machine.py` changed, or `None verified`. +When there are upstream defects, add one line, `Upstream in sillsdev/machine.py, +not this PR:`, naming each with its Python 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 it applies. Of 140 review threads @@ -157,4 +174,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 ff6997b32..e100e6f27 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -11,10 +11,49 @@ concurrency: cancel-in-progress: true jobs: + # A rebase or a merge from master 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-22.04 + permissions: + contents: read + pull-requests: read + issues: read + outputs: + review: ${{ steps.compare.outputs.review }} + steps: + - name: Checkout repository + uses: actions/checkout@v6 + 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 # Fork PRs get no secrets, so only same-repo branches are reviewed. That also keeps the # restore from running a non-member's MSBuild targets, and keeps the PR's AGENTS.md trusted. - 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-22.04 permissions: @@ -22,6 +61,7 @@ jobs: pull-requests: read issues: read id-token: write + actions: read # Lets the reviewer cite CI results from gh pr checks steps: - name: Checkout repository @@ -65,6 +105,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 decides what gets posted. prompt: '/pr-review ${{ github.event.pull_request.number }}' @@ -72,6 +114,21 @@ jobs: # knows, so review 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(dotnet build:*),Bash(dotnet test:*)" + --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(dotnet build:*),Bash(dotnet test:*)" + --append-system-prompt "Allowed commands match by prefix, so run dotnet build and dotnet test 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 96006e8ef..30fc2da89 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -91,7 +91,7 @@ jobs: claude_args: >- --add-dir ../machine.py --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.py show:*),Bash(git -C ../machine.py cat-file:*),Bash(git -C ../machine.py log:*),Bash(git -C ../machine.py blame:*),Bash(dotnet build:*),Bash(dotnet test:*),Bash(dotnet csharpier:*),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 master --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 restored, and you work on the branch this action created: where a skill such as port-pr says to restore, 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 dotnet 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 master --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. 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 test count in its place; if it did not run to that line, say it did not run. Dependencies are already restored, and you work on the branch this action created: where a skill such as port-pr says to restore, 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 dotnet 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 @@ -108,3 +108,10 @@ jobs: | "\(.tool_name): \((.tool_input.command? // .tool_input.file_path? // .tool_input.url? // .tool_input.pattern? // "") | @json | .[:300])"' "$EXECUTION_FILE" + + # In machine.py, 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 a 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/.github/workflows/create-porting-issue.yml b/.github/workflows/create-porting-issue.yml index 87363fd9a..8545309a3 100644 --- a/.github/workflows/create-porting-issue.yml +++ b/.github/workflows/create-porting-issue.yml @@ -10,10 +10,12 @@ permissions: jobs: create-port-issue: - # Porting is the default. The no porting label opts a PR out. + # Porting is the default. The no porting label opts a PR out, and so does Hermit Crab: + # machine.py has no HermitCrab to port to. if: | github.event.pull_request.merged == true && - !contains(github.event.pull_request.labels.*.name, 'no porting') + !contains(github.event.pull_request.labels.*.name, 'no porting') && + !contains(github.event.pull_request.labels.*.name, 'Hermit Crab') runs-on: ubuntu-latest steps: diff --git a/local_check.sh b/local_check.sh index 2b9717dad..e80ed7ff3 100755 --- a/local_check.sh +++ b/local_check.sh @@ -26,14 +26,18 @@ if [ "$agent_strict" = true ]; then fi fi -dotnet tool restore -dotnet restore -dotnet csharpier check . -if [ $? -ne 0 ]; then - exit 1 -fi -dotnet build --no-restore -c Release -if [ $? -ne 0 ]; then +# Every step runs even after one fails, and the last line names the failures, so +# output read through tail cannot pass off a build failure as a test pass. +failed=() + +dotnet tool restore || failed+=(tool-restore) +dotnet restore || failed+=(restore) +dotnet csharpier check . || failed+=(csharpier) +dotnet build --no-restore -c Release || failed+=(build) +dotnet test --verbosity normal || failed+=(test) + +if [ "${#failed[@]}" -ne 0 ]; then + echo "local_check: FAILED (${failed[*]})" exit 1 fi -dotnet test --verbosity normal +echo "local_check: PASSED"