From 0cdd0d07d55547ed93f5779ff97076a9cc328621 Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 13:08:15 -0400 Subject: [PATCH 01/12] Improve the Claude code review workflow Switch from the code-review plugin to the built-in skill at high effort, running on Opus 5. The plugin dropped any finding scoring under 80 on its confidence rubric, which filtered out design and public API issues that don't change current behavior. Install the project dependencies and fetch full history so the reviewer can reproduce a suspected bug instead of only reasoning about it. Add a CLAUDE.md covering what the repo doesn't already record: the C# port relationship, the comment and breaking change conventions, and how to use the environment during review. Also drop EFLOMAL_PATH from the CI build. Nothing has read it since the switch to the native eflomal implementation in #336. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 2 -- .github/workflows/claude-code-review.yml | 45 +++++++++++++++++++++--- CLAUDE.md | 29 +++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) create mode 100644 CLAUDE.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6b6c751b..8abf7b8e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,8 +49,6 @@ jobs: run: poetry run pyright - name: Test with pytest run: poetry run pytest --cov --cov-report=xml - env: - EFLOMAL_PATH: /home/runner/work/machine.py/machine.py/.venv/lib/python${{ matrix.python-version }}/site-packages/eflomal/bin - name: Upload coverage reports to Codecov uses: codecov/codecov-action@v7 env: diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 771943ba..90a876c9 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -10,10 +10,15 @@ on: # - "src/**/*.js" # - "src/**/*.jsx" +env: + PYTHON_VERSION: "3.12" + jobs: claude-review: # 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. if: github.event.pull_request.head.repo.full_name == github.repository runs-on: ubuntu-latest @@ -27,17 +32,47 @@ jobs: - name: Checkout repository uses: actions/checkout@v7 with: - fetch-depth: 1 + # 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 + + - name: Set up Python ${{ env.PYTHON_VERSION }} + uses: actions/setup-python@v6 + with: + python-version: ${{ env.PYTHON_VERSION }} + + - name: Install Poetry + uses: snok/install-poetry@v1 + with: + version: 2.4.1 + virtualenvs-create: true + virtualenvs-in-project: true + installer-parallel: true + + - name: Restore virtualenv + uses: actions/cache@v4 + with: + path: .venv + key: review-venv-${{ runner.os }}-py${{ env.PYTHON_VERSION }}-${{ hashFiles('poetry.lock') }} + + # Gives the reviewer an interpreter to falsify its own findings with. It is not + # here to lint or run the suite -- the CI build already does both. + - name: Install dependencies + run: poetry install --no-interaction --all-extras - name: Run Claude Code Review id: claude-review uses: anthropics/claude-code-action@v1 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - plugin_marketplaces: 'https://github.com/anthropics/claude-code.git' - plugins: 'code-review@claude-code-plugins' - prompt: '/code-review:code-review --comment ${{ github.repository }}/pull/${{ github.event.pull_request.number }}' - claude_args: '--allowedTools "mcp__github_inline_comment__create_inline_comment"' + # The built-in code-review skill, at "high" effort. The code-review plugin + # this replaces discarded anything scoring under 80 on its confidence rubric, + # which filtered out design and public-API findings that don't change behavior. + prompt: '/code-review high --comment ${{ github.event.pull_request.number }}' + # v1 has no `model` input; the model is passed through to the CLI instead. + claude_args: >- + --model claude-opus-5 + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr list:*),Bash(gh api:*),Bash(gh search:*),Bash(poetry run:*),Bash(python:*)" # 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/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..9b71e4ad --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,29 @@ +# machine.py + +Much of this library is a Python port of [sillsdev/machine](https://github.com/sillsdev/machine) (C#). +When changing code that exists on both sides, check what the C# implementation does first — divergence +should be deliberate, not incidental. + +## Comments + +Follow the convention the surrounding code already establishes. + +A comment that no longer describes the code below it is a defect. When changing logic, update or +delete the comments that explain it. + +## Breaking changes + +The names a subpackage re-exports are what downstream packages import from the published wheel, so +changing an exported signature or its semantics is a breaking change even when every caller inside +this repo still works. Say so in the PR description. + +Watch for changes that fail silently rather than loudly — a parameter widened from `dict` to an +iterable of dicts will accept a positional `dict` and iterate its keys. + +## Reviewing changes + +CI covers lint, type and test breakage on every push, so re-running the suite during review adds +nothing. Use the environment for what CI can't do: check whether a suspected bug actually manifests. +Write a throwaway script or run one targeted test file, and compare against the base branch when the +question is whether behavior changed. A finding you tried and failed to reproduce is worth more than +one you only reasoned about. From c75907cf1981db08883198fd30a466fe37e25ec3 Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 13:33:18 -0400 Subject: [PATCH 02/12] Fix the reviewer's tool permissions The allowlist named `python`, which on the runner is the setup-python interpreter without the project dependencies, not the Poetry venv. Allow `poetry run` and the venv interpreter instead. Also allow the git history commands. The full checkout added in the previous commit was pointless without them. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/claude-code-review.yml | 2 +- CLAUDE.md | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 90a876c9..4b31c0f4 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -72,7 +72,7 @@ jobs: # v1 has no `model` input; the model is passed through to the CLI instead. claude_args: >- --model claude-opus-5 - --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr list:*),Bash(gh api:*),Bash(gh search:*),Bash(poetry run:*),Bash(python:*)" + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr view:*),Bash(gh pr diff:*),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:*)" # 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/CLAUDE.md b/CLAUDE.md index 9b71e4ad..c2e24162 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,5 +25,6 @@ iterable of dicts will accept a positional `dict` and iterate its keys. CI covers lint, type and test breakage on every push, so re-running the suite during review adds nothing. Use the environment for what CI can't do: check whether a suspected bug actually manifests. Write a throwaway script or run one targeted test file, and compare against the base branch when the -question is whether behavior changed. A finding you tried and failed to reproduce is worth more than -one you only reasoned about. +question is whether behavior changed. Reach the project environment through `poetry run` — the bare +`python` on `PATH` is a different interpreter without the dependencies installed. A finding you +tried and failed to reproduce is worth more than one you only reasoned about. From db1664083b60f0113501226c298b7dfc6c5bbbce Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 13:48:59 -0400 Subject: [PATCH 03/12] Port 'Add agent guidance, review rules, and a comment-hygiene gate' from machine PR #514 Adds AGENTS.md, five Claude skills, path-scoped review rules under docs/review/, issue and pull request templates, and a comment-hygiene gate over the lines a branch adds. The gate is rewritten in Python rather than vendoring the PowerShell module. On this repo's current tree both find the same 16 violations, but the PowerShell rules cannot see Python docstrings, which the code-comments skill holds to the same content rules, and this repo needs no pwsh otherwise. The review rules are rebuilt from this repository's own history rather than carrying the C# commit citations, which do not resolve here. Marker and paragraph handling is the defect class that ships most often, then reference and versification arithmetic, then crashes in punctuation analysis. CLAUDE.md now imports AGENTS.md. The review action restores CLAUDE.md from main but not the file it imports, so the fork gate on the review workflow is what keeps that guidance trusted; the workflow comment says so. Closes #375 Co-Authored-By: Claude Opus 5.5 (1M context) --- .claude/skills/code-comments/SKILL.md | 72 +++ .claude/skills/commit-messages/SKILL.md | 26 ++ .claude/skills/issue-authoring/SKILL.md | 57 +++ .claude/skills/pr-authoring/SKILL.md | 88 ++++ .claude/skills/pr-review/SKILL.md | 72 +++ .github/ISSUE_TEMPLATE/bug_report.yml | 68 +++ .github/ISSUE_TEMPLATE/config.yml | 1 + .github/ISSUE_TEMPLATE/feature_request.yml | 43 ++ .github/ISSUE_TEMPLATE/porting_request.yml | 47 ++ .github/PULL_REQUEST_TEMPLATE.md | 32 ++ .github/workflows/claude-code-review.yml | 3 +- .github/workflows/comment-hygiene.yml | 50 +++ .gitignore | 3 + AGENTS.md | 96 ++++ CLAUDE.md | 33 +- docs/review/corpora-usfm.md | 29 ++ docs/review/devils-advocate.md | 26 ++ docs/review/jobs.md | 24 + docs/review/machine-library.md | 29 ++ docs/review/machine-tests.md | 25 ++ docs/review/punctuation.md | 28 ++ local_check.sh | 26 +- scripts/comment_hygiene.py | 490 +++++++++++++++++++++ 23 files changed, 1338 insertions(+), 30 deletions(-) create mode 100644 .claude/skills/code-comments/SKILL.md create mode 100644 .claude/skills/commit-messages/SKILL.md create mode 100644 .claude/skills/issue-authoring/SKILL.md create mode 100644 .claude/skills/pr-authoring/SKILL.md create mode 100644 .claude/skills/pr-review/SKILL.md create mode 100644 .github/ISSUE_TEMPLATE/bug_report.yml create mode 100644 .github/ISSUE_TEMPLATE/config.yml create mode 100644 .github/ISSUE_TEMPLATE/feature_request.yml create mode 100644 .github/ISSUE_TEMPLATE/porting_request.yml create mode 100644 .github/PULL_REQUEST_TEMPLATE.md create mode 100644 .github/workflows/comment-hygiene.yml create mode 100644 AGENTS.md create mode 100644 docs/review/corpora-usfm.md create mode 100644 docs/review/devils-advocate.md create mode 100644 docs/review/jobs.md create mode 100644 docs/review/machine-library.md create mode 100644 docs/review/machine-tests.md create mode 100644 docs/review/punctuation.md create mode 100644 scripts/comment_hygiene.py diff --git a/.claude/skills/code-comments/SKILL.md b/.claude/skills/code-comments/SKILL.md new file mode 100644 index 00000000..e54d4ecb --- /dev/null +++ b/.claude/skills/code-comments/SKILL.md @@ -0,0 +1,72 @@ +--- +name: code-comments +description: MUST use before writing or editing any comment in this repository - content rules, budget, width. +--- + +# machine.py code comments + +Before you write a comment, delete it. If the code and its names already say it, +it was noise. What survives explains a contract, an invariant, a compatibility +requirement, a performance tradeoff, or a non-obvious reason. + +Say WHAT the code guarantees and WHY, in the present tense. Do not narrate HOW +it works - the comment should survive an equivalent rewrite. A docstring +describes that function's own contract, not its caller's. + +Match the file you are editing. Placement, density, and idiom are local here; +read what is already there before you add to it, and use the terms it uses. + +## Never write these + +`scripts/comment_hygiene.py` fails on them over the lines your branch adds: + +1. **Process framing** - `Phase 1`, `later we'll`, `we'll eventually`. +2. **History** - `it used to`, `previously returned`, `was removed`, + `renamed from`, `no longer used`. +3. **Provenance** - `extracted from`, `shared by X and Y`, `the only caller`. +4. **Pointers** - to a Markdown file, a numbered section, a review note, or + another file's comment. +5. **Non-ASCII punctuation** - use `--`, `->`, `...`, `-`, `x`, plain quotes. + Typography only; comment text may use any script the language data needs. + +Present tense about current state is not history: "Returns None when the row has +no text" is a contract. A compatibility note about behavior that must stay true +is welcome, as is an issue reference that is part of the contract. + +## Fit the budget + +One block - a run of whole-line `#` comments, ended by a blank line, code, or a +docstring - gets **200 characters total**, markers and indentation excluded. +Every line fits **120 display columns**, which is black's `line-length`. + +Docstrings are exempt from the budget, not from the content rules or the width +limit. + +Over budget? Shorten it, or let the code express it directly. Do not convert a +`#` block to a docstring to buy the exemption. + +The tree already holds comments over budget. They are known debt, not a +convention: do not copy them, and do not sweep them either. Shorten one when you +are already changing the code it describes. + +## Docstrings + +Write one when a caller needs a contract the signature cannot state: units, +`None` semantics, ownership of a handle, an exception they must handle. Type +hints already carry the types, so do not restate them in prose. + +Omit an `Args:` or `Returns:` entry that only repeats a parameter name or its +annotation; keep it for real result semantics. Document every parameter or none. +No module banners, no divider comments, no fact repeated in both the summary and +the parameter list. + +A test comment explains a non-obvious fixture or setup constraint. It does not +restate the test name. + +## Run the check + +`poetry run python scripts/comment_hygiene.py` scans the lines your branch adds. +Add `--full --advisory` to size existing debt, or `--self-test` to check the +rules themselves. Agents run `./local_check.sh --agent-strict`, which makes the +scan blocking; the pull request check is advisory, so its green tick proves +nothing. diff --git a/.claude/skills/commit-messages/SKILL.md b/.claude/skills/commit-messages/SKILL.md new file mode 100644 index 00000000..0b0c2543 --- /dev/null +++ b/.claude/skills/commit-messages/SKILL.md @@ -0,0 +1,26 @@ +--- +name: commit-messages +description: How to write a commit message in sillsdev/machine.py - imperative subject, short body. +--- + +# Commit messages + +Name the change in an imperative, sentence-case subject under about 72 +characters, with no terminal punctuation: + +- `Fix unclosed style marker crash (#364)` +- `Port the marker placement unit test from sillsdev/machine#496 (#367)` + +A body, when there is one: blank line after the subject, wrapped at about 80 +columns, saying what changed and why. Reference a GitHub issue when one exists. + +## Two traps in the history + +1. The `(#N)` suffix is added by GitHub when a pull request is squashed. Never + type it into a local commit. +2. A handful of old commits carry a Jira identifier such as `LT-22605`. That is + historical; use a GitHub issue reference. + +The 72-character limit is not enforced and subjects over 100 characters exist. +Do not rewrite shared history to satisfy it, or to fix a message on a pushed +branch - add a corrective commit unless the author asks for the rewrite. diff --git a/.claude/skills/issue-authoring/SKILL.md b/.claude/skills/issue-authoring/SKILL.md new file mode 100644 index 00000000..433c5879 --- /dev/null +++ b/.claude/skills/issue-authoring/SKILL.md @@ -0,0 +1,57 @@ +--- +name: issue-authoring +description: How to write a GitHub issue in sillsdev/machine.py - one symptom, short body, real evidence. +argument-hint: Optional issue type, title, symptoms, acceptance criteria, or source PR +user-invocable: true +--- + +# Writing a machine.py issue + +Search open and recently closed issues first, and say what you searched. Then +write the title and the three-sentence lede; the form fields hold the rest. + +GitHub issues are the tracker here. + +## 1. Title: one symptom + +Under about 70 characters, in the reader's words. No "investigate", no +"improve", no component prefix the labels already carry. + +Bad: *USFM parser improvements* +Good: *Unclosed character style swallows the text after a paragraph break* + +## 2. Lede: three sentences + +1. **The symptom** - what goes wrong. +2. **The trigger** - the smallest condition that produces it. +3. **The cost** - who is blocked, or what the caller sees instead. + +Under 25 words each. For a feature, the same three: what is missing, when it +bites, what it costs. + +Good: *An unclosed character style swallows the text after a paragraph break. A +`\w` with no `\w*` in a Paratext project is enough to trigger it. The updated +USFM silently loses a verse.* + +## 3. Body: labelled lines, never a wall + +Write `Unknown` where you do not know, and say how to find out. + +- **Bug** - affected module or API; version, OS, Python, extras installed; the + smallest input that shows it; expected vs actual; sanitized traceback; when it + started; the test that catches it. +- **Feature** - who is blocked and by what; the proposed behavior and its + compatibility cost; acceptance criteria an outsider could check; non-goals. +- **Porting** - the source PR URL, what behavior matters here, what does not. + +Sanitize first: no secrets, tokens, customer text, or private project data. + +## 4. Check it is ready + +Ready means another maintainer can reproduce the bug, judge the acceptance +criteria, or find the change to port - without asking you a question. + +A workflow files the porting issue after a merge, marked `AUTO-GENERATED-ISSUE`. +Do not write a second one by hand. + +Hand back the title, labels, and body. The author decides whether to publish. diff --git a/.claude/skills/pr-authoring/SKILL.md b/.claude/skills/pr-authoring/SKILL.md new file mode 100644 index 00000000..e76afdfa --- /dev/null +++ b/.claude/skills/pr-authoring/SKILL.md @@ -0,0 +1,88 @@ +--- +name: pr-authoring +description: How to write a pull request in sillsdev/machine.py - strong lede, short body, honest evidence. +argument-hint: Optional branch purpose, issue number, or PR number +user-invocable: true +--- + +# Writing a machine.py PR + +Write the body to a file, then `gh pr edit --body-file`. Do not push, open, +or edit a PR unless the author asked. + +What to check and what to run is in `AGENTS.md` and `docs/review/`. This is the +write-up. + +## 1. Write the lede + +Three sentences, each with a different job, before anything else: + +1. **What it does** - what a caller can now do, or what stopped being broken. +2. **The first unknown, answered** - usually "what breaks?" or "why so big?" +3. **The boundary** - what it does not touch. + +Under 25 words each. A sentence needing a subordinate clause belongs in the +body. + +Bad: *This PR refactors the USFM parser and adds some tests.* + +Good: *A verse range matched by several rows keeps every row's metadata, where +the last row used to win. No public signature changes outside +`UsfmUpdateBlock`. Nothing in `machine/jobs` is touched.* + +Invisible to callers? Lead with what it protects: *Agents can no longer land a +comment that narrates its own history.* + +## 2. Fill the top zone + +Under 200 words. Drop any section that would be empty. + +```markdown +## Quick summary + + +## Where to look +- -- + +## Deliberately not included +- + +## Validation +- -- + +## Issue / porting context + +``` + +This mirrors `.github/PULL_REQUEST_TEMPLATE.md`. If the two ever differ, the +template is what contributors actually see; fix this to match it. + +## 3. Put the reasoning below the rule + +Everything longer goes under a `---`, in closed `
` blocks: *Reading +this a year from now*, *Decisions, and why*, *Paths not taken*, *Deferred, and +what would unblock it*. Long reasoning is welcome there and nowhere above. + +No preamble, no apology, no "should be fine", no recap. + +## 4. Check the claims + +Every count, path, symbol, and test name must match the tree. A wrong number in +a PR body outlives the PR. + +Validation lines carry the command and its result, nothing else. Never list a +command you did not run. Never call a local run CI-equivalent - CI collects +coverage and runs the full OS and Python matrix, and `local_check.sh` does +neither. Name any check you skipped, and say which platform produced a result +that a skipped optional dependency could change. + +## Replying to review comments + +Reply in the thread, on the line, in two or three sentences. Classify first: + +- **Fix** - sound and unambiguous; make the smallest change. +- **Clarify** - ask the one specific question. +- **Reply only** - state the verified behavior; change nothing. +- **Defer** - name the follow-up and why it is outside this PR. + +Resolve only a thread that is fully answered and that you did not dispute. diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md new file mode 100644 index 00000000..ada50847 --- /dev/null +++ b/.claude/skills/pr-review/SKILL.md @@ -0,0 +1,72 @@ +--- +name: pr-review +description: How to write up a code review in sillsdev/machine.py - short line comments, evidence, severity. +argument-hint: "[optional PR number, branch, or review focus]" +user-invocable: true +--- + +# Writing a machine.py 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. + +What to look for is in `docs/review/`; `AGENTS.md` maps a changed path to its +rules file. + +## 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. + +## 2. Lead with the claim + +First sentence names the defect. Evidence second, fix third, if it fits. + +``` +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. Collect a list. +``` + +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 +demonstrated: a failing command, a broken contract, a missing gate. A worry is +not Critical. + +## 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. + +- Do not report pre-existing issues the diff does not touch. +- Do not ask for a migration, modernization, or benchmark the diff gave no + reason for. +- A search that found nothing proves absence only if you state what you + searched. +- 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 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. + +## 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`. +3. Counts by severity. +4. What you ran, and its result. +5. What you could not verify. + +Say which public API, optional dependency, published-wheel surface, or parity +contract with `sillsdev/machine` changed, or `None verified`. + +Then mark each finding **changed**, **accepted**, or **unverified**. Leave +nothing implicit: a thread with no follow-up leaves nobody able to tell which +findings mattered. + +For an adversarial second pass, apply `docs/review/devils-advocate.md`. diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml new file mode 100644 index 00000000..82f7459e --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -0,0 +1,68 @@ +name: Bug report +description: Report a reproducible problem in sil-machine +title: "[Bug]: " +labels: + - bug +body: + - type: markdown + attributes: + value: | + Please remove secrets and private project data from examples and logs. + - type: textarea + id: summary + attributes: + label: Summary + description: >- + Three short sentences: the symptom, the smallest trigger, and what it + costs. Detail goes in the fields below. + placeholder: | + A USFM attribute is dropped when the tokenizer runs on a marker with no end marker. + Any character style left unclosed at a paragraph break swallows the following text. + Round-tripping the project silently loses the attribute. + validations: + required: true + - type: input + id: package + attributes: + label: Affected module or API + description: Name the machine subpackage or public API, such as machine.corpora.UsfmTokenizer. + validations: + required: true + - type: input + id: version + attributes: + label: Version or commit + description: Include the sil-machine version or commit SHA. + validations: + required: true + - type: input + id: environment + attributes: + label: Environment + description: OS, architecture, Python version, and any extras installed. + validations: + required: true + - type: textarea + id: reproduction + attributes: + label: Reproduction + description: Minimal input or fixture and exact steps, including frequency. + placeholder: | + 1. ... + 2. ... + Expected: ... + Actual: ... + validations: + required: true + - type: textarea + id: logs + attributes: + label: Logs or traceback + description: Paste sanitized output, or write None. + - type: textarea + id: regression + attributes: + label: Regression and test idea + description: State the first known good version, if known, and the smallest regression test. + validations: + required: true diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml new file mode 100644 index 00000000..0086358d --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1 @@ +blank_issues_enabled: true diff --git a/.github/ISSUE_TEMPLATE/feature_request.yml b/.github/ISSUE_TEMPLATE/feature_request.yml new file mode 100644 index 00000000..3a517c55 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/feature_request.yml @@ -0,0 +1,43 @@ +name: Feature request +description: Propose new or changed behavior in sil-machine +title: "[Feature]: " +labels: + - enhancement +body: + - type: textarea + id: summary + attributes: + label: Summary + description: >- + Three short sentences: what is missing, when it bites, and what it + costs. Detail goes in the fields below. + validations: + required: true + - type: textarea + id: problem + attributes: + label: Who is blocked, and by what + description: Describe the caller and the work they cannot do today. + validations: + required: true + - type: textarea + id: proposal + attributes: + label: Proposed behavior + description: >- + The behavior you want and its compatibility cost. Name any change to a + public API, an optional dependency, or the published wheel. + validations: + required: true + - type: textarea + id: acceptance + attributes: + label: Acceptance criteria + description: Criteria an outsider could check without asking you a question. + validations: + required: true + - type: textarea + id: nongoals + attributes: + label: Non-goals + description: What this deliberately does not cover, or write None. diff --git a/.github/ISSUE_TEMPLATE/porting_request.yml b/.github/ISSUE_TEMPLATE/porting_request.yml new file mode 100644 index 00000000..28713750 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/porting_request.yml @@ -0,0 +1,47 @@ +name: Port from machine +description: Track relevant behavior to port from sillsdev/machine +title: "Port: " +labels: + - porting +body: + - type: markdown + attributes: + value: | + A merged PR normally creates this issue automatically. Use this form when no generated issue exists. + - type: textarea + id: summary + attributes: + label: Summary + description: >- + Three short sentences: the behavior to port, why it matters here, and + what is out of scope. Detail goes in the fields below. + validations: + required: true + - type: input + id: source_pr + attributes: + label: machine source PR or commit + description: Full GitHub URL. + validations: + required: true + - type: textarea + id: behavior + attributes: + label: Behavior to port + description: Describe the relevant behavior and what is not relevant here. + validations: + required: true + - type: textarea + id: target + attributes: + label: Target modules and compatibility + description: Name the target modules and public API, platform implications, and compatibility risks. + validations: + required: true + - type: textarea + id: tests + attributes: + label: Evidence and tests + description: Link source validation and propose target regression tests. Mark unknowns explicitly. + validations: + required: true diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 00000000..f1ab89e7 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,32 @@ +## Quick summary + + + +## Where to look + + + +## Deliberately not included + + + +## Validation + + + +- `./local_check.sh` -- +- `./local_check.sh --agent-strict` -- +- `git diff --check ...HEAD` -- +- + +## Issue / porting context + + + + diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 4b31c0f4..8aa61266 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -18,7 +18,8 @@ jobs: # 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. + # 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 runs-on: ubuntu-latest diff --git a/.github/workflows/comment-hygiene.yml b/.github/workflows/comment-hygiene.yml new file mode 100644 index 00000000..d00fd892 --- /dev/null +++ b/.github/workflows/comment-hygiene.yml @@ -0,0 +1,50 @@ +name: Comment hygiene + +on: + pull_request: + types: [opened, synchronize, reopened] + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + # Separate from CI build so the report does not wait on the Poetry install and + # the test matrix. The checker is standard library only, so it needs no + # dependencies of its own. + comment_hygiene: + name: Report comment hygiene (${{ matrix.os }}) + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest] + + steps: + - name: Check out full history + uses: actions/checkout@v7 + with: + # The scan diffs against the merge base; a shallow checkout leaves + # that commit unreachable. + fetch-depth: 0 + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: "3.12" + + - name: Verify the hygiene rules + run: python scripts/comment_hygiene.py --self-test + + # Advisory: the scan annotates the lines this pull request adds and always + # exits 0. Agents get the blocking version through ./local_check.sh + # --agent-strict, so existing comment debt and any false positive cannot + # block a human contributor. + - name: Scan the lines this pull request adds + run: > + python scripts/comment_hygiene.py + --advisory + --base-ref ${{ github.event.pull_request.base.sha }} diff --git a/.gitignore b/.gitignore index 007871a6..ede546d6 100644 --- a/.gitignore +++ b/.gitignore @@ -142,3 +142,6 @@ out/ # Ignore Poetry plugins .poetry/ + +# Local, generated review summaries +.review/ diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..72e88600 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,96 @@ +# machine.py contributor and agent guide + +What an agent must do here, and where looking at the tree will mislead you. +Everything else - layout, dependencies, what a workflow runs - read from the +tree; it is accurate and this file would only rot. + +If this file disagrees with `local_check.sh`, `.github/workflows/ci.yml`, or the +code you are changing, prefer the executable behavior and say so. + +## Validation + +Run `./local_check.sh` from the repository root: it installs, formats with black +and isort, lints with flake8, type-checks with pyright, and tests. Do not skip a +failing step or report success without fresh output. + +Agents must also run `./local_check.sh --agent-strict`, which makes +`scripts/comment_hygiene.py` blocking over the lines the branch adds. The +standard it enforces is the `code-comments` skill. Do not drop the flag to get a +run through. + +CI collects coverage and `local_check.sh` does not, so a local run is never +coverage-equivalent. + +## Where the tree misleads + +- `ci.yml` triggers on `push`, not `pull_request`. A green check on a pull + request reflects the pushed head, not the merge result, and not a PR gate. +- The `Comment hygiene` check is advisory and never fails. Its green tick is not + evidence that the strict scan passed. +- `poetry install --all-extras` does not install torch. That lives in the `gpu` + Poetry group, which is a group and not an extra. `eflomal` installs only on + Linux, so an alignment path can be exercised on one machine and skipped on + another without saying so. +- pyright runs in `basic` mode. An annotation it accepts is not evidence the + types are sound under `strict`. +- Tests import helpers as `testutils.*`, which resolves only because + `tests/conftest.py` appends that directory to `sys.path`. The package has no + `__init__.py` and is not importable from elsewhere. +- Three different things are called a reference here: a `ScriptureRef`, a + versification-mapped verse location, and ordinary object identity. Say which. + Likewise a "row" is a corpus row and not a USFM line; a "segment" is a + `ScriptureRef` path component in corpora but a text span in tokenization; and + "alignment" is word alignment unless you name another domain. + +## Changing code + +- Add or update focused tests with every behavior change. Keep fixtures + deterministic and platform assumptions explicit. +- USFM marker and paragraph handling is the defect class that ships here most + often - unclosed and implicitly closed markers (`29088f8`, `962ca36`), an + element in an unexpected position (`9868016`), and spacing around end markers + (`417c95f`). Cover the malformed and empty input with the fix. +- Reference and versification arithmetic recurs next (`29d4dbb`, `2a80929`, + `7d85f16`). Assert over the affected book, chapter, and verse mapping rather + than over a rendered string. +- In `punctuation_analysis`, index by text element and not by code unit, and + assume the chapter or verse is missing or unparsable (`53992c8`, `ca37757`). + This area's history is almost entirely crash fixes from live Paratext data. +- Close streams, models, and trainers according to their contracts, and be + explicit about who owns a handle that is passed in. +- The names a subpackage re-exports through `__all__` are what downstream + packages import from the published `sil-machine` wheel. Changing an exported + signature or its semantics is a breaking change even when every caller inside + this repo still works. Watch for a change that fails silently rather than + loudly: a parameter widened from `dict` to an iterable of dicts accepts a + positional `dict` and iterates its keys. +- Prefer an existing abstraction to a parallel one. + +## Branch hygiene + +Preserve unrelated changes and pre-existing untracked files. Never use +`reset --hard`, `checkout --`, broad deletion, or broad staging as a cleanup +shortcut. Local review notes go in `.review/`, which `.gitignore` excludes. + +Much of this library is a port of [sillsdev/machine](https://github.com/sillsdev/machine) +(C#). When changing code that exists on both sides, check what the C# +implementation does first - divergence should be deliberate, not incidental, and +a porting change links its source pull request. A workflow files the porting +issue after merge; do not hand-file a duplicate. + +## Agent guidance + +`CLAUDE.md` imports this file. Claude workflows live under `.claude/skills/`. +Path-scoped review rules live under `docs/review/`; match the changed path, +first row wins: + +| Path glob | Rules file | +| --- | --- | +| `machine/corpora/**/*.py` | `docs/review/corpora-usfm.md` | +| `machine/punctuation_analysis/**/*.py` | `docs/review/punctuation.md` | +| `machine/jobs/**/*.py` | `docs/review/jobs.md` | +| any other `machine/**/*.py` | `docs/review/machine-library.md` | +| `tests/**/*.py` | `docs/review/machine-tests.md` | + +`docs/review/devils-advocate.md` is an optional adversarial pass for a high-risk +change. Add a nested `AGENTS.md` only when a subtree needs different rules. diff --git a/CLAUDE.md b/CLAUDE.md index c2e24162..ed5df484 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,30 +1,7 @@ -# machine.py +@AGENTS.md -Much of this library is a Python port of [sillsdev/machine](https://github.com/sillsdev/machine) (C#). -When changing code that exists on both sides, check what the C# implementation does first — divergence -should be deliberate, not incidental. +## Claude Code -## Comments - -Follow the convention the surrounding code already establishes. - -A comment that no longer describes the code below it is a defect. When changing logic, update or -delete the comments that explain it. - -## Breaking changes - -The names a subpackage re-exports are what downstream packages import from the published wheel, so -changing an exported signature or its semantics is a breaking change even when every caller inside -this repo still works. Say so in the PR description. - -Watch for changes that fail silently rather than loudly — a parameter widened from `dict` to an -iterable of dicts will accept a positional `dict` and iterate its keys. - -## Reviewing changes - -CI covers lint, type and test breakage on every push, so re-running the suite during review adds -nothing. Use the environment for what CI can't do: check whether a suspected bug actually manifests. -Write a throwaway script or run one targeted test file, and compare against the base branch when the -question is whether behavior changed. Reach the project environment through `poetry run` — the bare -`python` on `PATH` is a different interpreter without the dependencies installed. A finding you -tried and failed to reproduce is worth more than one you only reasoned about. +- Repository-wide guidance is `AGENTS.md`, imported above. +- Claude-only workflows live under `.claude/skills/`; path-scoped review rules + under `docs/review/`; `.github/` holds only workflows and templates. diff --git a/docs/review/corpora-usfm.md b/docs/review/corpora-usfm.md new file mode 100644 index 00000000..301f6c18 --- /dev/null +++ b/docs/review/corpora-usfm.md @@ -0,0 +1,29 @@ +# Corpora and USFM Review + +*Review corpus and USFM changes for deterministic marker and token behavior, +ScriptureRef and versification correctness, Unicode handling, and safe file +inputs.* + +Governs `machine/corpora/**/*.py`. + +- Trace changed behavior from source text or corpus row through tokenization, + parsing, ScriptureRef and versification conversion, update handling, and + emitted text. A round trip that produces the same string is not proof that the + intermediate structure is right. +- Marker and paragraph handling is the defect class that recurs most here. + Unclosed and implicitly closed character styles (`29088f8`, `962ca36`), an + element in an unexpected position (`9868016`), spacing around end markers + (`417c95f`), and paragraph markers in row text (`b07fcb6`) were all shipped + bugs. For a change that touches any of them, assert over marker nesting, empty + input, and malformed input, not only the happy path. +- Reference and versification arithmetic is the next most common + (`29d4dbb`). Check that a verse range, a segment path, and a versification + change resolve or fail according to the existing contract, and assert over the + affected book, chapter, and verse mapping rather than over rendered text. +- Compare markers, tokens, and identifiers by exact string identity. Case + folding and locale-aware normalization belong only where the operation is + genuinely linguistic; quotation-mark identity is not. +- For files, ZIPs, and streams, preserve entry and byte limits, path validation, + closing, and actionable errors. Be explicit about who owns a handle passed in. +- A row that produces no text still carries metadata. A verse range matched by + several rows must not silently collapse to one of them. diff --git a/docs/review/devils-advocate.md b/docs/review/devils-advocate.md new file mode 100644 index 00000000..38a0a6c7 --- /dev/null +++ b/docs/review/devils-advocate.md @@ -0,0 +1,26 @@ +# Machine Devil's Advocate + +*Optional adversarial second pass for a high-risk change.* + +Read the merge-base diff, the normal review output, and the tests. Change +nothing. + +This pass prunes the first review; it does not sweep for new categories. Take +the findings it produced and make each earn its place. + +The threshold: a concern is a finding only when the evidence is already in hand. +Label each item `Verified objection` or `Unverified question`, and say what +evidence would settle an unverified one instead of supplying the answer. Drop +what survives neither label. + +Challenge the most consequential claim first, one objection at a time, each with +a `path:line` and a concrete scenario. In this repository the claims that have +failed before are: + +- a USFM or reference change whose test proves the happy path only; +- a parity claim about `sillsdev/machine` with no checked comparison; +- a public API change called internal because every caller in this repo still + compiles; +- a build-job change asserted to be safe under failure without a run that failed. + +Close with `Top risk`, `Evidence still needed`, and `Would this block merge?`. diff --git a/docs/review/jobs.md b/docs/review/jobs.md new file mode 100644 index 00000000..0ca70573 --- /dev/null +++ b/docs/review/jobs.md @@ -0,0 +1,24 @@ +# Build Jobs Review + +*Review ClearML build jobs for failure handling, resource limits, and the +contract with the caller that consumes their output.* + +Governs `machine/jobs/**/*.py`. + +This code runs unattended against remote services, so review it for what happens +when something else fails, not for the successful build. + +- A job that is cancelled, preempted, or killed must end without raising + (`daa1b8e`). Check that cancellation is observed at every await or long loop, + not only at the top. +- Memory is a real limit here (`4faa596`). For a change that grows a batch, a + cache, or an in-memory corpus, say what bounds it. +- Remote storage and ClearML calls fail transiently (`de29377`). A retry must be + bounded and an unrecoverable error must say which artifact and which step. +- Output is a contract. Pretranslations, alignments, and their indices are + consumed by Serval, so a change to their shape or ordering is a breaking + change even though nothing in this repo reads them (`0354cc7`). +- This subpackage needs the `jobs` extra and is not installed by a plain + `pip install sil-machine`. A new import must not leak into the base package. +- Model and trainer changes belong with a measured artifact, not an assertion + (`50cb1de`). diff --git a/docs/review/machine-library.md b/docs/review/machine-library.md new file mode 100644 index 00000000..ffff8d31 --- /dev/null +++ b/docs/review/machine-library.md @@ -0,0 +1,29 @@ +# Machine Library Review + +*Review shipped library code for public API compatibility, deterministic +comparison, and resource ownership.* + +Governs any `machine/**/*.py` no more specific rules file claims. + +- This is shipped library code, published as `sil-machine`. The names a + subpackage re-exports through `__all__` are its public surface. Check the + shape of a changed signature, its defaults, and its return type; an in-repo + clean rename is still a breaking change for a downstream caller (`deb112b`). +- Watch for a change that fails silently rather than loudly. Widening a + parameter from a mapping to an iterable of mappings still accepts the old + positional argument and iterates its keys; a keyword-only parameter, or a + different name, fails at the call site instead. +- Compare markers, tokens, identifiers, and protocol text by exact string + identity. Reserve case folding and locale-aware collation for genuinely + linguistic operations. +- Close streams, models, engines, and trainers according to their contracts. + Prefer a context manager to a `close` the caller must remember, and state who + owns a handle that is passed in. +- Type hints are checked by pyright in `basic` mode, so an annotation that + passes is not evidence the types are sound. Review a changed annotation for a + false promise; do not ask for a repository-wide migration. +- Optional dependencies are real. Code reachable from a plain install must not + import from the `huggingface`, `thot`, `sentencepiece`, or `jobs` extras at + module scope. +- Add focused tests for the decisions the diff changes. Report the commands you + ran; a coverage percentage is not evidence. diff --git a/docs/review/machine-tests.md b/docs/review/machine-tests.md new file mode 100644 index 00000000..9977cf21 --- /dev/null +++ b/docs/review/machine-tests.md @@ -0,0 +1,25 @@ +# Machine Tests Review + +*Review tests as evidence for changed behavior, edge cases, contracts, and +resource boundaries.* + +Governs `tests/**/*.py`. + +- A test must exercise the changed behavior, not merely execute the changed + function. Identify the branches, guards, ordering, error paths, and limits in + the production diff, and name which test covers each. +- Prefer a focused pytest function in the matching `tests//` module. Keep + fixtures deterministic: no sleeps, no ambient machine state, no dependence on + filesystem ordering. +- For string, token, marker, and USFM behavior, include the Unicode, empty, + malformed, and nested cases. Assert over the parsed structure where the + contract is structural; a comparison of rendered text can pass while the + structure underneath is wrong. +- A test that only round-trips input to output proves serialization, not + behavior. When the change is about what a row or block carries, assert on that + value directly. +- Some tests are skipped rather than failed when an optional dependency is + absent. A green run on one platform is not a green run everywhere - say which + platform produced the result you are reporting. +- Name the test that proves the change. "Where is the test?" is the single most + common review question here; answer it before it is asked. diff --git a/docs/review/punctuation.md b/docs/review/punctuation.md new file mode 100644 index 00000000..bfd085e7 --- /dev/null +++ b/docs/review/punctuation.md @@ -0,0 +1,28 @@ +# Punctuation Analysis Review + +*Review quotation-mark and punctuation analysis for Unicode safety, malformed +input, and chapter and verse bookkeeping.* + +Governs `machine/punctuation_analysis/**/*.py`. + +This area's history is almost entirely crash fixes reported from live data, so +review it for the input that should not have reached the code rather than for +the happy path. + +- Index by text element, not by code unit. A surrogate pair, a combining mark, + or a multi-code-point quotation mark must not split. Python strings hide the + surrogate problem that other runtimes surface, which makes the combining-mark + case easier to miss, not less likely. +- Assume the chapter or verse is missing, out of range, or unparsable. The + extractor runs over real Paratext projects: `53992c8` fixed chapter numbers + that came back wrong and `ca37757` fixed a crash on an invalid chapter. +- A depth or state machine that tracks open and close marks must terminate on + unbalanced input and say what it saw, rather than running to the end of the + text. +- Treat quotation-mark identity as exact. Denormalization maps one code point to + another; a case-insensitive or locale-aware comparison here is a bug. +- Quote convention detection reads whole projects and must degrade rather than + raise when a book is absent or unreadable (`0c3cd9c`, `d93dacd`). +- Add a fixture for the malformed case with the fix, in + `tests/punctuation_analysis/`. Every fix above came from live data, not from + review. diff --git a/local_check.sh b/local_check.sh index eea97921..69aa3825 100755 --- a/local_check.sh +++ b/local_check.sh @@ -1,6 +1,30 @@ #!/bin/bash +# Usage: ./local_check.sh [--agent-strict] +# +# --agent-strict adds the comment-hygiene check and fails on a violation in the +# lines this branch adds. Agents must pass it; humans need not. + +agent_strict=false +if [ "${1:-}" = "--agent-strict" ]; then + agent_strict=true + shift +fi + +if [ "$#" -ne 0 ]; then + echo "Usage: $0 [--agent-strict]" >&2 + exit 2 +fi + poetry install +if [ "$agent_strict" = true ]; then + echo "=================== comment hygiene =================" + poetry run python scripts/comment_hygiene.py + if [ $? -ne 0 ]; then + exit 1 + fi +fi + echo "======================= black ======================" poetry run black . echo "======================= flake8 ======================" @@ -10,4 +34,4 @@ poetry run isort . echo "======================= pyright ======================" poetry run pyright echo "======================= pytest ======================" -poetry run pytest \ No newline at end of file +poetry run pytest diff --git a/scripts/comment_hygiene.py b/scripts/comment_hygiene.py new file mode 100644 index 00000000..6f9f2dcd --- /dev/null +++ b/scripts/comment_hygiene.py @@ -0,0 +1,490 @@ +#!/usr/bin/env python +"""Check the comment hygiene of the lines this branch adds. + +Diffs the working tree against the merge base with the base ref, collects the +added lines in Python and shell files, and applies the rules below. Untracked +in-scope files count entirely as added. + +Scoping to added lines is what makes the check usable: existing comment debt +stays out of the way, and a branch is answerable only for what it writes. + +The standard it enforces is the code-comments skill, named in the failure output. +""" + +from __future__ import annotations + +import argparse +import ast +import io +import json +import re +import subprocess +import sys +import tokenize +from dataclasses import dataclass +from pathlib import Path +from typing import Iterable, Optional, Sequence + +MAX_BLOCK_CHARS = 200 +DEFAULT_MAX_LINE_LENGTH = 120 +TAB_WIDTH = 4 + +SCOPED_PATH_SPECS = [ + "machine/*.py", + "tests/*.py", + "scripts/*.py", + "local_check.sh", +] + +# Built from code points rather than literals so the set survives a source +# re-encoding. Only these are reported; a comment may otherwise carry any script +# the language data needs. +NON_ASCII_PUNCTUATION = [ + "—", + "–", + "→", + "←", + "↔", + "…", + "•", + "×", + "‘", + "’", + "“", + "”", + "§", +] + +_ABSENCE_NARRATION = ( + r"\b(?:it|this|that|these|those|we|they|which)\s+used to\b" + r"|\bused to be\b|\bpreviously (?:read|worked|did|returned|used|called)\b" + r"|\b(?:was|were) removed\b|\b(?:was|were) stale\b|\brenamed from\b|\bfirst shipped\b" + r"|\bno longer (?:used|needed|exists|exist|supported|present|applies|apply|valid)\b" +) + +# A bare "used to" or "no longer" usually reads as purpose or present state, so +# both require explicitly historical phrasing. +_RULES: dict[str, str] = { + "process-framing": r"\bPhase[\s-]?\d+\b|\blater we'll\b|\bwe'll (?:later|eventually)\b", + "doc-pointer": r"\b[\w./-]+\.md\b|\bsection\s+\d+[a-z]?\b", + "absence-narration": _ABSENCE_NARRATION, + "cross-file-pointer": r"\bsee [A-Za-z]+'s note\b|\bas documented (?:on|in) [A-Za-z]+\b", + "provenance": r"\bshared by \w+ and \w+\b|\bthe only caller\b|\bthe sole caller\b|\bextracted from\b", + "non-ascii-punctuation": "(?:" + "|".join(re.escape(c) for c in NON_ASCII_PUNCTUATION) + ")", +} + +CATEGORIES = {name: re.compile(pattern, re.IGNORECASE) for name, pattern in _RULES.items()} + + +@dataclass(frozen=True) +class Violation: + file: str + line: int + category: str + text: str + + +@dataclass +class CommentLine: + """One whole-line comment: its text, and whether the budget counts it.""" + + line: int + body: str + exempt: bool + + +def display_width(line: str) -> int: + """Return a line's width in columns, expanding tabs to the next tab stop.""" + width = 0 + for char in line: + if char == "\t": + width += TAB_WIDTH - (width % TAB_WIDTH) + else: + width += 1 + return width + + +def max_line_length(repo_root: Path) -> int: + """Read the width limit from black's setting so the two cannot drift apart.""" + pyproject = repo_root / "pyproject.toml" + if not pyproject.exists(): + return DEFAULT_MAX_LINE_LENGTH + in_black = False + for raw in pyproject.read_text(encoding="utf-8").splitlines(): + stripped = raw.strip() + if stripped.startswith("["): + in_black = stripped == "[tool.black]" + continue + if not in_black: + continue + match = re.match(r"^line-length\s*=\s*(\d+)$", stripped) + if match: + return int(match.group(1)) + return DEFAULT_MAX_LINE_LENGTH + + +def _docstring_rows(tree: ast.AST) -> set[int]: + """Return every line a module, class, or function docstring occupies.""" + rows: set[int] = set() + holders = (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef) + for node in ast.walk(tree): + if not isinstance(node, holders) or not node.body: + continue + first = node.body[0] + if isinstance(first, ast.Expr) and isinstance(first.value, ast.Constant): + if isinstance(first.value.value, str) and first.end_lineno is not None: + rows.update(range(first.lineno, first.end_lineno + 1)) + return rows + + +def _python_comments(text: str) -> list[CommentLine]: + """Classify a Python file's whole-line comments and its docstrings. + + Uses the tokenizer rather than line prefixes so a number sign inside a + string literal is never mistaken for a comment. A docstring is exempt from + the budget, not from the content rules or the width limit. + """ + try: + tokens = list(tokenize.generate_tokens(io.StringIO(text).readline)) + tree = ast.parse(text) + except (tokenize.TokenError, IndentationError, SyntaxError, ValueError): + return [] + + lines = text.splitlines() + skip = (tokenize.COMMENT, tokenize.NL, tokenize.NEWLINE, tokenize.INDENT, tokenize.DEDENT, tokenize.ENDMARKER) + code_rows = {token.start[0] for token in tokens if token.type not in skip} + + comments: list[CommentLine] = [] + for token in tokens: + if token.type != tokenize.COMMENT: + continue + row = token.start[0] + if row in code_rows: + continue + if row == 1 and token.string.startswith("#!"): + continue + comments.append(CommentLine(row, token.string.lstrip("#"), exempt=False)) + + for row in sorted(_docstring_rows(tree)): + if row <= len(lines): + comments.append(CommentLine(row, lines[row - 1], exempt=True)) + + return sorted(comments, key=lambda c: c.line) + + +def _shell_comments(text: str) -> list[CommentLine]: + """Classify a shell script's whole-line comments. A shebang is not one.""" + comments: list[CommentLine] = [] + for index, raw in enumerate(text.splitlines(), start=1): + stripped = raw.strip() + if index == 1 and stripped.startswith("#!"): + continue + if stripped.startswith("#"): + comments.append(CommentLine(index, stripped[1:], exempt=False)) + return comments + + +def classify(path: Path, text: str) -> Optional[list[CommentLine]]: + """Return the file's whole-line comments, or None when it is out of scope.""" + if path.suffix == ".py": + return _python_comments(text) + if path.suffix == ".sh" or path.name == "local_check.sh": + return _shell_comments(text) + return None + + +def scan_file(path: Path, repo_root: Path, allowed: Optional[set[int]], width_limit: int) -> list[Violation]: + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return [] + + comments = classify(path, text) + if comments is None: + return [] + + lines = text.splitlines() + relative = path.relative_to(repo_root).as_posix() + violations: list[Violation] = [] + + for comment in comments: + if allowed is not None and comment.line not in allowed: + continue + for category, pattern in CATEGORIES.items(): + if pattern.search(comment.body): + violations.append(Violation(relative, comment.line, category, comment.body.strip())) + # Width applies to every comment line including a docstring: those are + # exempt from what they may say, not from how wide they may run. + raw = lines[comment.line - 1] if comment.line <= len(lines) else "" + width = display_width(raw) + if width > width_limit: + text_ = f"{width} columns (max {width_limit}): {comment.body.strip()}" + violations.append(Violation(relative, comment.line, "comment-line-too-long", text_)) + + violations.extend(_budget_violations(comments, relative, allowed)) + return violations + + +def _budget_violations(comments: list[CommentLine], relative: str, allowed: Optional[set[int]]) -> list[Violation]: + """Report a run of consecutive non-exempt comment lines over the budget. + + A block whose untouched lines alone already exceed the budget is left to a + separate cleanup, so a branch is never blocked by debt it did not write. + """ + violations: list[Violation] = [] + by_line = {c.line: c for c in comments if not c.exempt} + block: list[CommentLine] = [] + + def flush() -> None: + if not block: + return + total = sum(len(c.body.strip()) for c in block) + if total <= MAX_BLOCK_CHARS: + return + if allowed is not None: + if not any(c.line in allowed for c in block): + return + untouched = sum(len(c.body.strip()) for c in block if c.line not in allowed) + if untouched > MAX_BLOCK_CHARS: + return + text = f"{total} chars (budget {MAX_BLOCK_CHARS}): {block[0].body.strip()}" + violations.append(Violation(relative, block[0].line, "comment-too-long", text)) + + for line in sorted(by_line): + if block and line != block[-1].line + 1: + flush() + block = [] + block.append(by_line[line]) + flush() + return violations + + +def _git(args: Sequence[str], cwd: Optional[Path] = None) -> str: + result = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True, encoding="utf-8") + if result.returncode != 0: + return "" + return result.stdout + + +def repo_root() -> Path: + top = _git(["rev-parse", "--show-toplevel"]).strip() + if not top: + raise SystemExit("comment-hygiene must run inside a git working tree.") + return Path(top) + + +def resolve_base_ref(explicit: Optional[str]) -> str: + """Return the first ref git can resolve, preferring an explicit value. + + git remote show is deliberately not used: resolving a base must not need + network access on a developer machine. + """ + import os + + candidates = [explicit] if explicit else [] + base_ref = os.environ.get("GITHUB_BASE_REF") + if base_ref: + candidates.append(f"origin/{base_ref}") + candidates += ["origin/HEAD", "origin/main"] + for candidate in candidates: + if _git(["rev-parse", "--verify", "--quiet", f"{candidate}^{{commit}}"]).strip(): + return candidate + raise SystemExit(f"No usable base ref. Tried: {', '.join(candidates)}. Fetch the base branch, or pass --base-ref.") + + +def added_line_filter(base: str, root: Path) -> tuple[str, dict[Path, set[int]]]: + """Map each in-scope path to the line numbers this branch adds. + + Diffs the working tree against the merge base so local edits are checked + before they are committed. + """ + merge_base = _git(["merge-base", base, "HEAD"]).strip() + if not merge_base: + raise SystemExit(f"No merge base between {base} and HEAD. Fetch enough history (fetch-depth: 0 in CI).") + + filter_: dict[Path, set[int]] = {} + current: Optional[Path] = None + line_number = 0 + diff = _git(["diff", "--unified=0", merge_base, "--", *SCOPED_PATH_SPECS]) + for line in diff.splitlines(): + if line.startswith("+++ "): + path = line[4:].strip() + if path == "/dev/null": + current = None + continue + current = root / (path[2:] if path.startswith("b/") else path) + filter_.setdefault(current, set()) + continue + match = re.match(r"^@@ -\S+ \+(\d+)(?:,\d+)? @@", line) + if match: + line_number = int(match.group(1)) + continue + if current is not None and line.startswith("+"): + filter_[current].add(line_number) + line_number += 1 + + untracked = _git(["ls-files", "--others", "--exclude-standard", "--", *SCOPED_PATH_SPECS]) + for path in untracked.splitlines(): + if not path.strip(): + continue + full = root / path.strip() + if not full.exists(): + continue + count = len(full.read_text(encoding="utf-8", errors="replace").splitlines()) + filter_[full] = set(range(1, count + 1)) + + return merge_base, filter_ + + +SELF_TEST_CASES: list[tuple[str, str, list[str], list[str]]] = [ + ("clean python is clean", "clean.py", ["# Guards against a missing row.", "a = 1"], []), + ( + "docstring is budget exempt", + "doc.py", + ['"""' + "y" * 60, "y" * 60, "y" * 60, "y" * 60 + '"""', "a = 1"], + [], + ), + ( + "block budget aggregates lines", + "block.py", + [f"# {'y' * 60}"] * 4 + ["a = 1"], + ["comment-too-long"], + ), + ("exactly at budget is clean", "exact.py", [f"# {'z' * 50}"] * 4 + ["a = 1"], []), + ( + "one over budget is reported", + "over.py", + [f"# {'z' * 50}"] * 3 + [f"# {'z' * 50}a", "a = 1"], + ["comment-too-long"], + ), + ( + "blank line ends a block", + "split.py", + [f"# {'y' * 60}", f"# {'y' * 60}", "", f"# {'y' * 60}", f"# {'y' * 60}", "a = 1"], + [], + ), + ("width counts the marker and indent", "wide.py", [f"# {'x' * 130}", "a = 1"], ["comment-line-too-long"]), + ("em dash is reported", "dash.py", ["# Uses a dash — here.", "a = 1"], ["non-ascii-punctuation"]), + ( + "absence narration is reported", + "absence.py", + ["# This field is no longer used.", "a = 1"], + ["absence-narration"], + ), + ("markdown pointer is reported", "pointer.py", ["# See design-notes.md for why.", "a = 1"], ["doc-pointer"]), + ("provenance is reported", "prov.py", ["# Extracted from the old tokenizer.", "a = 1"], ["provenance"]), + ("a number sign in a string is not a comment", "str.py", ['a = "# no longer used"'], []), + ( + "a multi-line docstring reports its content once", + "multi.py", + ['"""Summary.', "", " Extracted from the old tokenizer.", ' """', "a = 1"], + ["provenance"], + ), + ("a trailing comment is not a whole-line comment", "trail.py", ["a = 1 # no longer used"], []), + ("shell shebang is exempt", "run.sh", ["#!/bin/bash", "echo hi"], []), + ("shell comment uses the same budget", "budget.sh", [f"# {'y' * 60}"] * 4 + ["echo hi"], ["comment-too-long"]), + ("unscoped extension is skipped", "notes.md", ["# no longer relevant"], []), +] + + +def self_test() -> int: + """Exercise the rules against fixtures, so they stay verifiable anywhere.""" + import tempfile + + failures: list[str] = [] + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / "pyproject.toml").write_text("[tool.black]\nline-length = 120\n", encoding="utf-8") + width = max_line_length(root) + for name, filename, content, expected in SELF_TEST_CASES: + path = root / filename + path.write_text("\n".join(content) + "\n", encoding="utf-8") + found = sorted(v.category for v in scan_file(path, root, None, width)) + if found != sorted(expected): + failures.append(f"{name}: expected {sorted(expected)} but found {found}") + + if failures: + print("comment-hygiene self-test FAILED") + for failure in failures: + print(f" {failure}") + return 1 + print("comment-hygiene self-test passed") + return 0 + + +def main(argv: Optional[Sequence[str]] = None) -> int: + parser = argparse.ArgumentParser(description="Check comment hygiene over the lines this branch adds.") + parser.add_argument("--base-ref", help="Ref to diff against. Defaults to the PR base, then origin/HEAD, main.") + parser.add_argument("--full", action="store_true", help="Scan every tracked in-scope file, to size existing debt.") + parser.add_argument("--advisory", action="store_true", help="Report violations and still exit 0.") + parser.add_argument("--report-path", help="Optional path for a JSON report.") + parser.add_argument("--self-test", action="store_true", help="Run the built-in rule cases and exit.") + args = parser.parse_args(argv) + + if args.self_test: + return self_test() + + root = repo_root() + width = max_line_length(root) + line_filter: Optional[dict[Path, set[int]]] = None + + if args.full: + tracked = _git(["ls-files", "--", *SCOPED_PATH_SPECS]) + files = [root / p.strip() for p in tracked.splitlines() if p.strip()] + scope = f"all {len(files)} tracked in-scope files" + else: + base = resolve_base_ref(args.base_ref) + merge_base, line_filter = added_line_filter(base, root) + files = list(line_filter) + scope = f"lines added since {base} (merge base {merge_base[:8]})" + + if not files: + print(f"comment-hygiene: no in-scope files to check ({scope}).") + return 0 + + violations: list[Violation] = [] + for path in files: + allowed = line_filter.get(path) if line_filter is not None else None + violations.extend(scan_file(path, root, allowed, width)) + violations.sort(key=lambda v: (v.file, v.line, v.category)) + + if args.report_path: + report = Path(args.report_path) + report.parent.mkdir(parents=True, exist_ok=True) + payload = { + "scope": scope, + "advisory": args.advisory, + "violationCount": len(violations), + "violations": [v.__dict__ for v in violations], + } + report.write_text(json.dumps(payload, indent=2), encoding="utf-8") + + if not violations: + print(f"comment-hygiene: clean ({scope}).") + return 0 + + print(f"comment-hygiene: {len(violations)} violation(s) in {scope}.") + print() + _report(violations, advisory=args.advisory) + print() + print("The standard is .claude/skills/code-comments/SKILL.md.") + print("Reproduce locally with: poetry run python scripts/comment_hygiene.py") + + if args.advisory: + print("Advisory only; not failing the run.") + return 0 + return 1 + + +def _report(violations: Iterable[Violation], advisory: bool) -> None: + import os + + in_actions = os.environ.get("GITHUB_ACTIONS") == "true" + for violation in violations: + print(f"{violation.file}:{violation.line}: {violation.category}: {violation.text}") + if advisory and in_actions: + print(f"::warning file={violation.file},line={violation.line}::{violation.category}: {violation.text}") + + +if __name__ == "__main__": + sys.exit(main()) From 26d36bd845584ea695d8f7c702771ed772c0c42f Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 15:22:54 -0400 Subject: [PATCH 04/12] Number review findings in the pr-review skill A reply or a follow-up round can then refer to a finding without quoting it. The prefix is F1, F2, ... rather than #1, which GitHub links to issue 1. Co-Authored-By: Claude Opus 5.5 (1M context) --- .claude/skills/pr-review/SKILL.md | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index ada50847..6da71b96 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -19,13 +19,20 @@ rules file. 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 start each +comment with its number, 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 number and severity, the first sentence names the defect. Evidence +second, fix third, if it fits. ``` -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. Collect a list. +F1 Important. 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. +Collect a list. ``` Three lines is long. A finding needing more is a design question - raise it in @@ -57,7 +64,7 @@ confirm `Unverified`; an unverified concern never blocks a merge. ## 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. @@ -65,8 +72,8 @@ confirm `Unverified`; an unverified concern never blocks a merge. Say which public API, optional dependency, published-wheel surface, or parity contract with `sillsdev/machine` changed, or `None verified`. -Then mark each finding **changed**, **accepted**, or **unverified**. Leave -nothing implicit: a thread with no follow-up leaves nobody able to tell which -findings mattered. +Then mark each finding, by number, **changed**, **accepted**, or +**unverified**. Leave nothing implicit: a thread with no follow-up leaves nobody +able to tell which findings mattered. For an adversarial second pass, apply `docs/review/devils-advocate.md`. From 783600695d1ff6130a8aa04beff9efe92003020b Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 15:27:48 -0400 Subject: [PATCH 05/12] Require corpus classes to stream in the corpora review rules Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/review/corpora-usfm.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/review/corpora-usfm.md b/docs/review/corpora-usfm.md index 301f6c18..909a3392 100644 --- a/docs/review/corpora-usfm.md +++ b/docs/review/corpora-usfm.md @@ -23,6 +23,12 @@ Governs `machine/corpora/**/*.py`. - Compare markers, tokens, and identifiers by exact string identity. Case folding and locale-aware normalization belong only where the operation is genuinely linguistic; quotation-mark identity is not. +- Every corpus class streams. Rows come from a `_get_rows` generator that + `get_rows` wraps in a `ContextManagedGenerator`, and a transform or new corpus + yields from its source inside `with ... get_rows() as rows:` rather than + collecting rows into a list. A whole-Bible corpus, or several of them in a + parallel corpus, must not have to fit in memory. The dictionary-backed corpora + are the exception: they wrap data the caller already holds. - For files, ZIPs, and streams, preserve entry and byte limits, path validation, closing, and actionable errors. Be explicit about who owns a handle passed in. - A row that produces no text still carries metadata. A verse range matched by From c8aa9b5c245d69367d9e2289921a1f5a295733fd Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 15:32:48 -0400 Subject: [PATCH 06/12] Cut agent guidance the codebase already states Drops what an agent can read from the tree: what local_check.sh runs, pyright's mode, the testutils import path, the extras layout, the directory map in CLAUDE.md, and restatements of AGENTS.md in the skills. pr-authoring now points at the PR template instead of copying it. Also drops claims that did not hold for this repo: a vocabulary list written by analogy with machine ("segment" never appears in machine/tokenization), a rule about platform-dependent test skips (the three skipped tests are manual-only), an "await" in the jobs rules (the jobs are synchronous), and a Jira trap backed by one commit. Co-Authored-By: Claude Opus 5.5 (1M context) --- .claude/skills/code-comments/SKILL.md | 10 +------ .claude/skills/commit-messages/SKILL.md | 13 +++------ .claude/skills/issue-authoring/SKILL.md | 5 ---- .claude/skills/pr-authoring/SKILL.md | 30 ++++----------------- .claude/skills/pr-review/SKILL.md | 7 +---- .github/PULL_REQUEST_TEMPLATE.md | 2 +- AGENTS.md | 36 +++++-------------------- CLAUDE.md | 6 ----- docs/review/corpora-usfm.md | 11 ++++---- docs/review/devils-advocate.md | 2 +- docs/review/jobs.md | 6 ++--- docs/review/machine-library.md | 15 +++++------ docs/review/machine-tests.md | 11 +++----- docs/review/punctuation.md | 11 +++----- 14 files changed, 39 insertions(+), 126 deletions(-) diff --git a/.claude/skills/code-comments/SKILL.md b/.claude/skills/code-comments/SKILL.md index e54d4ecb..609c59af 100644 --- a/.claude/skills/code-comments/SKILL.md +++ b/.claude/skills/code-comments/SKILL.md @@ -37,7 +37,7 @@ is welcome, as is an issue reference that is part of the contract. One block - a run of whole-line `#` comments, ended by a blank line, code, or a docstring - gets **200 characters total**, markers and indentation excluded. -Every line fits **120 display columns**, which is black's `line-length`. +Every line fits **120 display columns**. Docstrings are exempt from the budget, not from the content rules or the width limit. @@ -62,11 +62,3 @@ the parameter list. A test comment explains a non-obvious fixture or setup constraint. It does not restate the test name. - -## Run the check - -`poetry run python scripts/comment_hygiene.py` scans the lines your branch adds. -Add `--full --advisory` to size existing debt, or `--self-test` to check the -rules themselves. Agents run `./local_check.sh --agent-strict`, which makes the -scan blocking; the pull request check is advisory, so its green tick proves -nothing. diff --git a/.claude/skills/commit-messages/SKILL.md b/.claude/skills/commit-messages/SKILL.md index 0b0c2543..eb4f2459 100644 --- a/.claude/skills/commit-messages/SKILL.md +++ b/.claude/skills/commit-messages/SKILL.md @@ -14,13 +14,8 @@ characters, with no terminal punctuation: A body, when there is one: blank line after the subject, wrapped at about 80 columns, saying what changed and why. Reference a GitHub issue when one exists. -## Two traps in the history +The `(#N)` suffix in the history is added by GitHub when a pull request is +squashed. Never type it into a local commit. -1. The `(#N)` suffix is added by GitHub when a pull request is squashed. Never - type it into a local commit. -2. A handful of old commits carry a Jira identifier such as `LT-22605`. That is - historical; use a GitHub issue reference. - -The 72-character limit is not enforced and subjects over 100 characters exist. -Do not rewrite shared history to satisfy it, or to fix a message on a pushed -branch - add a corrective commit unless the author asks for the rewrite. +Do not rewrite shared history to fix a message on a pushed branch - add a +corrective commit unless the author asks for the rewrite. diff --git a/.claude/skills/issue-authoring/SKILL.md b/.claude/skills/issue-authoring/SKILL.md index 433c5879..a997ce26 100644 --- a/.claude/skills/issue-authoring/SKILL.md +++ b/.claude/skills/issue-authoring/SKILL.md @@ -10,8 +10,6 @@ user-invocable: true Search open and recently closed issues first, and say what you searched. Then write the title and the three-sentence lede; the form fields hold the rest. -GitHub issues are the tracker here. - ## 1. Title: one symptom Under about 70 characters, in the reader's words. No "investigate", no @@ -51,7 +49,4 @@ Sanitize first: no secrets, tokens, customer text, or private project data. Ready means another maintainer can reproduce the bug, judge the acceptance criteria, or find the change to port - without asking you a question. -A workflow files the porting issue after a merge, marked `AUTO-GENERATED-ISSUE`. -Do not write a second one by hand. - Hand back the title, labels, and body. The author decides whether to publish. diff --git a/.claude/skills/pr-authoring/SKILL.md b/.claude/skills/pr-authoring/SKILL.md index e76afdfa..72599cb8 100644 --- a/.claude/skills/pr-authoring/SKILL.md +++ b/.claude/skills/pr-authoring/SKILL.md @@ -35,27 +35,9 @@ comment that narrates its own history.* ## 2. Fill the top zone -Under 200 words. Drop any section that would be empty. - -```markdown -## Quick summary - - -## Where to look -- -- - -## Deliberately not included -- - -## Validation -- -- - -## Issue / porting context - -``` - -This mirrors `.github/PULL_REQUEST_TEMPLATE.md`. If the two ever differ, the -template is what contributors actually see; fix this to match it. +Use the sections in `.github/PULL_REQUEST_TEMPLATE.md`, in under 200 words. The +Quick summary is the lede and nothing else. Drop any section that would be +empty. ## 3. Put the reasoning below the rule @@ -71,10 +53,8 @@ Every count, path, symbol, and test name must match the tree. A wrong number in a PR body outlives the PR. Validation lines carry the command and its result, nothing else. Never list a -command you did not run. Never call a local run CI-equivalent - CI collects -coverage and runs the full OS and Python matrix, and `local_check.sh` does -neither. Name any check you skipped, and say which platform produced a result -that a skipped optional dependency could change. +command you did not run, and never call a local run CI-equivalent. Name any +check you skipped. ## Replying to review comments diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 6da71b96..6096ac77 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -11,9 +11,6 @@ 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. -What to look for is in `docs/review/`; `AGENTS.md` maps a changed path to its -rules file. - ## 1. One finding, one comment Anchor it on the line. Two problems on one line are two comments. A reviewer @@ -54,9 +51,7 @@ confirm `Unverified`; an unverified concern never blocks a merge. reason for. - A search that found nothing proves absence only if you state what you searched. -- 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. +- Name the commands you ran and what they returned. - 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. diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index f1ab89e7..62b6a279 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -22,7 +22,7 @@ neither, so a local run is not CI-equivalent. --> - `./local_check.sh` -- - `./local_check.sh --agent-strict` -- - `git diff --check ...HEAD` -- -- +- ## Issue / porting context diff --git a/AGENTS.md b/AGENTS.md index 72e88600..f21392b8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,17 +9,9 @@ code you are changing, prefer the executable behavior and say so. ## Validation -Run `./local_check.sh` from the repository root: it installs, formats with black -and isort, lints with flake8, type-checks with pyright, and tests. Do not skip a -failing step or report success without fresh output. - -Agents must also run `./local_check.sh --agent-strict`, which makes -`scripts/comment_hygiene.py` blocking over the lines the branch adds. The -standard it enforces is the `code-comments` skill. Do not drop the flag to get a -run through. - -CI collects coverage and `local_check.sh` does not, so a local run is never -coverage-equivalent. +Run `./local_check.sh` from the repository root, and do not skip a failing step +or report success without fresh output. Agents must also pass +`./local_check.sh --agent-strict`; do not drop the flag to get a run through. ## Where the tree misleads @@ -27,20 +19,6 @@ coverage-equivalent. request reflects the pushed head, not the merge result, and not a PR gate. - The `Comment hygiene` check is advisory and never fails. Its green tick is not evidence that the strict scan passed. -- `poetry install --all-extras` does not install torch. That lives in the `gpu` - Poetry group, which is a group and not an extra. `eflomal` installs only on - Linux, so an alignment path can be exercised on one machine and skipped on - another without saying so. -- pyright runs in `basic` mode. An annotation it accepts is not evidence the - types are sound under `strict`. -- Tests import helpers as `testutils.*`, which resolves only because - `tests/conftest.py` appends that directory to `sys.path`. The package has no - `__init__.py` and is not importable from elsewhere. -- Three different things are called a reference here: a `ScriptureRef`, a - versification-mapped verse location, and ordinary object identity. Say which. - Likewise a "row" is a corpus row and not a USFM line; a "segment" is a - `ScriptureRef` path component in corpora but a text span in tokenization; and - "alignment" is word alignment unless you name another domain. ## Changing code @@ -53,7 +31,7 @@ coverage-equivalent. - Reference and versification arithmetic recurs next (`29d4dbb`, `2a80929`, `7d85f16`). Assert over the affected book, chapter, and verse mapping rather than over a rendered string. -- In `punctuation_analysis`, index by text element and not by code unit, and +- In `punctuation_analysis`, index by text element and not by code point, and assume the chapter or verse is missing or unparsable (`53992c8`, `ca37757`). This area's history is almost entirely crash fixes from live Paratext data. - Close streams, models, and trainers according to their contracts, and be @@ -70,7 +48,7 @@ coverage-equivalent. Preserve unrelated changes and pre-existing untracked files. Never use `reset --hard`, `checkout --`, broad deletion, or broad staging as a cleanup -shortcut. Local review notes go in `.review/`, which `.gitignore` excludes. +shortcut. Local review notes go in `.review/`. Much of this library is a port of [sillsdev/machine](https://github.com/sillsdev/machine) (C#). When changing code that exists on both sides, check what the C# @@ -80,7 +58,6 @@ issue after merge; do not hand-file a duplicate. ## Agent guidance -`CLAUDE.md` imports this file. Claude workflows live under `.claude/skills/`. Path-scoped review rules live under `docs/review/`; match the changed path, first row wins: @@ -92,5 +69,4 @@ first row wins: | any other `machine/**/*.py` | `docs/review/machine-library.md` | | `tests/**/*.py` | `docs/review/machine-tests.md` | -`docs/review/devils-advocate.md` is an optional adversarial pass for a high-risk -change. Add a nested `AGENTS.md` only when a subtree needs different rules. +Add a nested `AGENTS.md` only when a subtree needs different rules. diff --git a/CLAUDE.md b/CLAUDE.md index ed5df484..43c994c2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,7 +1 @@ @AGENTS.md - -## Claude Code - -- Repository-wide guidance is `AGENTS.md`, imported above. -- Claude-only workflows live under `.claude/skills/`; path-scoped review rules - under `docs/review/`; `.github/` holds only workflows and templates. diff --git a/docs/review/corpora-usfm.md b/docs/review/corpora-usfm.md index 909a3392..930f8715 100644 --- a/docs/review/corpora-usfm.md +++ b/docs/review/corpora-usfm.md @@ -23,12 +23,11 @@ Governs `machine/corpora/**/*.py`. - Compare markers, tokens, and identifiers by exact string identity. Case folding and locale-aware normalization belong only where the operation is genuinely linguistic; quotation-mark identity is not. -- Every corpus class streams. Rows come from a `_get_rows` generator that - `get_rows` wraps in a `ContextManagedGenerator`, and a transform or new corpus - yields from its source inside `with ... get_rows() as rows:` rather than - collecting rows into a list. A whole-Bible corpus, or several of them in a - parallel corpus, must not have to fit in memory. The dictionary-backed corpora - are the exception: they wrap data the caller already holds. +- Every corpus class streams: a transform or new corpus yields from its source + inside `with ... get_rows() as rows:` rather than collecting rows into a list. + A whole-Bible corpus, or several of them in a parallel corpus, must not have + to fit in memory. The dictionary-backed corpora are the exception: they wrap + data the caller already holds. - For files, ZIPs, and streams, preserve entry and byte limits, path validation, closing, and actionable errors. Be explicit about who owns a handle passed in. - A row that produces no text still carries metadata. A verse range matched by diff --git a/docs/review/devils-advocate.md b/docs/review/devils-advocate.md index 38a0a6c7..84d84d84 100644 --- a/docs/review/devils-advocate.md +++ b/docs/review/devils-advocate.md @@ -20,7 +20,7 @@ failed before are: - a USFM or reference change whose test proves the happy path only; - a parity claim about `sillsdev/machine` with no checked comparison; - a public API change called internal because every caller in this repo still - compiles; + works; - a build-job change asserted to be safe under failure without a run that failed. Close with `Top risk`, `Evidence still needed`, and `Would this block merge?`. diff --git a/docs/review/jobs.md b/docs/review/jobs.md index 0ca70573..b8f27b81 100644 --- a/docs/review/jobs.md +++ b/docs/review/jobs.md @@ -9,8 +9,8 @@ This code runs unattended against remote services, so review it for what happens when something else fails, not for the successful build. - A job that is cancelled, preempted, or killed must end without raising - (`daa1b8e`). Check that cancellation is observed at every await or long loop, - not only at the top. + (`daa1b8e`). A new long-running step checks for cancellation, not only the + start of the job. - Memory is a real limit here (`4faa596`). For a change that grows a batch, a cache, or an in-memory corpus, say what bounds it. - Remote storage and ClearML calls fail transiently (`de29377`). A retry must be @@ -18,7 +18,5 @@ when something else fails, not for the successful build. - Output is a contract. Pretranslations, alignments, and their indices are consumed by Serval, so a change to their shape or ordering is a breaking change even though nothing in this repo reads them (`0354cc7`). -- This subpackage needs the `jobs` extra and is not installed by a plain - `pip install sil-machine`. A new import must not leak into the base package. - Model and trainer changes belong with a measured artifact, not an assertion (`50cb1de`). diff --git a/docs/review/machine-library.md b/docs/review/machine-library.md index ffff8d31..45f4b01b 100644 --- a/docs/review/machine-library.md +++ b/docs/review/machine-library.md @@ -5,10 +5,9 @@ comparison, and resource ownership.* Governs any `machine/**/*.py` no more specific rules file claims. -- This is shipped library code, published as `sil-machine`. The names a - subpackage re-exports through `__all__` are its public surface. Check the - shape of a changed signature, its defaults, and its return type; an in-repo - clean rename is still a breaking change for a downstream caller (`deb112b`). +- Check the shape of a changed exported signature, its defaults, and its return + type; an in-repo clean rename is still a breaking change for a downstream + caller (`deb112b`). - Watch for a change that fails silently rather than loudly. Widening a parameter from a mapping to an iterable of mappings still accepts the old positional argument and iterates its keys; a keyword-only parameter, or a @@ -19,11 +18,9 @@ Governs any `machine/**/*.py` no more specific rules file claims. - Close streams, models, engines, and trainers according to their contracts. Prefer a context manager to a `close` the caller must remember, and state who owns a handle that is passed in. -- Type hints are checked by pyright in `basic` mode, so an annotation that - passes is not evidence the types are sound. Review a changed annotation for a - false promise; do not ask for a repository-wide migration. -- Optional dependencies are real. Code reachable from a plain install must not - import from the `huggingface`, `thot`, `sentencepiece`, or `jobs` extras at +- Review a changed annotation for a false promise; do not ask for a + repository-wide typing migration. +- Code reachable from a plain install must not import an optional extra at module scope. - Add focused tests for the decisions the diff changes. Report the commands you ran; a coverage percentage is not evidence. diff --git a/docs/review/machine-tests.md b/docs/review/machine-tests.md index 9977cf21..9127b316 100644 --- a/docs/review/machine-tests.md +++ b/docs/review/machine-tests.md @@ -8,9 +8,8 @@ Governs `tests/**/*.py`. - A test must exercise the changed behavior, not merely execute the changed function. Identify the branches, guards, ordering, error paths, and limits in the production diff, and name which test covers each. -- Prefer a focused pytest function in the matching `tests//` module. Keep - fixtures deterministic: no sleeps, no ambient machine state, no dependence on - filesystem ordering. +- Prefer a focused test. Keep fixtures deterministic: no sleeps, no ambient + machine state, no dependence on filesystem ordering. - For string, token, marker, and USFM behavior, include the Unicode, empty, malformed, and nested cases. Assert over the parsed structure where the contract is structural; a comparison of rendered text can pass while the @@ -18,8 +17,4 @@ Governs `tests/**/*.py`. - A test that only round-trips input to output proves serialization, not behavior. When the change is about what a row or block carries, assert on that value directly. -- Some tests are skipped rather than failed when an optional dependency is - absent. A green run on one platform is not a green run everywhere - say which - platform produced the result you are reporting. -- Name the test that proves the change. "Where is the test?" is the single most - common review question here; answer it before it is asked. +- Name the test that proves the change. diff --git a/docs/review/punctuation.md b/docs/review/punctuation.md index bfd085e7..3dd4eb28 100644 --- a/docs/review/punctuation.md +++ b/docs/review/punctuation.md @@ -9,10 +9,8 @@ This area's history is almost entirely crash fixes reported from live data, so review it for the input that should not have reached the code rather than for the happy path. -- Index by text element, not by code unit. A surrogate pair, a combining mark, - or a multi-code-point quotation mark must not split. Python strings hide the - surrogate problem that other runtimes surface, which makes the combining-mark - case easier to miss, not less likely. +- Index by text element, not by code point. A combining mark or a + multi-code-point quotation mark must not split. - Assume the chapter or verse is missing, out of range, or unparsable. The extractor runs over real Paratext projects: `53992c8` fixed chapter numbers that came back wrong and `ca37757` fixed a crash on an invalid chapter. @@ -23,6 +21,5 @@ the happy path. another; a case-insensitive or locale-aware comparison here is a bug. - Quote convention detection reads whole projects and must degrade rather than raise when a book is absent or unreadable (`0c3cd9c`, `d93dacd`). -- Add a fixture for the malformed case with the fix, in - `tests/punctuation_analysis/`. Every fix above came from live data, not from - review. +- Add a fixture for the malformed case with the fix. Every fix above came from + live data, not from review. From ec9f7dc19d01c3f06908c50d5db8eaa1fb3b9c51 Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 15:39:05 -0400 Subject: [PATCH 07/12] Post automated reviews in the pr-review format The built-in code-review skill still finds and verifies the findings, but now reports back instead of posting, and the pr-review skill sets what is posted: numbered findings, severity labels, and the summary comment. That summary needs gh pr comment, so it joins the allowlist. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/claude-code-review.yml | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 8aa61266..cdc4eeb7 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -66,14 +66,17 @@ jobs: uses: anthropics/claude-code-action@v1 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - # The built-in code-review skill, at "high" effort. The code-review plugin - # this replaces discarded anything scoring under 80 on its confidence rubric, - # which filtered out design and public-API findings that don't change behavior. - prompt: '/code-review high --comment ${{ github.event.pull_request.number }}' + # The built-in code-review skill finds and verifies, at "high" effort; the + # plugin it replaces dropped anything under 80 on its confidence rubric. + # Without --comment it reports back, and pr-review sets what gets posted. + prompt: | + Review pull request ${{ github.event.pull_request.number }}. + Run /code-review high on it without --comment, then post the verified + findings as the pr-review skill describes. # v1 has no `model` input; the model is passed through to the CLI instead. claude_args: >- --model claude-opus-5 - --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr view:*),Bash(gh pr diff:*),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 api:*),Bash(gh search:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*),Bash(git diff:*),Bash(poetry run:*),Bash(.venv/bin/python:*)" # 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 From 171cd29e4c7630dee48afa91087e6920b6fca061 Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 15:42:43 -0400 Subject: [PATCH 08/12] Make pr-review run the whole review pr-review now gets its findings from the built-in code-review skill unless the caller already has them, so /pr-review works on its own, locally or in CI. The workflow prompt becomes a single /pr-review, and the two-step sequence lives in a skill the action restores from main rather than in the prompt. Co-Authored-By: Claude Opus 5.5 (1M context) --- .claude/skills/pr-review/SKILL.md | 6 +++++- .github/workflows/claude-code-review.yml | 11 ++++------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 6096ac77..20888942 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.py - short line comments, evidence, severity. +description: Review a pull request in sillsdev/machine.py - 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 --- @@ -11,6 +11,10 @@ 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. +Unless verified findings are already in hand, get them first with +`/code-review high `, without `--comment`: it finds and verifies, and +this skill decides what gets posted. + ## 1. One finding, one comment Anchor it on the line. Two problems on one line are two comments. A reviewer diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index cdc4eeb7..230db188 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -66,13 +66,10 @@ jobs: uses: anthropics/claude-code-action@v1 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} - # The built-in code-review skill finds and verifies, at "high" effort; the - # plugin it replaces dropped anything under 80 on its confidence rubric. - # Without --comment it reports back, and pr-review sets what gets posted. - prompt: | - Review pull request ${{ github.event.pull_request.number }}. - Run /code-review high on it without --comment, then post the verified - findings as the pr-review skill describes. + # 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. + prompt: '/pr-review ${{ github.event.pull_request.number }}' # v1 has no `model` input; the model is passed through to the CLI instead. claude_args: >- --model claude-opus-5 From d939dcfb7554a85a8e031f26cfc0b6ba720e0c9c Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 15:47:10 -0400 Subject: [PATCH 09/12] Run the automated review on the latest Opus Use the opus alias instead of pinning a model ID, so the review picks up new Opus releases without a workflow change. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/claude-code-review.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 230db188..92a9884f 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -71,8 +71,10 @@ jobs: # dropped anything under 80 on its confidence rubric. prompt: '/pr-review ${{ github.event.pull_request.number }}' # v1 has no `model` input; the model is passed through to the CLI instead. + # 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 claude-opus-5 + --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:*)" # 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 From 1be63fff60f93185fcf3e16bff694f695f87774b Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 16:02:32 -0400 Subject: [PATCH 10/12] Set Reviewable dispositions from review severity Reviewable sets a discussion's disposition from a shorthand keyword at the start of a comment posted on GitHub, so the severity labels become its words: Major blocks, Minor stays open, FYI starts resolved. The summary starts with FYI so it never blocks on its own. A test on #377 confirmed the leading keyword works; a disposition on its own line does not, on a new comment. Co-Authored-By: Claude Opus 5.5 (1M context) --- .claude/skills/pr-review/SKILL.md | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 20888942..63e60447 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -20,19 +20,19 @@ this skill decides what gets posted. 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 start each -comment with its number, 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 +Number findings `F1`, `F2`, ... in the order you post them, and put the number +right after the severity word, 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 -After the number and severity, the first sentence names the defect. Evidence +After the severity and number, the first sentence names the defect. Evidence second, fix third, if it fits. ``` -F1 Important. 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. +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. Collect a list. ``` @@ -41,14 +41,22 @@ the summary instead. ## 3. Label the severity -**Critical** blocks merge, then **Important**, then **Minor**. Critical means -demonstrated: a failing command, a broken contract, a missing gate. A worry is -not Critical. +The first word of every comment is its severity. Reviewable reads it from a +comment posted on GitHub and sets the discussion's disposition to match: + +- **Major** blocks review completion until a maintainer dismisses it. Major + means demonstrated: a failing command, a broken contract, a missing gate. A + worry is not Major. +- **Minor** stays open until the author answers it. +- **FYI** starts resolved: worth knowing, no answer needed. + +A comment that starts with any other word gets Reviewable's default instead, +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 Major. - Do not report pre-existing issues the diff does not touch. - Do not ask for a migration, modernization, or benchmark the diff gave no @@ -62,6 +70,9 @@ confirm `Unverified`; an unverified concern never blocks a merge. ## 5. Close with five lines +Start the summary comment with `FYI`, so it never holds up the review on its +own; the findings carry their own severity. + 1. Verdict: approve, approve with fixes, or request changes. 2. The one thing that matters most, with its number and `path:line`. 3. Counts by severity. From 08b6ac8d08351015763d47c861042cda1d1cd436 Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 16:36:18 -0400 Subject: [PATCH 11/12] Drop the FYI prefix from the review summary A top-level PR comment lands in Reviewable's main discussion, which has no dispositions and is always resolved, so the prefix did nothing. Co-Authored-By: Claude Opus 5.5 (1M context) --- .claude/skills/pr-review/SKILL.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 63e60447..adb44d35 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -70,9 +70,6 @@ confirm `Unverified`; an unverified concern is never Major. ## 5. Close with five lines -Start the summary comment with `FYI`, so it never holds up the review on its -own; the findings carry their own severity. - 1. Verdict: approve, approve with fixes, or request changes. 2. The one thing that matters most, with its number and `path:line`. 3. Counts by severity. From 351f8946750b785d0c74469c5785baf138efff67 Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Tue, 22 Sep 2026 16:40:58 -0400 Subject: [PATCH 12/12] Keep Reviewable keywords to posted finding comments Findings are Critical, Important, or Low. Only a finding comment posted to the PR starts with the Reviewable keyword for its severity, now followed by a colon; the summary, replies, and unposted reviews use the severity names, because Minor reads as trivial and marks an Important finding. Co-Authored-By: Claude Opus 5.5 (1M context) --- .claude/skills/pr-review/SKILL.md | 38 +++++++++++++++++++------------ 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index adb44d35..b7041d3f 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -21,18 +21,18 @@ 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 severity word, 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 +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 -After the severity and number, the first sentence names the defect. Evidence +After the keyword and number, the first sentence names the defect. Evidence second, fix third, if it fits. ``` -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. +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. Collect a list. ``` @@ -41,22 +41,30 @@ the summary instead. ## 3. Label the severity -The first word of every comment is its severity. Reviewable reads it from a -comment posted on GitHub and sets the discussion's disposition to match: +Every finding is **Critical**, **Important**, or **Low**. Critical means +demonstrated: a failing command, a broken contract, a missing gate. A worry is +not Critical. Important needs an answer from the author; Low is worth knowing +and needs none. -- **Major** blocks review completion until a maintainer dismisses it. Major - means demonstrated: a failing command, a broken contract, a missing gate. A - worry is not Major. -- **Minor** stays open until the author answers it. -- **FYI** starts resolved: worth knowing, no answer needed. +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: -A comment that starts with any other word gets Reviewable's default instead, -which for a reviewer is Blocking. +| 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 is never Major. +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