From 280d81fe485723660eee23762ad6022a67062b77 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Tue, 20 Oct 2026 14:41:54 +0000 Subject: [PATCH 1/4] Make local_check.sh fail on any step local_check.sh now runs every step, even after one fails. The last line names the steps that failed, so a build failure cannot hide behind a passing test run. Co-authored-by: Eli C. Lowry <83078660+Enkidu93@users.noreply.github.com> Co-Authored-By: Claude Sonnet 5.5 --- local_check.sh | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) 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" From 4a31736f3aa6c5a145589a52351292bb6711b144 Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 20 Oct 2026 16:56:38 -0400 Subject: [PATCH 2/4] Port the Claude review and skill changes from machine.py This ports the rest of sillsdev/machine.py#409 for #544. The review now skips a push when the diff matches the last reviewed commit. It cites CI results from gh pr checks, and it posts a one-line summary when a round has no new findings. Agents now report the last line of local_check.sh instead of a test count. I did not port the rule for upstream defects. In machine.py, it skips defects that are shared with the C# code. In this repo, the C# code is the reference. Co-Authored-By: Claude Opus 5.5 --- .claude/skills/port-pr/SKILL.md | 5 +- .claude/skills/pr-review/SKILL.md | 14 ++++++ .github/workflows/claude-code-review.yml | 61 +++++++++++++++++++++++- .github/workflows/claude.yml | 9 +++- 4 files changed, 84 insertions(+), 5 deletions(-) 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..c0d8e20b3 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -79,6 +79,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 +130,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,6 +142,8 @@ 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`. @@ -157,4 +166,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" From 75e401c6e0f64de913c150ea84a8bbb2bd77e528 Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 20 Oct 2026 16:56:38 -0400 Subject: [PATCH 3/4] Skip porting issues for PRs labeled Hermit Crab machine.py does not have HermitCrab, so these PRs have nothing to port. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/create-porting-issue.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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: From b2955b54d684e5178ba917be58261f72527204f8 Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 20 Oct 2026 17:03:29 -0400 Subject: [PATCH 4/4] Port the upstream defect rule to the review skill A defect that a PR shares with the machine.py code it ports or matches gets its own fix. The review names it in the summary and does not post it as a finding. Changes can start in either repo, so the rule applies here too. Co-Authored-By: Claude Opus 5.5 --- .claude/skills/pr-review/SKILL.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index c0d8e20b3..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 @@ -147,6 +152,9 @@ 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