Improve the Claude code review workflow

.claude/skills/code-comments/SKILL.md

@@ -0,0 +1,64 @@
+---
+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**.
+
+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.

.claude/skills/commit-messages/SKILL.md

@@ -0,0 +1,21 @@
+---
+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.
+
+The `(#N)` suffix in the history is added by GitHub when a pull request is
+squashed. Never type it into a local commit.
+
+Do not rewrite shared history to fix a message on a pushed branch - add a
+corrective commit unless the author asks for the rewrite.

.claude/skills/issue-authoring/SKILL.md

@@ -0,0 +1,52 @@
+---
+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.
+
+## 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.
+
+Hand back the title, labels, and body. The author decides whether to publish.

.claude/skills/pr-authoring/SKILL.md

@@ -0,0 +1,68 @@
+---
+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 <n> --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
+
+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
+
+Everything longer goes under a `---`, in closed `<details>` 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, and never call a local run CI-equivalent. Name any
+check you skipped.
+
+## 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.

.claude/skills/pr-review/SKILL.md

@@ -0,0 +1,94 @@
+---
+name: pr-review
+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
+---
+
+# 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.
+
+Unless verified findings are already in hand, get them first with
+`/code-review high <target>`, without `--comment`: it finds and verifies, and
+this skill decides what gets posted.
+
+## 1. One finding, one comment
+
+Anchor it on the line. Two problems on one line are two comments. A reviewer
+scrolling the diff should meet each point where it applies.
+
+Number findings `F1`, `F2`, ... in the order you post them, and put the number
+right after the keyword, so a reply or a later review can refer to one without
+quoting it. Not `#1`: GitHub links that to issue 1. Numbers are stable - a
+withdrawn finding keeps its number, and a later round continues the sequence.
+
+## 2. Lead with the claim
+
+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.
+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
+
+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.
+
+A finding comment posted to the PR starts with the Reviewable keyword for its
+severity, followed by a colon. Reviewable reads it and sets the discussion's
+disposition:
+
+| Severity | Comment starts with | Disposition in Reviewable |
+| --- | --- | --- |
+| Critical | `Major:` | Blocking, until a maintainer dismisses it |
+| Important | `Minor:` | Discussing, open until the author answers |
+| Low | `FYI:` | Informing, starts resolved |
+
+Use the keywords there and nowhere else. The summary, replies, and a review
+that is not posted use the severity names: `Minor` reads as trivial, and an
+Important finding is not. A finding comment that starts with any other word gets
+Reviewable's default, which for a reviewer is Blocking.
+
+## 4. Carry the evidence
+
+Every comment gets a `path:line` and a consequence. Mark what you did not
+confirm `Unverified`; an unverified concern 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
+  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.
+- 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 number and `path:line`.
+3. Counts by severity.
+4. What you ran, and its result.
+5. What you could not verify.
+
+Say which public API, optional dependency, published-wheel surface, or parity
+contract with `sillsdev/machine` changed, or `None verified`.
+
+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`.

.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

.github/ISSUE_TEMPLATE/config.yml

@@ -0,0 +1 @@
+blank_issues_enabled: true