From c635615adf92532ea6174326859e757cbf791b0e Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Fri, 2 Oct 2026 19:05:37 -0400 Subject: [PATCH] Port Claude workflow and skill updates from machine.py This adds the Claude Code workflows from machine.py. The @claude workflow can now open PRs from issues, and a porting issue starts a port with the new port-pr skill. The code review runs on every push and follows up on its earlier findings instead of posting them again. A merged PR now gets a porting issue unless it has the no porting label. Ports sillsdev/machine.py#377, #384, #389, #391, #392, #394 and #393. Co-Authored-By: Claude Opus 5.5 --- .claude/skills/port-pr/SKILL.md | 120 +++++++++++++++++++++ .claude/skills/pr-review/SKILL.md | 120 ++++++++++++++++++--- .github/workflows/claude-code-review.yml | 77 +++++++++++++ .github/workflows/claude.yml | 96 +++++++++++++++++ .github/workflows/create-porting-issue.yml | 7 +- 5 files changed, 403 insertions(+), 17 deletions(-) create mode 100644 .claude/skills/port-pr/SKILL.md create mode 100644 .github/workflows/claude-code-review.yml create mode 100644 .github/workflows/claude.yml diff --git a/.claude/skills/port-pr/SKILL.md b/.claude/skills/port-pr/SKILL.md new file mode 100644 index 000000000..fa9e67a4a --- /dev/null +++ b/.claude/skills/port-pr/SKILL.md @@ -0,0 +1,120 @@ +--- +name: port-pr +description: Port a machine.py (Python) PR into machine (C#). Given a GitHub issue number for a porting task in sillsdev/machine, finds the linked machine.py PR, ports its changes to the C# codebase, runs ./local_check.sh, and opens a PR that closes the issue. Use when asked to port a PR/issue from machine.py, complete a "porting" issue, or sync a machine.py change into machine. +--- + +# Port a machine.py (Python) PR into machine (C#) + +`machine` (C#) and `machine.py` (Python) are direct, intentionally-synced ports of each +other. This skill ports a change that already landed in `machine.py` into `machine`, +driven by a "porting" issue in `sillsdev/machine`. + +**Required argument:** the GitHub issue number in `sillsdev/machine` (always exists). + +## Repos + +- C# target repo: the current working directory (`sillsdev/machine`). +- Python source repo: the sibling clone at `../machine.py` (`sillsdev/machine.py`). + Use the local clone for reading surrounding context; use `gh ... --repo sillsdev/machine.py` + for authoritative PR data. + +## Step 1 - Read the porting issue + +```bash +gh issue view --json title,body,labels +``` + +- The body looks like: `Port any relevant changes in https://github.com/sillsdev/machine.py/pull/ from machine.py to machine.` + Extract `` - the machine.py PR number - from that URL. An issue filed with the + porting form carries the URL in its `machine.py source PR or commit` field instead. +- The issue title looks like `Port ''`. Keep `<Title>` for the branch and PR. +- If the body has no machine.py PR link, stop and ask the user for the source PR. + +## Step 2 - Understand the source change + +```bash +gh pr view <PR> --repo sillsdev/machine.py --json title,body,files,commits,mergeCommit +gh pr diff <PR> --repo sillsdev/machine.py +``` + +Read the full diff. For each changed Python file, read the corresponding file(s) in +`../machine.py` as of the merge commit, with `git -C ../machine.py show <mergeCommit>:<path>`, +to understand the surrounding context. The working tree may have moved on since the PR +merged. First check the commit with `git -C ../machine.py cat-file -e <mergeCommit>`. If it +fails, the clone predates the merge: run `git -C ../machine.py fetch`. Identify the C# +counterpart (see mapping below). Read the existing C# code you're about to change so +the port matches local idiom. + +Note: not every change ports. Skip Python-only concerns (`pyproject.toml`/`poetry.lock` +dependencies, `__init__.py` re-exports, black/flake8/isort/pyright configuration, PyPI +packaging, and `machine/jobs`, which has no C# counterpart here). The issue says "any +*relevant* changes" - use judgment and call out anything you intentionally skip. + +## Step 3 - File & API mapping + +| machine.py (Python) | machine (C#) | +|---|---| +| `machine/<area>/<snake_case>.py` | `src/SIL.Machine/<PascalArea>/<PascalCase>.cs` (or the matching `SIL.Machine.*` project) | +| `tests/<area>/test_<snake_case>.py` | `tests/SIL.Machine.Tests/<PascalArea>/<PascalCase>Tests.cs` (or the matching `*.Tests` project) | +| `snake_case` functions/vars | `PascalCase` methods / `camelCase` locals / `_camelCase` fields | +| `Sequence[T]`/`list` / `Mapping`/`dict` / `Set`/`set` etc. | `IReadOnlyList<T>` / `IReadOnlyDictionary<,>` / `IReadOnlyCollection<T>` etc. - match the neighbors | +| pytest plain `assert` | NUnit `Assert.That(...)` (check neighboring test files) | +| `pyproject.toml` `version` | `src/AssemblyInfo.props` `<Version>` | + +Python modules usually map one-to-one onto a folder of `src/SIL.Machine` (`corpora` -> +`Corpora`, `punctuation_analysis` -> `PunctuationAnalysis`, and so on), but a module may +hold several types that C# splits into one file each. Find the C# counterpart by searching +for the type/method name (translated to PascalCase) before assuming a path: +`grep -rn "<TypeOrMethodName>" src tests`. + +Port the **behavior**, not the syntax. Match existing C# patterns in the neighboring +code, and apply the `AGENTS.md` rules Python never makes you think about: ordinal string +comparison for markers, tokens, and identifiers; disposal and stream ownership; and +`netstandard2.0` compatibility for the published libraries. Port the tests too. + +## Step 4 - Branch & apply + +Create a branch off `master` (do not commit to `master`): + +```bash +git switch master && git pull && git switch -c port-<slug> +``` + +where `<slug>` is a short kebab-case form of the issue title (e.g. +`port-fix-unclosed-style-marker-crash`). + +Apply the ported changes with Edit/Write. + +## Step 5 - Verify locally + +```bash +dotnet csharpier format . +./local_check.sh --agent-strict +``` + +`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. + +## Step 6 - Commit, push, open PR (pause first) + +Show the user a summary of the diff and the proposed PR title/body, and **confirm before +pushing**. Then commit with a message following the commit-messages skill +(`Port '<Title>' from machine.py PR #<PR>`), push, and open the PR: + +```bash +git push -u origin port-<slug> +gh pr create --title "Port '<Title>' from machine.py" --body-file <file> +``` + +Write the body with the pr-authoring skill. Link the source PR in its porting +context section, name anything deliberately not ported, and end with +`Closes #<ISSUE>`. + +## Notes + +- Keep the two codebases as similar as is reasonable for a C#-vs-Python port. +- If the source PR spans multiple commits, the squashed PR diff is the source of truth, but + reading individual commits can clarify intent. +- If a change has no sensible C# counterpart, say so in the PR body rather than forcing it. diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 3701b384c..4d9e1e491 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -1,6 +1,6 @@ --- name: pr-review -description: How to write up a code review in sillsdev/machine - short line comments, evidence, severity. +description: Review a pull request in sillsdev/machine - find and verify with code-review, then post short numbered line comments with evidence and severity. argument-hint: "[optional PR number, branch, or review focus]" user-invocable: true --- @@ -8,24 +8,81 @@ user-invocable: true # Writing a machine 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 threads it resolves are its own findings, once addressed, withdrawn, +or, unless Critical, rejected or deferred by the author. + +Unless verified findings are already in hand, get them first with +`/code-review high <target>`, without `--comment`: it finds and verifies, and +this skill decides what gets posted. What to look for is in `docs/review/`; `AGENTS.md` maps a changed path to its rules file. +## 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=<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/<n>/comments`. An earlier + finding is a thread whose first comment's author is `claude`, as GraphQL + spells `claude[bot]`, numbered `F<n>` 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/<n>/comments/<id>/replies -f body=...`: + - Fixed: `F3 addressed in <sha>:` and what fixed it, then resolve the + thread by its `id`: + `gh api graphql -F id=<id> -f query='mutation($id: ID!) { resolveReviewThread(input: {threadId: $id}) { thread { isResolved } } }'` + - Author rejected or deferred it: weigh any reason given. If it holds, + `F3 withdrawn:` and why, then resolve the thread. If not, `F3 accepted:` + with your evidence or where it was deferred, once, then resolve it. An + accepted Critical finding stays open for a maintainer to dismiss. + - Still applies and its code changed: `F3 still applies at <sha>:` 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 <reviewed-sha> <head-sha>` if + `gh api repos/{owner}/{repo}/compare/<reviewed-sha>...<head-sha> --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 scrolling the diff should meet each point where it applies. +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 +withdrawn finding keeps its number, and a later round continues the sequence. + ## 2. Lead with the claim -First sentence names the defect. Evidence second, fix third, if it fits. +After the keyword and number, the first sentence names the defect. Evidence +second, fix third, if it fits. ``` -Ordinal comparison missing: `marker.IndexOf(":")` is culture-sensitive, so -tr-TR splits this marker differently. Pass `StringComparison.Ordinal`. +Minor: F1. Ordinal comparison missing: `marker.IndexOf(":")` is +culture-sensitive, so tr-TR splits this marker differently. Pass +`StringComparison.Ordinal`. ``` Three lines is long. A finding needing more is a design question - raise it in @@ -33,14 +90,30 @@ the summary instead. ## 3. Label the severity -**Critical** blocks merge, then **Important**, then **Minor**. Critical means +Every finding is **Critical**, **Important**, or **Low**. Critical means demonstrated: a failing command, a broken contract, a missing gate. A worry is -not Critical. +not Critical. Important needs an answer from the author; Low is worth knowing +and needs none. + +A finding comment posted to the PR starts with the Reviewable keyword for its +severity, followed by a colon. Reviewable reads it and sets the discussion's +disposition: + +| Severity | Comment starts with | Disposition in Reviewable | +| --- | --- | --- | +| Critical | `Major:` | Blocking, until a maintainer dismisses it | +| Important | `Minor:` | Discussing, open until the author answers | +| Low | `FYI:` | Informing, starts resolved | + +Use the keywords there and nowhere else. The summary, replies, and a review +that is not posted use the severity names: `Minor` reads as trivial, and an +Important finding is not. A finding comment that starts with any other word gets +Reviewable's default, which for a reviewer is Blocking. ## 4. Carry the evidence Every comment gets a `path:line` and a consequence. Mark what you did not -confirm `Unverified`; an unverified concern never blocks a merge. +confirm `Unverified`; an unverified concern is never Critical. - Do not report pre-existing issues the diff does not touch. - Do not ask for a migration, modernization, or benchmark the diff gave no @@ -50,21 +123,38 @@ confirm `Unverified`; an unverified concern never blocks a merge. - Name the commands you ran and what they returned. `./local_check.sh` is the full local sequence; an agent-authored branch also needs `--agent-strict`, and a green advisory `Comment hygiene` check does not stand in for it. +- 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. ## 5. Close with five lines 1. Verdict: approve, approve with fixes, or request changes. -2. The one thing that matters most, with its `path:line`. +2. The one thing that matters most, with its number and `path:line`. 3. Counts by severity. 4. What you ran, and its result. 5. What you could not verify. -Say which public API, target framework, package, or parity contract changed, or -`None verified`. +Say which public API, target framework, package, or parity contract with +`sillsdev/machine.py` changed, or `None verified`. -Then mark each finding **changed**, **accepted**, or **unverified**. Of 140 -review threads here in three years, 128 have no follow-up, so nobody can tell -which findings mattered. Leave nothing implicit. +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 +here in three years, 128 have no follow-up, so nobody can tell which findings +mattered. Leave nothing implicit. + +End with `Reviewed at <head-sha>`, the PR head from +`gh pr view <n> --json headRefOid`, not the merge commit checked out, so the +next round knows where this one stopped. + +Once the new summary is posted, minimize each earlier one as outdated, so only +the latest shows. An earlier summary is a top-level comment by `claude[bot]` +containing `Reviewed at`; leave its other comments, such as replies to an +`@claude` mention, visible. Take its `node_id` from the comments listing: + +``` +gh api graphql -F id=<node_id> -f query='mutation($id: ID!) { minimizeComment(input: {subjectId: $id, classifier: OUTDATED}) { minimizedComment { isMinimized } } }' +``` 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 new file mode 100644 index 000000000..ff6997b32 --- /dev/null +++ b/.github/workflows/claude-code-review.yml @@ -0,0 +1,77 @@ +name: Claude Code Review + +on: + pull_request: + types: [opened, synchronize, ready_for_review, reopened] + +# 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 + +jobs: + claude-review: + # 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 + + runs-on: ubuntu-22.04 + permissions: + contents: read + pull-requests: read + issues: read + id-token: write + + steps: + - name: Checkout repository + uses: actions/checkout@v6 + with: + # Full history so the reviewer can read git log and blame, and diff against + # the base branch locally rather than only seeing the PR's final state. + fetch-depth: 0 + + - uses: lukka/get-cmake@v4.2.3 + + - name: Setup .NET + uses: actions/setup-dotnet@v5 + with: + dotnet-version: | + 10.0.x + + - name: Install libgoogle-perftools-dev + run: | + sudo apt-get update + sudo apt-get install -y libunwind-dev + sudo apt-get install -y libgoogle-perftools-dev + + # Lets the reviewer build and run tests to falsify its own findings. It is not here + # to gate the PR -- the CI build already does that. + - name: sentencepiece4c build + run: | + cmake -S ${{ github.workspace }}/src/sentencepiece4c -B ${{ github.workspace }}/src/sentencepiece4c/build -G Ninja -DCMAKE_BUILD_TYPE=Release + cmake --build ${{ github.workspace }}/src/sentencepiece4c/build --config Release --target sentencepiece4c + + - name: Restore dependencies + run: | + dotnet tool restore + dotnet restore + + - name: Run Claude Code Review + id: claude-review + uses: anthropics/claude-code-action@v1 + with: + claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + # 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 + # 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 }}' + # v1 has no `model` input; the alias follows the newest Opus the action's CLI + # 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:*)" + # 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 diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml new file mode 100644 index 000000000..2308f81d2 --- /dev/null +++ b/.github/workflows/claude.yml @@ -0,0 +1,96 @@ +name: Claude Code + +on: + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] + issues: + types: [opened, assigned, labeled] + pull_request_review: + types: [submitted] + +jobs: + claude: + # The issue or PR body reaches the prompt, so its author must be a member too. Claude's + # own PRs pass: the bot's association is NONE, but it wrote the body and pushed the branch. + if: | + 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.issue.user.login || github.event.pull_request.user.login) == 'claude[bot]') && + ((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' && github.event.action != 'labeled' && + !contains(github.event.issue.labels.*.name, 'porting') && (contains(github.event.issue.body, '@claude') || contains(github.event.issue.title, '@claude'))) || + (github.event_name == 'issues' && github.event.action == 'labeled' && github.event.label.name == 'porting')) + runs-on: ubuntu-22.04 + permissions: + contents: read + pull-requests: read + issues: read + id-token: write + actions: read # Required for Claude to read CI results on PRs + steps: + - name: Checkout repository + uses: actions/checkout@v6 + with: + # Full history so git log and blame can find the commits AGENTS.md cites. + fetch-depth: 0 + # A persisted GITHUB_TOKEN header overrides the action's app token, so pushes would go + # out as github-actions[bot] and be denied. + persist-credentials: false + + - uses: lukka/get-cmake@v4.2.3 + + - name: Setup .NET + uses: actions/setup-dotnet@v5 + with: + dotnet-version: | + 10.0.x + + - name: Install libgoogle-perftools-dev + run: | + sudo apt-get update + sudo apt-get install -y libunwind-dev + sudo apt-get install -y libgoogle-perftools-dev + + # A change made from an issue is validated with ./local_check.sh before its PR is opened. + - name: sentencepiece4c build + run: | + cmake -S ${{ github.workspace }}/src/sentencepiece4c -B ${{ github.workspace }}/src/sentencepiece4c/build -G Ninja -DCMAKE_BUILD_TYPE=Release + cmake --build ${{ github.workspace }}/src/sentencepiece4c/build --config Release --target sentencepiece4c + + - name: Restore dependencies + run: | + dotnet tool restore + dotnet restore + + # port-pr reads the Python source as of each source PR's merge commit, so it needs full + # history. Outside the workspace, CSharpier and the solution never see it. + - name: Clone sillsdev/machine.py + run: git clone https://github.com/sillsdev/machine.py.git ../machine.py + + - name: Run Claude Code + id: claude + uses: anthropics/claude-code-action@v1 + with: + claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + # Porting issues are created with this label, so each one starts a port. + label_trigger: porting + + # This is an optional setting that allows Claude to read CI results on PRs + additional_permissions: | + actions: read + + # 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: >- + --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:*)" + --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 <your branch>. 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 #<issue number> 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. 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 diff --git a/.github/workflows/create-porting-issue.yml b/.github/workflows/create-porting-issue.yml index 0722d63e2..87363fd9a 100644 --- a/.github/workflows/create-porting-issue.yml +++ b/.github/workflows/create-porting-issue.yml @@ -10,12 +10,15 @@ permissions: jobs: create-port-issue: - if: github.event.pull_request.merged == true + # Porting is the default. The no porting label opts a PR out. + if: | + github.event.pull_request.merged == true && + !contains(github.event.pull_request.labels.*.name, 'no porting') runs-on: ubuntu-latest steps: - name: Create issue in opposite repository - uses: actions/github-script@v7 + uses: actions/github-script@v9 with: github-token: ${{ secrets.PORTING_API_TOKEN }} script: |