Skip to content

Port Claude workflow and skill updates from machine.py #535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ddaspit wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Port Claude workflow and skill updates from machine.py #535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 120 additions & 0 deletions .claude/skills/port-pr/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 <ISSUE> --json title,body,labels
```

- The body looks like: `Port any relevant changes in https://github.com/sillsdev/machine.py/pull/<PR> from machine.py to machine.`
Extract `<PR>` - 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 '<Title>'`. 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.
120 changes: 105 additions & 15 deletions .claude/skills/pr-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,119 @@
---
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
---

# 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
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
Expand All @@ -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`.
77 changes: 77 additions & 0 deletions .github/workflows/claude-code-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Loading
Loading