From 458da3dbafb7450173ee23feb71c99a1671f0420 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 17 Sep 2026 17:02:30 -0400 Subject: [PATCH 01/10] Add agent guidance, authoring and review skills, and templates Introduce the shared agent context layer for this repository, modelled on the layering used in sillsdev/FieldWorks but scoped to a cross-platform library rather than a Windows desktop application. AGENTS.md is the repository-wide operational source of truth: layout, target frameworks, the local_check.sh sequence, the SentencePiece4c native boundary, the current CI workflow, the stale AppVeyor WebApi paths, and the machine.py porting relationship. CLAUDE.md imports it. CONTEXT.md is a terminology and relationship layer for corpora, rows, tokenization, Scripture references, translation engines and models, word alignment, and HermitCrab morphology, with each term anchored to a current path under src/. Four Claude skills cover pull request authoring, issue authoring, commit messages, and evidence-first pull request review. Four path-scoped Copilot instruction files attach review rules to the library, tests, HermitCrab, and corpora/USFM trees, and a read-only devil's advocate role is available for high-risk changes. The GitHub pull request template and issue forms mirror the same evidence contract, and .gitignore now excludes the local .review/ directory that the authoring skill writes to. No build, test, or CI behaviour changes. Co-Authored-By: Claude Opus 5 --- .claude/skills/commit-messages/SKILL.md | 41 ++++ .claude/skills/issue-authoring/SKILL.md | 86 ++++++++ .claude/skills/pr-authoring/SKILL.md | 197 +++++++++++++++++ .claude/skills/pr-review/SKILL.md | 186 ++++++++++++++++ .github/ISSUE_TEMPLATE/bug_report.yml | 55 +++++ .github/ISSUE_TEMPLATE/config.yml | 1 + .github/ISSUE_TEMPLATE/feature_request.yml | 44 ++++ .github/ISSUE_TEMPLATE/porting_request.yml | 38 ++++ .github/PULL_REQUEST_TEMPLATE.md | 34 +++ .github/agents/devils-advocate.agent.md | 21 ++ .github/copilot-instructions.md | 28 +++ .../corpora-usfm-review.instructions.md | 21 ++ .../hermitcrab-review.instructions.md | 24 ++ .../machine-library-review.instructions.md | 26 +++ .../machine-tests-review.instructions.md | 24 ++ .gitignore | 3 + AGENTS.md | 106 +++++++++ CLAUDE.md | 9 + CONTEXT.md | 207 ++++++++++++++++++ 19 files changed, 1151 insertions(+) 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/agents/devils-advocate.agent.md create mode 100644 .github/copilot-instructions.md create mode 100644 .github/instructions/corpora-usfm-review.instructions.md create mode 100644 .github/instructions/hermitcrab-review.instructions.md create mode 100644 .github/instructions/machine-library-review.instructions.md create mode 100644 .github/instructions/machine-tests-review.instructions.md create mode 100644 AGENTS.md create mode 100644 CLAUDE.md create mode 100644 CONTEXT.md diff --git a/.claude/skills/commit-messages/SKILL.md b/.claude/skills/commit-messages/SKILL.md new file mode 100644 index 000000000..72f3d9a55 --- /dev/null +++ b/.claude/skills/commit-messages/SKILL.md @@ -0,0 +1,41 @@ +--- +name: commit-messages +description: Use before writing a commit message in sillsdev/machine; apply the repository commit conventions and check the new commit range before pushing. +--- + +# Commit messages + +Write a concise, imperative, sentence-case subject that names the actual change. +Recent history is descriptive and GitHub-native, for example: + +- `Fix bug in MergeEquivalentAnalyses (#493)` +- `Use StringComparison.Ordinal when locating token indices in PlaceMarkersUsfmUpdateBlockHandler (#496)` +- `Port changes from sillsdev/machine.py#336 (#498)` + +## Conventions + +- Keep the subject under about 72 characters when you can. This is not enforced + by CI, and existing history contains longer subjects, so do not rewrite shared + history to satisfy it. +- No trailing period or other terminal punctuation on the subject. +- No leading, trailing, or interior tab and trailing-whitespace damage. +- If a body is present, leave one blank line after the subject and wrap body + lines at about 80 characters. +- Explain what changed and why. Reference a GitHub issue when one exists. +- The `(#N)` suffix is added by GitHub when a pull request is squashed. Do not + add it by hand to an ordinary local commit. +- Older commits use Jira identifiers such as `LT-22605`. That convention is + historical; use a GitHub issue reference for new work unless a maintainer asks + otherwise. + +## Check the range before pushing + +``` +git fetch origin --quiet +git log --check --pretty=format:'--- %h %s' origin/master..HEAD +``` + +`git log --check` reports whitespace damage in the commits you are about to +push. A failed check is not a pass. Do not rewrite a pushed or shared branch to +fix a message; add a corrective commit unless the author explicitly authorizes +the rewrite. diff --git a/.claude/skills/issue-authoring/SKILL.md b/.claude/skills/issue-authoring/SKILL.md new file mode 100644 index 000000000..aad7854cf --- /dev/null +++ b/.claude/skills/issue-authoring/SKILL.md @@ -0,0 +1,86 @@ +--- +name: issue-authoring +description: Use when creating, refining, or triaging a GitHub issue in sillsdev/machine; produce evidence-based bug, feature, or machine.py porting issues without inventing Jira requirements. +argument-hint: Optional issue type, title, symptoms, acceptance criteria, or source PR +user-invocable: true +--- + +# Issue Authoring + +Use GitHub issues as the native issue system for sillsdev/machine. This skill +supports Bug, Feature, and Porting issues. It does not fetch, assign, transition, +or comment on Jira tickets. An LT- reference may be included as an external +reference only when supplied and verified. + +## Common intake and evidence gate + +1. Search existing open and recently closed GitHub issues for duplicates and + related PRs before drafting. +2. Ask for the smallest concrete example distinguishing a problem from an + enhancement request. +3. Separate observed facts, reproduction/acceptance evidence, and hypotheses. +4. Remove secrets, tokens, private data, and unsanitized customer/project data. +5. Name affected version/commit, OS, architecture, runtime, and package when + known. +6. If a fact is unknown, write Unknown and identify how to verify it. + +An issue is ready when another maintainer can reproduce the bug, evaluate the +feature acceptance criteria, or identify the exact source change to port. + +## Bug issue + +Require: + +* concise symptom and affected package/API; +* version or commit, OS, architecture, and .NET runtime; +* minimal input, fixture, code sample, or repository state; +* exact reproduction steps and frequency; +* expected result and actual result; +* sanitized exception/log output; +* regression range or not known; and +* a minimal regression-test idea. + +If automation is feasible, propose a failing test before implementation and name +the likely test project. If not, state the concrete reason--visual/manual +behavior, unavailable external service, or packaging infrastructure--and give an +alternative verification plan. + +## Feature issue + +Require: + +* user/problem statement and affected consumers; +* use cases and non-goals; +* proposed behavior or API, including compatibility concerns; +* observable acceptance criteria; +* test strategy and representative edge cases; +* performance/resource/platform constraints; and +* deliberately excluded follow-up work. + +Do not prescribe an implementation before behavior and acceptance criteria are +clear. Use Fixes #N only when closing the issue on merge is intended. + +## machine.py porting issue + +Include source repository and PR/commit URL, target behavior to port, what is +not relevant to machine, verified target projects/files if known, +compatibility/test implications, and source validation evidence or an explicit +gap. + +The existing merged-PR workflow normally creates the opposite-repository issue +with title Port '', label porting, and this body: + + Port any relevant changes in from to . + + + +Do not duplicate that issue. If a closing issue reference already contains the +marker, the workflow skips another generated issue. If the port is not covered +by a merged PR, create a normal Porting issue using the fields above. + +## Handoff + +Return proposed title, type/labels, complete body, duplicate-search result, +evidence gaps, and links. The author decides whether to publish it. Do not +claim that a test, reproduction, or external issue was checked unless it was +actually checked. diff --git a/.claude/skills/pr-authoring/SKILL.md b/.claude/skills/pr-authoring/SKILL.md new file mode 100644 index 000000000..c6f21b3bb --- /dev/null +++ b/.claude/skills/pr-authoring/SKILL.md @@ -0,0 +1,197 @@ +--- +name: pr-authoring +description: Use when preparing, opening, updating, or responding to a GitHub pull request in sillsdev/machine; verify branch hygiene and actual validation evidence, then compose a concise reviewer-ready PR body. +argument-hint: Optional branch purpose, issue number, or PR number +user-invocable: true +--- + +# PR Authoring + +Use this as the single entrypoint for preparing or updating a pull request in +sillsdev/machine. It prepares evidence and copy; it does not claim that a check +ran when it did not. Do not push, create, edit, or close a PR unless the user +explicitly requested that operation or confirmed a readiness offer. + +## 1. Announce the stages + +Tell the author that this run will: + +1. inspect the branch and target diff; +2. review contracts, implementation, tests, CI/dependencies, and packaging risk + appropriate to the changed files; +3. ask about Important/Critical risks and validation gaps; +4. run or record the exact applicable checks; and +5. prepare the PR body and review handoff. + +If a finding is ambiguous, ask for the missing product or design decision. If the +author cannot explain a changed mechanism, record the area as an understanding +gap instead of silently treating the change as safe. + +## 2. Branch and working-tree gate + +Run these read-only checks first: + +~~~text +git branch --show-current +git status --short +git symbolic-ref --short refs/remotes/origin/HEAD +git fetch origin --quiet +git merge-base origin/master HEAD +git diff --name-status origin/master...HEAD +git log --check --pretty=format:"---% h% s" origin/master.. +git diff --check origin/master...HEAD +~~~ + +The verified target default is origin/master. If the remote default changes, use +the resolved remote default and state it in the report; never silently use main. +Stop if the current branch is master and the request is to open a feature PR. +Preserve unrelated changes and pre-existing untracked files. Never use +reset --hard, checkout --, broad deletion, or broad staging as a cleanup +shortcut. + +Record merge base, HEAD, changed-file count, branch purpose, and pre-existing +work. A local review summary may be written to .review/, which .gitignore +excludes. Never commit it, and keep other transient notes outside the repo. + +## 3. Review the diff against the target's actual shape + +Use the smallest relevant set of passes: + +* **Contract/API:** public signatures, serialization, compatibility, package + metadata, and behavior visible to callers. +* **Implementation:** correctness, error handling, resource bounds, + cancellation/concurrency, determinism, and edge cases. +* **Tests and fixtures:** regression coverage, expected/actual behavior, + deterministic fixtures, test isolation, and platform assumptions. +* **Build/CI/dependencies:** project references, native SentencePiece boundary, + dependency changes, CI matrix, and NuGet packing when relevant. + +For a change under src/sentencepiece4c, include the exact matrix-specific CMake +commands from .github/workflows/ci.yml in required validation. For package or +project-file changes, inspect the package job's eight explicit dotnet pack +projects and say which package outputs are affected. Do not import FieldWorks +desktop, COM, installer, localization, or Jira rules into this review. + +Verify every named type, method, project, test, path, and count against the +current tree before putting it in the body. If a draft document conflicts with +code, correct the document or report the uncertainty; do not make code fit +stale prose. + +Classify findings as Critical, Important, or Minor. Keep positive observations +and evidence gaps separate. Deduplicate only identical concerns. + +## 4. Author interview + +Ask one Critical or Important question at a time: + +* What caller-visible behavior or contract changes? +* What makes the changed path safe for existing callers? +* Which test or fixture proves the reported bug or acceptance criterion? +* Which OS/runtime/package path is at risk? +* What was deliberately left out, and what would unblock it? + +For a vague answer, ask one focused follow-up and then record the concern as +unresolved if it remains unclear. For a small low-risk diff, do not manufacture +an interview; state that no interview was needed and why. + +## 5. Validation contract + +The authoritative managed CI sequence is: + +~~~text +dotnet tool restore +dotnet restore +dotnet csharpier check . +dotnet build --no-restore -c Release +dotnet test --verbosity normal --collect:"Xplat Code Coverage" +~~~ + +Run the sequence for a normal code/project change unless the author explicitly +chooses a narrower check and the report names omitted checks. local_check.sh is +a useful shortcut, but it does not collect coverage; do not call it +CI-equivalent. For native SentencePiece changes, also run the applicable Linux +or Windows CMake commands exactly as shown in ci.yml. For packaging changes, run +or record the affected dotnet pack ... -c Release -o artifacts command and +distinguish local results from the CI package job. + +For documentation/skill/template-only changes, run dotnet csharpier check . +only if C# files are touched; otherwise use git diff --check and available +YAML/Markdown diagnostics. Always state skipped checks and why. Never claim +manual validation unless it was directly performed or explicitly confirmed by +the author. + +## 6. PR body + +Write the body from a file. Keep the top zone concise: + +~~~markdown +## Quick summary + + + + + +## Where to look + +- -- +- -- + +## Deliberately not included + +- + +## Validation + +- +- + +## Issue / porting context + + +~~~ + +The top zone should normally be 200-400 words for a substantive PR, but clarity +beats an artificial count for a tiny change. Do not open with process narration, +apology, or "should be fine." Do not duplicate proof in every section. + +When durable reasoning matters, put it below a horizontal rule in closed details +sections: Reading this a year from now; Decisions, and why; Paths not taken; +What this does NOT authorize; Deferred, and what would unblock it; and Evidence. + +Synthesize reasoning; do not paste scratchpads. Do not automatically delete +markdown from the branch. If temporary research is intentionally removed, list +the files and obtain confirmation before deletion. + +## 7. Machine.py porting + +For a change ported from sillsdev/machine.py, identify the source PR/commit, +state which behavior is relevant to this repository, and include affected +tests. create-porting-issue.yml creates the opposite-repository issue after a +merged PR, with title Port '', label porting, and an +AUTO-GENERATED-ISSUE marker. Do not create a duplicate by hand when that +workflow has already created one. + +## 8. Review comments + +For each Copilot or human comment, classify it as: + +* **Fix** -- technically sound and unambiguous; make the minimum change. +* **Clarify** -- missing context or a design decision; ask the specific question. +* **Reply only** -- explain verified behavior without changing code. +* **Defer** -- record the follow-up and why it is outside this PR. + +Validate fixes with applicable commands. Reply in the anchored GitHub thread. +Resolve only an unresolved thread that the API permits resolving and that is +fully answered with no open question. Do not resolve disputed, ambiguous, +unverified, or deferred comments. + +## 9. Readiness handoff + +Before offering to publish, report branch, base, merge base, changed files, +findings/status, exact checks run/skipped/red, issue/porting links, +commit-message/whitespace status, and reviewer focus. + +Only after confirmation may the workflow stage intended files, commit, push, +create/update the PR, or update its body. Preserve unrelated staged changes and +ask before including them. diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md new file mode 100644 index 000000000..d0c5c2553 --- /dev/null +++ b/.claude/skills/pr-review/SKILL.md @@ -0,0 +1,186 @@ +--- +name: pr-review +description: Evidence-first review of machine pull requests and branch diffs, focused on public library compatibility, deterministic language processing, async contracts, HermitCrab performance, tests, and packaging. +argument-hint: "[optional PR number, branch, or review focus]" +--- + +# Machine PR Review + +Review the current machine branch or named pull request. This is a read-only review. Do +not edit, commit, push, resolve comments, or open a PR unless a separate workflow +explicitly authorizes that action. + +## Review contract + +- Review the actual branch diff from the merge-base with the PR base, normally + `origin/master`, not today's commits and not filenames alone. +- Read the changed code, relevant tests, project files, repository instructions, and the + directly affected public callers or fixtures before making a finding. +- Use only findings grounded in code, a reproducible command, a test/result, a cited + history change, or a cited consumer contract. +- Every finding must include `path:line` and a concrete consequence. +- Mark each statement as `Verified` or `Unverified follow-up`. An unverified concern is never a blocking finding. +- Do not report a pre-existing issue unless the diff changes its behavior, exposes it + through a changed contract, or the review explicitly asks for a baseline audit. +- Do not request a broad modernization (nullable migration, async streaming, + language-version change, analyzer cleanup, or benchmark suite) without a changed-code + reason. + +## Establish scope + +1. Confirm the repository, branch, worktree, and base. Use `git status --short + --branch`, `git merge-base HEAD`, and `git diff --stat ...HEAD`. +2. Read `README.md`, `.editorconfig`, `.csharpierrc.yaml`, the relevant solution/project + files, and any target-local instructions. +3. Classify changed files: shipped API/library, tests, HermitCrab, corpus/USFM, + SentencePiece/native, tool, workflow/package, or documentation. +4. Apply only the review axes and path-scoped instructions relevant to that classification. +5. Search for public declarations, interface implementations, project references, + package metadata, tests, fixtures, and downstream references before concluding that a + contract changed. + +## Review axes + +### Public API, binary compatibility, and package contract + +For changes under shipped library projects, inspect public/protected types and members, +overloads, optional parameters, interface shape, return types, serialized models, target +frameworks, package references, assembly/package versioning, and XML documentation. A +removal, narrowing, incompatible default/behavior, or accidental target/package change +is a finding only when the diff and contract establish the consequence. Check +implementations and tests of changed interfaces. Do not infer downstream breakage from a +public-looking filename. + +### Nullable annotations + +The current shipped libraries do not opt into nullable reference types, while test +projects do. Review `#nullable`, project settings, `?`, null-forgiving operators, and +generated/public annotations when touched. Flag an annotation that promises a false null +contract or creates inconsistent implementation/interface behavior. Do not require a +repository-wide NRT migration as part of an unrelated change. + +### Async, cancellation, and concurrency + +The public API uses `Task` and optional `CancellationToken`; corpus/tokenizer APIs are +synchronous, and the current source has no `IAsyncEnumerable` surface. For changed async +code, verify token propagation to every meaningful wait/I/O/dataflow operation, ordering +and partial-result behavior, disposal/pool lifetime, and absence of sync-over-async +deadlocks. Check `ConfigureAwait(false)` where library context capture is not intended. +A new async-streaming API requires an explicit compatibility and consumer decision. + +### Determinism and culture + +For token, marker, identifier, persisted, protocol, dictionary-order, and serialization +logic, verify explicit comparison/equality/culture choices. Use ordinal comparison for +identity/search where the domain is byte/code-point identity; use invariant or +culture-aware behavior only when the semantic contract requires it. The `418ff225` +`StringComparison.Ordinal` fix and Hindi regression test are precedent, not a blanket +replacement rule. Add a non-English-culture test when the changed behavior can vary by +culture. + +### HermitCrab semantic/performance safety + +For `src/SIL.Machine.Morphology.HermitCrab/**`, verify that analysis-state keys include +every field read by rules, frozen/mutable objects cannot invalidate keys, replay +preserves analysis results and prefixes, memo entries are complete before storage, and +resource/parallelism caps remain effective. Review allocations and retained lifetimes in +inner loops when changed. Require focused semantic tests for behavior changes and a +benchmark or measured artifact when the PR claims performance improvement. Do not accept +hit-count improvement as proof of semantic equivalence. + +### Python parity + +For a port or algorithm explicitly shared with `machine.py`, require a cited Python +source/commit/issue and paired behavior evidence. The post-merge porting workflow is +follow-up coordination, not proof. If the Python source is unavailable, record parity as +`Unverified follow-up`; do not invent a comparison or block without a stated parity +contract. + +### Tests and coverage + +Map changed branches, guards, error paths, ordering, resource limits, and side effects +to tests. Prefer focused tests in the affected test project, including Unicode/culture, +cancellation, malformed input, and boundary cases where relevant. Run the smallest +meaningful command first and then the repository check when practical. Report exact +commands, results, filters, and exclusions. Coverage percentage alone is not evidence +that changed decisions are covered. + +### Formatting and repository checks + +Run `dotnet csharpier check .`, `dotnet build --no-restore -c Release`, and `dotnet test +--verbosity normal` as applicable. `local_check.sh` is the canonical combined local +sequence. Respect `.editorconfig` and CSharpier's 120-column configuration. Formatting +is normally an Important/Minor issue, never a Critical issue by itself. + +### USFM, ScriptureRef, and versification + +For `src/SIL.Machine/Corpora/**` changes involving USFM, ScriptureRef, `ScrVers`, +tokenization, or update handlers, trace input -> parse/tokenize -> reference mapping -> +output. Require paired fixtures or assertions for valid, empty, malformed, nested, +Unicode, and cross-versification cases appropriate to the change. Verify marker and +token identity comparisons are deterministic. Do not claim render parity; this library +has semantic text behavior, not FieldWorks desktop rendering. + +### Input, resource, and native boundary safety + +For changed streams, archives, paths, subprocess/tool inputs, or SentencePiece native +loading, check size limits, path/entry validation, disposal, error propagation, platform +selection, and secrets. Cite the changed boundary and test/evidence. Do not apply this +checklist to unrelated pure algorithms. + +## Validation + +- Prefer `./local_check.sh` (or `bash local_check.sh`) for the full local check when the environment supports it. +- Otherwise run the equivalent `dotnet tool restore`, `dotnet restore`, `dotnet + csharpier check .`, `dotnet build --no-restore -c Release`, and `dotnet test + --verbosity normal` commands. +- For focused behavior, run the relevant test project with an explicit filter and then + state whether the full matrix was run. +- Treat the visible GitHub `CI Build` result as evidence only when its commit SHA is the + reviewed head. This workflow is push/tag-triggered, not a `pull_request`-triggered + gate. +- Do not treat Codecov upload as a pass/fail threshold; inspect changed-line tests directly. +- If a command cannot run, state why and what remains unverified. Never convert unavailable evidence into a pass. + +## Output format + +### Contract/API Changes Summary + +State whether shipped public APIs, target frameworks, package metadata, native +artifacts, serialized formats, or parity contracts changed. Say `None verified` when +appropriate. + +### Findings + +#### Critical (must address) + +- `[Verified] path:line -- consequence; evidence/command; required correction.` + +Use only for demonstrated correctness, security/resource, compatibility, build/package, +or test-gate failures that block a safe merge. + +#### Important (should address) + +- `[Verified] path:line -- concrete risk or missing contract evidence; evidence/command; requested action.` + +#### Minor (consider) + +- `[Verified] path:line -- scoped maintainability/style/test improvement.` + +#### Unverified follow-ups + +- `[Unverified] concern -- exact evidence still needed; why it is not a finding.` + +### Positive Observations + +List concrete safeguards or tests that the diff preserves or adds. + +### Required Validation + +List commands with status (`passed`, `failed`, `not run`, or `blocked`), exact filters, artifacts, and remaining gaps. + +### Suggested Review Focus + +Name the one to three highest-value areas for a human reviewer. If an adversarial pass +is requested, use the separate library devil's-advocate role and keep its objections +evidence-linked. diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml new file mode 100644 index 000000000..7ff9f52e5 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -0,0 +1,55 @@ +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: input + id: package + attributes: + label: Affected package or API + description: Name the SIL.Machine package, project, or public API. + validations: + required: true + - type: input + id: version + attributes: + label: Version or commit + description: Include the package version or commit SHA. + validations: + required: true + - type: input + id: environment + attributes: + label: Environment + description: OS, architecture, and .NET runtime. + validations: + required: true + - type: textarea + id: reproduction + attributes: + label: Reproduction + description: Minimal input/fixture and exact steps, including frequency. + placeholder: | + 1. ... + 2. ... + Expected: ... + Actual: ... + validations: + required: true + - type: textarea + id: logs + attributes: + label: Logs or exception + 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 000000000..0086358db --- /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 000000000..801700ee6 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/feature_request.yml @@ -0,0 +1,44 @@ +name: Feature request +description: Propose a behavior or API improvement for SIL Machine +title: "[Feature]: " +labels: + - enhancement +body: + - type: textarea + id: problem + attributes: + label: Problem and users + description: Who needs this and what cannot they do today? + validations: + required: true + - type: textarea + id: use_cases + attributes: + label: Use cases + description: Give concrete workflows and representative inputs. + validations: + required: true + - type: textarea + id: proposal + attributes: + label: Proposed behavior or API + description: Describe observable behavior; include compatibility concerns. + validations: + required: true + - type: textarea + id: non_goals + attributes: + label: Non-goals and deferred work + description: State what this request does not include. + - type: textarea + id: acceptance + attributes: + label: Acceptance criteria and tests + description: List observable outcomes, edge cases, and the test strategy. + validations: + required: true + - type: textarea + id: constraints + attributes: + label: Compatibility, performance, and platform constraints + description: Include package/API, resource, OS, or runtime constraints. diff --git a/.github/ISSUE_TEMPLATE/porting_request.yml b/.github/ISSUE_TEMPLATE/porting_request.yml new file mode 100644 index 000000000..ad3b8a571 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/porting_request.yml @@ -0,0 +1,38 @@ +name: Port from machine.py +description: Track relevant behavior to port from sillsdev/machine.py +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: input + id: source_pr + attributes: + label: machine.py 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 projects and compatibility + description: Name verified target projects/APIs, 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/acceptance 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 000000000..dca93775b --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,34 @@ +## Quick summary + + + +## Issue / porting context + + + +## What changed + + + +## Deliberately not included + + + +## Validation + +- [ ] dotnet tool restore +- [ ] dotnet restore +- [ ] dotnet csharpier check . +- [ ] dotnet build --no-restore -c Release +- [ ] dotnet test --verbosity normal +- [ ] ./local_check.sh --agent-strict (required for agents; optional for humans) +- [ ] Applicable SentencePiece CMake build run, or not relevant +- [ ] Applicable package output checked, or not relevant +- [ ] git diff --check and commit-range whitespace checked + + + +## Reviewer focus + + diff --git a/.github/agents/devils-advocate.agent.md b/.github/agents/devils-advocate.agent.md new file mode 100644 index 000000000..4e4d6a5ad --- /dev/null +++ b/.github/agents/devils-advocate.agent.md @@ -0,0 +1,21 @@ +--- +name: machine-devils-advocate +description: Read-only adversarial second pass for machine PR reviews, focused on compatibility, determinism, parity, memoization safety, and evidence gaps. +tools: ['read', 'search'] +--- + +# Machine Devil's Advocate + +Act as a skeptical senior reviewer after the normal machine PR review. Read the merge-base diff, the normal review output, relevant tests, project files, and cited history. Do not edit files, commit, push, or propose an unbounded rewrite. + +Challenge the most consequential conclusion first. Ask one objection at a time and support it with `path:line`, a concrete execution scenario, a consumer/fixture contract, or a missing command/artifact. Focus on: + +- public API/source/binary compatibility and target/package changes; +- false nullable promises or inconsistent interface implementations; +- cancellation, ordering, disposal, pooling, and parallel/sequential divergence; +- culture-sensitive token/marker/USFM behavior and Unicode regressions; +- incomplete HermitCrab keys, unsafe replay, retained-memory bounds, or unsupported performance claims; +- Python-port parity claims without a checked comparison; and +- tests that pass while leaving changed decisions or failure paths unverified. + +Do not call a concern a finding unless the evidence is present. Label each item `Verified objection` or `Unverified question`. Do not supply the solution in the objection section; state the evidence needed to settle it. Finish with `Top risk`, `Evidence still needed`, and `Would this block merge?` with a reason tied to the normal review severity contract. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 000000000..c608707f8 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,28 @@ +# Copilot guidance for machine + +Read `AGENTS.md` at the repository root for operational rules and `CONTEXT.md` +for domain vocabulary. Those are the shared sources of truth; do not restate +their rules here. Path-scoped review rules live in +`.github/instructions/*.instructions.md` and attach automatically to matching +files. + +Before making or reviewing a change, inspect the current source, project files, +tests, `local_check.sh`, and `.github/workflows/ci.yml`. Use actual code and +current configuration as evidence. A search that finds nothing is not proof that +a concept is absent unless you state the scope you searched. + +Pay particular attention to: + +- `netstandard2.0` library compatibility versus `net10.0` tools and tests; +- corpus row, tokenization, Scripture-reference, and alignment semantics; +- explicit string-comparison and culture choices in marker, token, and + identifier logic; +- the SentencePiece4c native boundary and its platform artifacts; +- disposal and lifetime of engines, models, trainers, and streams; +- deterministic tests and stable public API behavior; +- the current CI workflow rather than the stale `appveyor.yml` WebApi paths. + +Keep review comments concise, actionable, and anchored to a real file and line. +Avoid unrelated refactors and speculative claims. If a source or workflow fact +contradicts the documentation, report the discrepancy and prefer the verified +current behavior until the documentation is corrected. diff --git a/.github/instructions/corpora-usfm-review.instructions.md b/.github/instructions/corpora-usfm-review.instructions.md new file mode 100644 index 000000000..fe4300c7c --- /dev/null +++ b/.github/instructions/corpora-usfm-review.instructions.md @@ -0,0 +1,21 @@ +--- +name: corpora-usfm-review +description: Review corpus and USFM changes for deterministic marker/token behavior, ScriptureRef and versification correctness, Unicode handling, and safe file inputs. +applyTo: "src/SIL.Machine/Corpora/**/*.cs" +--- + +- Trace changed behavior from source text/file or corpus row through tokenization, + parsing, ScriptureRef/`ScrVers` conversion, update handling, and emitted text. +- Use ordinal comparison for marker, token, identifier, and protocol identity unless the + code's contract is explicitly linguistic or user-facing. Do not blanket-replace + culture-aware comparisons; justify the semantic choice. +- For USFM/versification changes, add paired input/output or reference assertions + covering the affected book/chapter/verse mapping, marker nesting, empty/malformed + input, and Unicode case relevant to the change. +- Check that missing, duplicate, or ambiguous references fail or resolve according to + the existing contract. Do not treat a parser snapshot as proof of visual rendering + parity. +- For files, ZIPs, and streams, preserve entry/byte limits, path validation, disposal, + cancellation, and actionable errors. +- Use existing corpus/USFM test helpers and run focused tests plus the normal test/build + checks. Report any fixture, full-suite, or culture/platform evidence not run. diff --git a/.github/instructions/hermitcrab-review.instructions.md b/.github/instructions/hermitcrab-review.instructions.md new file mode 100644 index 000000000..9c17331b7 --- /dev/null +++ b/.github/instructions/hermitcrab-review.instructions.md @@ -0,0 +1,24 @@ +--- +name: hermitcrab-review +description: Review HermitCrab morphology changes for analysis equivalence, memoization-key completeness, retained-memory bounds, parallelism, and hot-path cost. +applyTo: "src/SIL.Machine.Morphology.HermitCrab/**/*.cs" +--- + +- Treat analysis output as the primary contract. A faster parse, more memo hits, or a + successful build does not prove equivalent analyses. +- When changing analysis-side rules or state, re-audit every field the rule reads + against `AnalysisStateKey`. Check freezing, cached hashes, mutable dictionaries, + equality, rule counts, non-head counts, feature structures, and stratum identity. +- Memoized results must represent fully expanded subtrees. Check replay prefixes, + deduplication, empty/nogood entries, in-flight recursion, and the separation between + sequential and parallel scopes. +- Preserve the per-parse scope rule and inspect the + 100,000-entry/1,000,000-retained-word backstops when changing storage or result lists. + Do not weaken a bound without measured evidence and tests. +- Inspect allocations and retained object lifetimes only in changed inner loops. If the + change claims a performance improvement, require a reproducible benchmark or measured + artifact in addition to semantic regression tests. +- Exercise sequential and parallel behavior where the changed path supports both, and + test cancellation/disposal if a boundary is asynchronous. +- Run focused HermitCrab tests and the normal release test/build checks. State any + unavailable benchmark, large-corpus, or platform evidence explicitly. diff --git a/.github/instructions/machine-library-review.instructions.md b/.github/instructions/machine-library-review.instructions.md new file mode 100644 index 000000000..25567e398 --- /dev/null +++ b/.github/instructions/machine-library-review.instructions.md @@ -0,0 +1,26 @@ +--- +name: machine-library-review +description: Review shipped machine library code for compatibility, deterministic language behavior, async contracts, resources, and focused tests. +applyTo: "src/SIL.Machine/**/*.cs,src/SIL.Machine.Translation.Thot/**/*.cs,src/SIL.Machine.Tokenization.SentencePiece/**/*.cs,src/SIL.Machine.Translation.TensorFlow/**/*.cs" +--- + +- Treat code in these projects as shipped library code. Check public/protected API + shape, overloads, optional parameters, return types, XML documentation, target + framework, package references, and assembly/package versioning when touched. +- The current library target is `netstandard2.0`; do not introduce a target or API that + silently removes existing consumers. Do not demand nullable reference types for the + whole library, but review any changed annotations or `#nullable` contract carefully + because test projects enable nullable while shipped libraries do not. +- Existing public asynchronous APIs use `Task` and `CancellationToken`; existing + corpus/tokenizer APIs are synchronous. Verify cancellation propagation, ordering, + disposal, and library-context behavior in changed async code. Treat a new + `IAsyncEnumerable` surface as an explicit compatibility decision. +- Use explicit culture/comparison semantics for token, marker, identifier, serialized, + and protocol logic. Preserve genuinely linguistic culture-aware behavior. Add a + culture regression test when the changed code can vary by culture. +- For file, stream, archive, process, or native-DLL changes, check bounds, paths, + disposal, platform selection, and failure propagation. +- Add or update focused tests for changed decisions and boundary cases. Report commands + and gaps; do not use a coverage percentage as the sole proof. +- Run `dotnet csharpier check .`, the relevant test project, and the release build as + appropriate. Respect `.editorconfig` and CSharpier's 120-column width. diff --git a/.github/instructions/machine-tests-review.instructions.md b/.github/instructions/machine-tests-review.instructions.md new file mode 100644 index 000000000..6a5e8810a --- /dev/null +++ b/.github/instructions/machine-tests-review.instructions.md @@ -0,0 +1,24 @@ +--- +name: machine-tests-review +description: Review machine tests as evidence for changed behavior, edge cases, contracts, and resource/cancellation boundaries. +applyTo: "tests/**/*.cs" +--- + +- A test must exercise the changed behavior, not merely execute the changed method. + Identify branches, guards, ordering, error paths, cancellation, limits, and side + effects in the production diff. +- Prefer focused NUnit tests in the affected test project. Keep fixtures deterministic + and avoid sleeps, ambient machine state, or localized selectors. +- For string/token/marker/USFM behavior, include relevant Unicode, empty, malformed, + nested, and non-English-culture cases. Do not use a culture-sensitive assertion helper + when the contract is ordinal identity. +- For async code, assert cancellation and completion behavior where the change promises + it; do not hide unobserved tasks. +- For HermitCrab changes, compare analysis semantics, not only memo-hit counts or + execution success. Exercise key completeness, replay, resource caps, and + parallel/sequential equivalence when touched. +- For public API changes, include compile/use coverage for the changed signature and + document any consumer or target-framework evidence that was not available. +- Run the focused test command, `dotnet test --verbosity normal`, and coverage + collection when useful. Record filters, skipped tests, failures, and unverified + changed lines. A global coverage percentage is not changed-line evidence. diff --git a/.gitignore b/.gitignore index af6b4a93c..0437b6304 100644 --- a/.gitignore +++ b/.gitignore @@ -55,3 +55,6 @@ tests/SIL.Machine.Tests/Corpora/TestData/usfm/target/* tests/SIL.Machine.Tests/Corpora/TestData/project/* tests/SIL.Machine.Tests/Corpora/TestData/pretranslations.json .idea + +# Local, generated review summaries +.review/ diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..cfd1df53e --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,106 @@ +# machine contributor and agent guide + +Repository-wide operational guidance for contributors and coding agents. Keep it +short and factual. Shared domain vocabulary lives in `CONTEXT.md`; do not +duplicate that glossary here. + +Check the current tree before relying on a rule. If this file disagrees with +`local_check.sh`, `.github/workflows/ci.yml`, a project file, or the code you are +changing, say so and prefer the current executable behavior until this file is +updated. + +## Repository shape + +- `Machine.sln` contains the source and test projects. +- Production code is under `src/`; tests are under `tests/`. +- Native SentencePiece sources are under `src/sentencepiece4c/`. +- Samples and notebooks are under `samples/`; repository scripts under `scripts/`. +- The published libraries target `netstandard2.0`: `SIL.Machine`, + `SIL.Machine.Translation.Thot`, `SIL.Machine.Translation.TensorFlow`, + `SIL.Machine.Morphology.HermitCrab`, and `SIL.Machine.Tokenization.SentencePiece`. +- `SIL.Machine.Tool`, `SIL.Machine.Morphology.HermitCrab.Tool`, + `SIL.Machine.Plugin`, and the test projects target `net10.0`. +- Shared assembly and package metadata comes from `src/AssemblyInfo.props`. + There is no `Directory.Build.props` in this repository. +- A directory under `src/` or `tests/` is not an active project unless a current + project file, solution entry, or CI step references it. + +## Local validation + +`local_check.sh` is the canonical sequence: + +``` +dotnet tool restore +dotnet restore +dotnet csharpier check . +dotnet build --no-restore -c Release +dotnet test --verbosity normal +``` + +Run it from the repository root. Do not skip a failing format, build, or test +step, and do not report success without fresh output. + +CSharpier is required for C#. The tool is pinned in `.config/dotnet-tools.json`, +`.csharpierrc.yaml` sets `printWidth: 120`, and `.editorconfig` sets +`max_line_length = 120` plus the repository's analyzer severities. +`.csharpierignore` excludes config, project, props, targets, and XML files. + +CI also collects coverage (`--collect:"Xplat Code Coverage"`); `local_check.sh` +does not, so do not describe a local run as coverage-equivalent. + +## Native SentencePiece boundary + +CI builds `src/sentencepiece4c` separately and feeds the platform-specific +artifact to the managed build and package job. The verified forms are: + +``` +cmake -S src/sentencepiece4c -B src/sentencepiece4c/build -G Ninja -DCMAKE_BUILD_TYPE=Release +cmake --build src/sentencepiece4c/build --config Release --target sentencepiece4c +``` + +Windows CI configures with `-A x64` instead of the Ninja generator. This is the +only native boundary in the repository; it is not a general native-before-managed +build policy. + +## Tests and changes + +- Add or update focused tests with every behavior change. +- Keep test data deterministic and make platform assumptions explicit. +- When a public API or package boundary changes, consider both `netstandard2.0` + consumer compatibility and `net10.0` tool and test behavior. +- Dispose engines, models, trainers, and streams according to their contracts. +- Preserve public API semantics unless the change intends otherwise and updates + tests and documentation. +- Use string comparisons that match the domain: ordinal for markers, tokens, and + other protocol identity; culture-sensitive only where the operation is genuinely + linguistic. +- Prefer existing abstractions over parallel ones. Keep terminology consistent + with `CONTEXT.md`. + +## CI and legacy CI + +`.github/workflows/ci.yml` is the current reference: Ubuntu and Windows, .NET 10, +native SentencePiece build, CSharpier check, Release build, tests with coverage, +and tag-triggered NuGet publishing. It triggers on `push`, not on `pull_request`, +so a green check on a PR reflects the pushed head rather than a PR event. + +`appveyor.yml` is legacy. It still references `SIL.Machine.WebApi` projects that +are absent from the tree. Do not add projects to satisfy it, and do not treat its +VS2019 assumptions as the current target matrix. + +## Porting to and from machine.py + +`.github/workflows/create-porting-issue.yml` files a porting issue in the sibling +repository when a pull request merges: `machine.py` when running in `machine`, and +the reverse in `machine.py`. It labels the issue `porting` and marks the body +`AUTO-GENERATED-ISSUE`. This is issue-level coordination, not a runtime +dependency. When a change ports work from the sibling repository, link the source +pull request. + +## Working with agent guidance + +- This file is the shared operational source of truth. `CLAUDE.md` imports it. +- Claude-specific workflows live under `.claude/skills/`. +- `.github/copilot-instructions.md` and `.github/instructions/*.instructions.md` + exist for GitHub Copilot compatibility and must not restate rules from here. +- Add a nested `AGENTS.md` only when a subtree genuinely needs different rules. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..a46b39ff7 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,9 @@ +@AGENTS.md + +## Claude Code + +- Keep repository-wide standing guidance in `AGENTS.md` and import it here. +- Put Claude-only workflows and task procedures under `.claude/skills/`. +- Keep `.github/` for GitHub-required compatibility files: workflows, issue and + pull request templates, `copilot-instructions.md`, and + `.github/instructions/*.instructions.md`. diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 000000000..638f3c489 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,207 @@ +# machine shared domain context + +## Scope and non-goals + +This file defines the shared language for machine's corpora, tokenization, +alignment, translation, Scripture references, and morphology code. It is a +terminology and relationship layer, not an architecture manual, an API reference, +or a release checklist. Operational rules belong in `AGENTS.md`. + +Prefer terms that a type under `src/` actually represents. If a term has no +current source or test anchor, label it external, historical, or proposed rather +than presenting it as current implementation. + +## Product scope + +machine is a natural-language-processing library, aimed in part at resource-poor +languages. The repository provides corpus abstractions, tokenizers and +detokenizers, Scripture-aware text corpora, word alignment and translation +abstractions, Thot SMT, TensorFlow SavedModel translation, and HermitCrab +morphology. `README.md` also documents the statistical methods, NuGet packages, +command-line tools, and tutorial notebooks. + +## Corpora and rows + +`ICorpus` is an enumerable corpus of rows where `T` implements `IRow` +(`src/SIL.Machine/Corpora/ICorpus.cs`). `Count` can include or exclude empty rows. + +`IText` is a corpus-backed text with an `Id` and a `SortKey` +(`src/SIL.Machine/Corpora/IText.cs`). `ITextCorpus` adds a collection of texts, an +`IsTokenized` state, Scripture versification, and row access by text id +(`src/SIL.Machine/Corpora/ITextCorpus.cs`). + +`IParallelTextCorpus` pairs a source and target side with separate tokenization +states (`src/SIL.Machine/Corpora/IParallelTextCorpus.cs`). `IAlignmentCorpus` +provides alignment rows (`src/SIL.Machine/Corpora/IAlignmentCorpus.cs`). + +`TextRow` carries a text id, a Scripture reference, a content type, flags, and a +segment held as `IReadOnlyList` (`src/SIL.Machine/Corpora/TextRow.cs`). +Its `Text` property joins the segment tokens with spaces. A row is empty when its +segment has no tokens. + +`ParallelTextRow` holds source and target segments, references, flags, and +aligned word pairs (`src/SIL.Machine/Corpora/ParallelTextRow.cs`). It is empty if +either side is empty; `Invert` swaps the sides and the alignment. +`NParallelTextRow` generalizes this to several parallel segments +(`src/SIL.Machine/Corpora/NParallelTextRow.cs`). `AlignmentRow` stores aligned +word pairs (`src/SIL.Machine/Corpora/AlignmentRow.cs`). + +A corpus is not automatically a list of tokens. Its rows may be tokenized, +untokenized, empty, parallel, alignment-bearing, or Scripture-aware. Always say +which representation a method expects. + +## Tokenization and detokenization + +`ITokenizer` tokenizes whole data or a range +(`src/SIL.Machine/Tokenization/ITokenizer.cs`); `IDetokenizer` rebuilds +data from tokens (`src/SIL.Machine/Tokenization/IDetokenizer.cs`). + +`StringTokenizer` is the base for string tokenizers. `WhitespaceTokenizer` +handles whitespace, zero-width space, and byte-order-mark boundaries. +`LatinWordTokenizer` adds URL, punctuation, inner-punctuation, abbreviation, and +apostrophe rules. `StringDetokenizer` defines no-op and merge-left, merge-right, +and merge-both behaviors with a configurable separator. + +Tokenization and detokenization are not guaranteed inverses for every rule set. +Preserve the tokenizer and detokenizer pair that an engine was configured with. + +USFM is the Scripture text format the corpus code handles. `UsfmToken` has token +types for book, chapter, verse, text, paragraph, character, note, end, milestone, +attribute, and unknown, plus marker, text, data, and source position +(`src/SIL.Machine/Corpora/UsfmToken.cs`). `UsfmTag` carries text type, style, and +property information. `UsfmTokenizer` uses a stylesheet and right-to-left order +and can preserve whitespace. `UsfmFileTextCorpus` reads `.SFM` files and +`UsxFileTextCorpus` reads `.usx` files. + +## Scripture references and versification + +`ScriptureTextCorpus` and `ScriptureText` carry a `ScrVers` versification and +build rows from Scripture references and ranges. + +`ScriptureRef` is a verse reference with a nested path +(`src/SIL.Machine/Corpora/ScriptureRef.cs`). `ScriptureRangeParser` +(`src/SIL.Machine/Scripture/ScriptureRangeParser.cs`) parses +chapters, verses, and ranges, defaulting to `ScrVers.Original` unless another +versification is supplied. + +"Reference" in corpus code means a Scripture location or row location, not object +identity. "Versification" is the numbering system used to interpret references. + +## Translation engines, models, and training + +`ITranslationEngine` translates strings or token lists, synchronously and +asynchronously, supports batches, and may return n-best results +(`src/SIL.Machine/Translation/ITranslationEngine.cs`). A segment here is the unit +submitted to an engine: a string or an ordered token list, depending on the +overload. + +`ITranslationModel` extends the engine abstraction and creates a trainer from an +`IParallelTextCorpus`. `ITrainer` trains, saves, and reports statistics. + +A model is learned or saved state. An engine is the object that performs +translation. A trainer creates or updates model state. A model may expose an +engine-like interface, but model and engine are not synonyms when discussing +lifecycle or persistence. + +`ThotSmtModel` is the Thot-backed statistical model. It owns direct, inverse, and +symmetrized word alignment models and exposes tokenizer and detokenizer +configuration. Thot's documented alignment methods are IBM 1-4, HMM, and +FastAlign. `SavedModelNmtEngine` loads a TensorFlow SavedModel using its +configured signature keys and defaults to whitespace tokenization. + +HuggingFace is not a current in-repo engine or adapter; treat it as an external +or future term only. + +## Word alignment + +`IWordAligner` aligns one token pair or a batch and returns a +`WordAlignmentMatrix`. `IWordAlignmentMethod` supplies the score-selection +policy. `IWordAlignmentModel` exposes vocabularies, training, scores, and best +aligned pairs. + +`ITransductiveWordAlignmentModel` exposes the training alignment count and +retrieval of a training alignment by index. Transductive here means access to the +alignments of the training examples, not a general claim about a learning +technique. + +`WordAlignmentMatrix` is a boolean matrix whose rows are source word positions +and columns are target word positions +(`src/SIL.Machine/Translation/WordAlignmentMatrix.cs`). It supports union, +intersection, priority symmetrization, and conversion to aligned word pairs. An +aligned word pair is a relation between positions, not a dictionary entry. + +## Morphology + +The neutral API is `IMorpheme`, `WordAnalysis`, `IMorphologicalAnalyzer`, and +`IMorphologicalGenerator` under `src/SIL.Machine/Morphology/`. `IMorpheme` +describes a stem or affix. `WordAnalysis` is an ordered morpheme analysis with a +root index and category; it is a public value, not HermitCrab's internal `Word`. + +HermitCrab's integration object is `Language`, which owns strata, feature +systems, lexicon and rule configuration, and analysis and synthesis compilation +(`src/SIL.Machine.Morphology.HermitCrab/Language.cs`). "Grammar" is acceptable as +an umbrella term for the configured morphology system; there is no `Grammar.cs` +type in this tree. + +`Stratum` holds a character definition table, morphological rules, and a lexicon +for one stage of the pipeline. A stratum is a pipeline stage, not a data layer. + +`Allomorph` is a conditioned realization of a morpheme; `RootAllomorph` is the +lexical-root specialization. `Segments` stores a representation through a +`CharacterDefinitionTable` and can expose a frozen `Shape`. Shape here is a +phonological form, not geometry. HermitCrab `Word` is internal morphology state +holding allomorphs, root, shape, rules, features, range, and stratum. +`Morpher.AnalyzeWord` produces `WordAnalysis`; `Morpher.GenerateWords` produces +surface forms. + +Analysis means decomposing a word into morphemes and features. Synthesis means +generating surface forms from morphemes and features. Do not say "analysis" for a +word alignment without naming the domain. + +## Architecture language + +A source project is a `.csproj` under `src/`, not every namespace or directory. A +test project is a current `.csproj` under `tests/` with a solution or CI entry; a +`bin` or `obj` directory is not evidence of a project. + +The `netstandard2.0` libraries are the reusable package boundary. `net10.0` +projects host tools, plugin behavior, and tests. + +SentencePiece4c is a native build and runtime boundary beneath the managed +SentencePiece project. It is the only such boundary; most of the repository is +platform-independent managed code. + +machine.py is a sibling repository coordinated through post-merge porting issues. +Serval is an external consumer. Neither is a verified in-repo dependency. + +## Disambiguation rules + +- **Corpus**: a row-producing collection; not a file, a tokenizer, or a model. +- **Row**: one corpus or alignment record; not a token. +- **Segment**: an ordered token sequence for a row, or the unit submitted to a + translation engine. In morphology, say phonological segment. +- **Token**: tokenizer output or a structured USFM token; not necessarily a + whitespace-delimited word. +- **Word**: qualify as corpus word position, `WordAnalysis`, or HermitCrab + `Word`. These are three different abstractions. +- **Model**: learned or saved translation or alignment state. +- **Engine**: the object that performs translation; name the backend when it + matters. +- **Trainer**: the object that trains and saves model state. +- **Alignment**: a relation between source and target positions; not a + translation. +- **Analysis**: morphological decomposition, unless another domain is named. +- **Grammar**: the HermitCrab configuration as a whole. Prefer `Language`, + `Stratum`, rules, lexicon, and features in code discussion. +- **Shape**: a HermitCrab phonological form. +- **Stratum**: one HermitCrab pipeline stage. +- **Reference**: a `ScriptureRef` or row location in corpus context. +- **Versification**: the Scripture numbering system. +- **SMT**: statistical machine translation, currently `ThotSmtModel`. + +## Maintenance + +When you add a term, anchor it to a current source or test path. When a type or +workflow changes, update the smallest relevant section. Do not copy command lists +from `AGENTS.md` into this file. Record uncertainty rather than promoting an +unverified relationship into architecture. From 412c11bb9470f90721c154ca6b1b4de5a84e723d Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 17 Sep 2026 17:12:10 -0400 Subject: [PATCH 02/10] Add comment-hygiene standard and a diff-scoped checker Port the FieldWorks comment-hygiene gate to this repository as a cross-platform PowerShell 7 script, so an agent has to fix its own comments before they reach review. scripts/CommentHygiene.psm1 holds the rules: the banned-content categories, the .editorconfig display-width limit, and the aggregate 200-character budget for a run of consecutive implementation comments. A /// block and a PowerShell block comment are exempt from that budget but not from the content or width rules. scripts/comment-hygiene.ps1 supplies the scope. It diffs the working tree against the merge base with the pull request base and checks only the lines the branch adds, treating an untracked in-scope file as entirely new. -Full sizes existing debt, -Advisory reports without failing, and -SelfTest exercises the rules over C#, shell, and PowerShell fixtures without needing a test framework. Two FieldWorks rules are deliberately absent. Project-XML comments are out of scope because CSharpier already ignores those files, and there is no complexity-based budget exception because a predictable budget is easier to work with than a heuristic one. The absence-narration pattern is narrower than the original: a bare "used to" or "no longer" reads as purpose or present state in most of this codebase, and a blocking gate needs precision more than recall. local_check.sh gains --agent-strict, which runs the blocking scan before the existing format, build, and test steps. The human default sequence is unchanged. The new pull request workflow runs the same scan in advisory mode on Ubuntu and Windows, so existing debt cannot block a contributor. A full scan reports 73 violations across the 727 files in scope: 22 over-width lines, 49 over-budget blocks, and 2 banned-content comments. That existing debt is why the gate is scoped to added lines. This branch passes its own strict check, and the self-test passes on PowerShell 7.6. Co-Authored-By: Claude Opus 5 --- .claude/skills/code-comments/SKILL.md | 123 +++++++++ .github/workflows/comment-hygiene.yml | 46 ++++ AGENTS.md | 17 ++ local_check.sh | 27 ++ scripts/CommentHygiene.psm1 | 356 ++++++++++++++++++++++++++ scripts/comment-hygiene.ps1 | 340 ++++++++++++++++++++++++ 6 files changed, 909 insertions(+) create mode 100644 .claude/skills/code-comments/SKILL.md create mode 100644 .github/workflows/comment-hygiene.yml create mode 100644 scripts/CommentHygiene.psm1 create mode 100644 scripts/comment-hygiene.ps1 diff --git a/.claude/skills/code-comments/SKILL.md b/.claude/skills/code-comments/SKILL.md new file mode 100644 index 000000000..a51024034 --- /dev/null +++ b/.claude/skills/code-comments/SKILL.md @@ -0,0 +1,123 @@ +--- +name: code-comments +description: MUST use before writing or editing any comment in this repository, in .cs, .cpp/.h, .ps1/.psm1, .sh, or .py alike. Covers the content contract, the banned content categories, XML documentation rules, the 200-character budget for an implementation comment block, and the 120-column width limit that CI reports on. +--- + +# Machine code comments + +Write comments for the next reader. A comment must explain a current contract, +invariant, constraint, compatibility requirement, performance tradeoff, or +non-obvious reason. If the code and its names already make the behaviour +obvious, delete the comment. + +`scripts/comment-hygiene.ps1` enforces the mechanical parts of this standard +over the lines a branch adds. It cannot judge whether a comment is accurate or +worth keeping; that is still the author's and reviewer's job. + +## Scope + +The checker examines whole-line comments in: + +- C# under `src/` and `tests/`; +- the C/C++ wrapper under `src/sentencepiece4c/`; +- PowerShell, shell, and Python under `scripts/`, plus `local_check.sh`. + +It examines `//`, `///`, and whole-line `#` comments. It does not examine +trailing comments, `/* ... */` blocks, Python docstrings, string literals, or a +shebang line. A bare `#` in a C# file is a preprocessor directive, not a +comment. + +## Content rules + +- Be accurate before being brief. Three or four sentences is usually enough. +- Explain WHAT the code guarantees and WHY the choice matters. Do not narrate + HOW the current implementation works; the comment should survive an equivalent + rewrite. +- A member summary describes that member's own contract, not a caller's or a + collaborator's. +- A comment must stand on its own. Delete restatements of the adjacent code. +- Private comments are for non-obvious behaviour, invariants, compatibility, + performance, or a subtle bug fix. +- Keep a compatibility comment about behaviour that must stay true, for example + "Matches the legacy normalization so persisted data stays interoperable". Do + not say that code was ported, changed in a commit, or used to behave + differently. +- Use ASCII punctuation: `--` for an em dash, `->` for an arrow, `...` for an + ellipsis, `-` for a bullet or en dash, `x` for a multiplication sign, and + plain quotes. This is about typography only; comments may contain any script, + IPA, or orthography the language data requires. +- Prefer a clear word to an abbreviation the next reader cannot resolve. + +## Banned comment content + +Do not write: + +- process framing such as `Phase 1`, `later we'll`, or `we'll eventually`; +- pointers to a Markdown document, a numbered section, or a review note; +- historical narration such as `it used to`, `used to be`, `previously + returned`, `was removed`, `renamed from`, `first shipped`, or `no longer + used`; +- provenance claims such as `shared by X and Y`, `the only caller`, or + `extracted from`; +- cross-file pointers such as `see X's note` or `as documented in X`. + +A present-tense statement about current state is fine: "Returns null when the +stratum has no rules" is a contract, not history. A GitHub issue reference that +is part of the current contract may stay. + +## XML documentation + +The shipped library projects set `GenerateDocumentationFile` and suppress +CS1591/CS1573, so the compiler does not require documentation. This standard +supplies the contract the compiler does not enforce. + +- Put one `` directly above the documented member. Give a public + constructor with parameters a summary too. +- Omit `` and `` when they only restate a name or a type. Keep + them when they add units, nullability, ownership, constraints, or real result + semantics. +- Be all-or-nothing for parameters: document every parameter, or fold the + explanation into the summary. +- Use `` for a property contract, `` for errors a caller is + expected to handle, and `` for contract-relevant symbols. +- Do not repeat the same fact in the summary, the parameters, and the returns. +- Do not add decorative file headers or section-divider comments. + +## Length and width + +An implementation-comment block is a consecutive run of whole-line `//` or `#` +comments; a blank line, a line of code, or a doc comment ends it. **The combined +trimmed text of one block must be at most 200 characters.** This is a single +aggregate budget, not 200 characters per line, and neither the indentation nor +the `//` marker counts toward it. + +A `///` block and a PowerShell block comment are exempt from that budget. They +are exempt from how much they may say, not from how they are written: the +content rules and the width limit still apply. + +Every comment line must fit `max_line_length` from `.editorconfig`, currently +120 display columns, counting indentation and the marker. A tab advances to the +next four-column stop. + +If a block needs more than 200 characters, the usual answer is that it belongs +in an XML summary on the member, or that it is explaining something the code +should express directly. + +## Tests + +A test comment should explain a non-obvious fixture, fake, or setup constraint. +It should not restate the test name or say that the test exists for coverage. + +## Running the check + +``` +pwsh ./scripts/comment-hygiene.ps1 # lines this branch adds; fails on a violation +pwsh ./scripts/comment-hygiene.ps1 -Advisory # same scan, always exits 0 +pwsh ./scripts/comment-hygiene.ps1 -Full -Advisory # size the existing debt +pwsh ./scripts/comment-hygiene.ps1 -SelfTest # verify the rules themselves +``` + +Agents must run `./local_check.sh --agent-strict`, which runs the blocking scan +before the format, build, and test steps. Do not drop the flag to get a run +through; fix the comments instead. The pull request workflow is advisory, so a +clean CI check is not evidence that the strict check passed. diff --git a/.github/workflows/comment-hygiene.yml b/.github/workflows/comment-hygiene.yml new file mode 100644 index 000000000..91da9d628 --- /dev/null +++ b/.github/workflows/comment-hygiene.yml @@ -0,0 +1,46 @@ +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 arrives in about a minute instead of + # waiting on the native build, the Release build, and the test matrix. + comment_hygiene: + name: Report comment hygiene (${{ matrix.os }}) + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + os: [ubuntu-22.04, windows-latest] + + steps: + - name: Check out full history + uses: actions/checkout@v6 + with: + # The scan diffs against the merge base; a shallow checkout leaves + # that commit unreachable. + fetch-depth: 0 + + - name: Verify the hygiene rules + shell: pwsh + run: ./scripts/comment-hygiene.ps1 -SelfTest + + # 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 + shell: pwsh + run: > + ./scripts/comment-hygiene.ps1 + -Advisory + -BaseRef '${{ github.event.pull_request.base.sha }}' diff --git a/AGENTS.md b/AGENTS.md index cfd1df53e..f95a60f2b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,6 +48,23 @@ CSharpier is required for C#. The tool is pinned in `.config/dotnet-tools.json`, CI also collects coverage (`--collect:"Xplat Code Coverage"`); `local_check.sh` does not, so do not describe a local run as coverage-equivalent. +## Comment hygiene + +Agents must run `./local_check.sh --agent-strict`. It adds +`scripts/comment-hygiene.ps1` to the sequence above and fails the run on any +comment violation in the lines the branch adds, so you fix your own comments +before they reach review. This flag is required of agents and optional for +humans. Do not drop it to get a run through. + +The standard is `.claude/skills/code-comments/SKILL.md`: a 200-character +aggregate budget for a block of implementation comments, the 120-column +`.editorconfig` width for every comment line, ASCII punctuation, and no process +framing, document pointers, historical narration, or provenance claims. + +The `Comment hygiene` pull request workflow runs the same scan in advisory mode. +It annotates and never fails, so a green check there is not evidence that the +strict check passed. + ## Native SentencePiece boundary CI builds `src/sentencepiece4c` separately and feeds the platform-specific diff --git a/local_check.sh b/local_check.sh index 44d78c087..2b9717dad 100755 --- a/local_check.sh +++ b/local_check.sh @@ -1,4 +1,31 @@ #!/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 + +if [ "$agent_strict" = true ]; then + if ! command -v pwsh > /dev/null 2>&1; then + echo "--agent-strict needs PowerShell 7 (pwsh) on PATH." >&2 + exit 1 + fi + pwsh -NoProfile -File ./scripts/comment-hygiene.ps1 + if [ $? -ne 0 ]; then + exit 1 + fi +fi + dotnet tool restore dotnet restore dotnet csharpier check . diff --git a/scripts/CommentHygiene.psm1 b/scripts/CommentHygiene.psm1 new file mode 100644 index 000000000..6229042a4 --- /dev/null +++ b/scripts/CommentHygiene.psm1 @@ -0,0 +1,356 @@ +<# +.SYNOPSIS + Comment-hygiene rules for the machine repository. + +.DESCRIPTION + Classifies whole-line comments and applies the banned-content categories, + the .editorconfig display-width limit, and the aggregate + implementation-comment budget. The rules stand apart from any file list or + diff scope, so they can be exercised on their own. +#> + +Set-StrictMode -Version Latest + +# Declared at module scope: under Set-StrictMode a script-scoped variable read +# before its first assignment throws rather than returning $null. +$script:editorConfigCache = @{} + +function Get-NonAsciiPunctuationCharacters { + <# + .SYNOPSIS + Returns the set of typographic characters the banned-punctuation rule reports. + + .DESCRIPTION + Built from [char] code points rather than escape sequences so the set is + identical under every PowerShell edition that can run this module. Only + these characters are reported; comments may otherwise contain any script, + IPA, or Unicode content the language data requires. + #> + return @( + [char]0x2014, [char]0x2013, [char]0x2192, [char]0x2190, [char]0x2194, + [char]0x2026, [char]0x2022, [char]0x00d7, [char]0x2018, [char]0x2019, + [char]0x201c, [char]0x201d, [char]0x00a7 + ) +} + +function Get-NonAsciiPunctuationPattern { + <# + .SYNOPSIS + Returns a regex matching any single character in Get-NonAsciiPunctuationCharacters. + #> + $escaped = Get-NonAsciiPunctuationCharacters | ForEach-Object { [regex]::Escape([string]$_) } + return '(?:' + ($escaped -join '|') + ')' +} + +function Get-CommentHygieneCategories { + <# + .SYNOPSIS + Returns the ordered category-name to regex-pattern map. + + .DESCRIPTION + Each pattern targets a specific low-signal comment shape. "Stage N" and + "this commit" are not banned: HermitCrab strata and Commit are domain + terms here. + #> + $absenceNarration = '(?i)\b(?:it|this|that|these|those|we|they|which)\s+used to\b' ` + + '|\bused to be\b|\bpreviously (?:read|worked|did|returned|used|called)\b' ` + + '|\b(?:was|were) removed\b|\b(?:was|were) stale\b|\brenamed from\b|\bfirst shipped\b' ` + + '|\bno longer (?:used|needed|exists|exist|supported|present|applies|apply|valid)\b' + $provenance = '(?i)\bshared by \w+ and \w+\b|\bthe only caller\b|\bthe sole caller\b|\bextracted from\b' + + return [ordered]@{ + 'process-framing' = '(?i)\bPhase[\s-]?\d+\b|\blater we\x27ll\b|\bwe\x27ll (?:later|eventually)\b' + 'doc-pointer' = '\b[\w./-]+\.md\b|(?i)\bsection\s+\d+[a-z]?\b' + # A bare "used to" or "no longer" usually reads as purpose or present + # state here, so both require an explicitly historical phrasing. + 'absence-narration' = $absenceNarration + 'cross-file-pointer' = "(?i)\bsee [A-Za-z]+\x27s note\b|\bas documented (?:on|in) [A-Za-z]+\b" + 'provenance' = $provenance + 'non-ascii-punctuation' = Get-NonAsciiPunctuationPattern + } +} + +function Get-CommentHygieneLanguage { + <# + .SYNOPSIS + Classifies a path into a comment-syntax family by extension. + + .OUTPUTS + 'CLike' for C# and the SentencePiece C/C++ wrapper, 'Script' for + PowerShell, shell, and Python, or $null for anything else. + #> + param([Parameter(Mandatory)][string] $Path) + + if ($Path -match '\.(ps1|psm1|sh|py)$') { return 'Script' } + if ($Path -match '\.(cs|cpp|cxx|cc|c|h|hpp)$') { return 'CLike' } + return $null +} + +function Get-CommentHygieneEditorConfig { + <# + .SYNOPSIS + Reads max_line_length and tab_width from the repository's .editorconfig [*] section. + + .DESCRIPTION + The comment width limit is whatever .editorconfig already declares for + every file, so the two cannot drift apart. Only the [*] section is read, + because that is where this repository declares the value. A language + section that later sets its own max_line_length would need section + matching here first. + + .OUTPUTS + A hashtable with MaxLineLength and TabWidth, defaulting to 120 and 4. + #> + param([Parameter(Mandatory)][string] $RepoRoot) + + if ($script:editorConfigCache.ContainsKey($RepoRoot)) { return $script:editorConfigCache[$RepoRoot] } + + $settings = @{ MaxLineLength = 120; TabWidth = 4 } + $path = Join-Path $RepoRoot '.editorconfig' + if (Test-Path -LiteralPath $path) { + $inStarSection = $false + foreach ($raw in [System.IO.File]::ReadAllLines($path, [System.Text.Encoding]::UTF8)) { + $line = $raw.Trim() + if ($line.StartsWith('#') -or $line.Length -eq 0) { continue } + if ($line.StartsWith('[')) { $inStarSection = ($line -eq '[*]'); continue } + if (-not $inStarSection) { continue } + if ($line -match '^max_line_length\s*=\s*(\d+)$') { $settings.MaxLineLength = [int]$Matches[1] } + if ($line -match '^tab_width\s*=\s*(\d+)$') { $settings.TabWidth = [int]$Matches[1] } + } + } + + $script:editorConfigCache[$RepoRoot] = $settings + return $settings +} + +function Get-CommentDisplayWidth { + <# + .SYNOPSIS + Returns a line's width in display columns, expanding tabs to the next tab stop. + + .DESCRIPTION + String length counts a tab as one character while max_line_length counts + display columns. The two disagree by enough to decide a violation either + way in tab-indented files. + #> + param( + [Parameter(Mandatory)][AllowEmptyString()][string] $Line, + [Parameter(Mandatory)][int] $TabWidth + ) + + $width = 0 + foreach ($char in $Line.ToCharArray()) { + if ($char -eq "`t") { $width += $TabWidth - ($width % $TabWidth) } + else { $width++ } + } + return $width +} + +function Get-CommentLineClassification { + <# + .SYNOPSIS + Classifies every line as an implementation comment, an exempt doc comment, or neither. + + .PARAMETER Lines + The file's lines. + + .PARAMETER Language + 'CLike' for the // and /// forms, or 'Script' for the number-sign form + and the PowerShell block-comment form. + + .OUTPUTS + A hashtable with parallel arrays Kinds ('impl', 'exempt', or $null per + line) and Bodies (comment text per line, or $null). An exempt line is + outside the aggregate budget but still subject to width and content + rules. A shebang is not a comment. A bare # is never a comment in a + CLike file, so a preprocessor directive is never misread as one. + #> + param( + [Parameter(Mandatory)][AllowEmptyCollection()][AllowEmptyString()][string[]] $Lines, + [Parameter(Mandatory)][ValidateSet('CLike', 'Script')][string] $Language + ) + + $kinds = New-Object 'object[]' $Lines.Count + $bodies = New-Object 'object[]' $Lines.Count + $inHelpBlock = $false + + for ($i = 0; $i -lt $Lines.Count; $i++) { + $trimmed = $Lines[$i].Trim() + + if ($Language -eq 'Script') { + if ($inHelpBlock) { + $kinds[$i] = 'exempt' + $bodies[$i] = $trimmed + if ($trimmed.EndsWith('#>')) { $inHelpBlock = $false } + continue + } + if ($trimmed.StartsWith('<#')) { + $kinds[$i] = 'exempt' + $bodies[$i] = $trimmed.Substring(2).TrimEnd('#', '>', ' ') + if (-not $trimmed.EndsWith('#>')) { $inHelpBlock = $true } + continue + } + if ($i -eq 0 -and $trimmed.StartsWith('#!')) { + $kinds[$i] = $null + $bodies[$i] = $null + continue + } + if ($trimmed.StartsWith('#')) { + $kinds[$i] = 'impl' + $bodies[$i] = $trimmed.Substring(1) + continue + } + } + else { + if ($trimmed.StartsWith('///')) { + $kinds[$i] = 'exempt' + $bodies[$i] = $trimmed.Substring(3) + continue + } + if ($trimmed.StartsWith('//')) { + $kinds[$i] = 'impl' + $bodies[$i] = $trimmed.Substring(2) + continue + } + } + + $kinds[$i] = $null + $bodies[$i] = $null + } + + return @{ Kinds = $kinds; Bodies = $bodies } +} + +function Get-CommentHygieneViolations { + <# + .SYNOPSIS + Scans the given files for mechanical comment-hygiene violations. + + .PARAMETER Files + Paths to scan. A file whose extension Get-CommentHygieneLanguage does not + recognize is skipped. + + .PARAMETER LineFilter + Optional map from a path to the set of 1-based line numbers to check. + Omit it to scan every line of every file. + + .PARAMETER RepoRoot + The root whose .editorconfig supplies the width limit. + + .OUTPUTS + One object per violation with File, Line, Category, and Text. The + 'comment-too-long' category covers a run of consecutive implementation + comment lines whose combined trimmed bodies exceed the budget; a block is + reported at its first line, and a block whose untouched lines alone + already exceed the budget is left to a separate cleanup. + #> + param( + [Parameter(Mandatory)][AllowEmptyCollection()][string[]] $Files, + [hashtable] $LineFilter, + [Parameter(Mandatory)][string] $RepoRoot + ) + + $editorConfig = Get-CommentHygieneEditorConfig -RepoRoot $RepoRoot + $categories = Get-CommentHygieneCategories + $violations = New-Object System.Collections.ArrayList + $maxImplCommentChars = 200 + + foreach ($file in $Files) { + if (-not (Test-Path -LiteralPath $file)) { continue } + + $language = Get-CommentHygieneLanguage -Path $file + if ($null -eq $language) { continue } + + $allowedLines = $null + if ($LineFilter -and $LineFilter.ContainsKey($file)) { $allowedLines = $LineFilter[$file] } + + # ReadAllLines rather than Get-Content: this scan runs on every pull + # request over every changed file, and StreamReader still honours a BOM. + $lines = [System.IO.File]::ReadAllLines($file, [System.Text.Encoding]::UTF8) + $classification = Get-CommentLineClassification -Lines $lines -Language $language + $kinds = $classification.Kinds + $bodies = $classification.Bodies + + for ($i = 0; $i -lt $lines.Count; $i++) { + if ($null -eq $kinds[$i]) { continue } + $lineNumber = $i + 1 + if ($allowedLines -and -not $allowedLines.Contains($lineNumber)) { continue } + + foreach ($category in $categories.Keys) { + if ($bodies[$i] -match $categories[$category]) { + [void]$violations.Add([PSCustomObject]@{ + File = $file + Line = $lineNumber + Category = $category + Text = $bodies[$i].Trim() + }) + } + } + + # Width comes from .editorconfig and applies to every comment line + # including doc comments: those are exempt from what they may say, + # not from how wide they may run. + $displayWidth = Get-CommentDisplayWidth -Line $lines[$i] -TabWidth $editorConfig.TabWidth + if ($displayWidth -gt $editorConfig.MaxLineLength) { + [void]$violations.Add([PSCustomObject]@{ + File = $file + Line = $lineNumber + Category = 'comment-line-too-long' + Text = ("{0} columns (max {1}): {2}" -f + $displayWidth, $editorConfig.MaxLineLength, $bodies[$i].Trim()) + }) + } + } + + $blockStart = -1 + $blockLength = 0 + for ($i = 0; $i -le $lines.Count; $i++) { + $isImplLine = ($i -lt $lines.Count) -and ($kinds[$i] -eq 'impl') + if ($isImplLine) { + if ($blockStart -lt 0) { $blockStart = $i } + $blockLength++ + continue + } + + if ($blockLength -gt 0) { + $blockIndexes = $blockStart..($blockStart + $blockLength - 1) + $totalChars = [int]( + $blockIndexes | ForEach-Object { $bodies[$_].Trim().Length } | Measure-Object -Sum + ).Sum + + if ($totalChars -gt $maxImplCommentChars) { + $touchesBlock = $true + $untouchedChars = 0 + if ($allowedLines) { + $touchesBlock = [bool]($blockIndexes | Where-Object { $allowedLines.Contains($_ + 1) }) + $untouchedIndexes = $blockIndexes | Where-Object { -not $allowedLines.Contains($_ + 1) } + if ($untouchedIndexes) { + $untouchedChars = [int]( + $untouchedIndexes | ForEach-Object { $bodies[$_].Trim().Length } | Measure-Object -Sum + ).Sum + } + } + if ($touchesBlock -and $untouchedChars -le $maxImplCommentChars) { + [void]$violations.Add([PSCustomObject]@{ + File = $file + Line = $blockStart + 1 + Category = 'comment-too-long' + Text = ("{0} chars (budget {1}): {2}" -f + $totalChars, $maxImplCommentChars, $bodies[$blockStart].Trim()) + }) + } + } + } + $blockStart = -1 + $blockLength = 0 + } + } + + # The unary comma stops the pipeline unrolling an empty or single-element + # array into $null or a bare scalar. + return , $violations.ToArray() +} + +Export-ModuleMember -Function @( + 'Get-CommentHygieneViolations' +) diff --git a/scripts/comment-hygiene.ps1 b/scripts/comment-hygiene.ps1 new file mode 100644 index 000000000..c33e6adb0 --- /dev/null +++ b/scripts/comment-hygiene.ps1 @@ -0,0 +1,340 @@ +#!/usr/bin/env pwsh +<# +.SYNOPSIS + Checks the comment hygiene of the lines this branch adds. + +.DESCRIPTION + Diffs the working tree against the merge base with the base ref, collects the + added lines in C#, C/C++, PowerShell, shell, and Python files, and applies the + rules in CommentHygiene.psm1. Untracked in-scope files are treated as entirely + new. + + 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. + +.PARAMETER BaseRef + The ref to diff against. Defaults to the pull request base in CI, then + origin/HEAD, then origin/master. + +.PARAMETER Full + Scans every tracked in-scope file instead of the added lines. Use it to size + existing debt; it is not the pull request gate. + +.PARAMETER Advisory + Reports violations and still exits 0. CI uses this so existing debt and any + false positive cannot block a contributor. + +.PARAMETER ReportPath + Optional path for a JSON report. + +.PARAMETER SelfTest + Runs the built-in rule cases and exits. It needs no test framework, so the + rules stay verifiable on any machine that can run this script. + +.EXAMPLE + ./scripts/comment-hygiene.ps1 + Checks the lines this branch adds and fails on a violation. + +.EXAMPLE + ./scripts/comment-hygiene.ps1 -Full -Advisory + Reports all existing comment debt without failing. +#> +[CmdletBinding()] +param( + [string] $BaseRef, + [switch] $Full, + [switch] $Advisory, + [string] $ReportPath, + [switch] $SelfTest +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +Import-Module (Join-Path $PSScriptRoot 'CommentHygiene.psm1') -Force + +$scopedPathSpecs = @( + 'src/*.cs', 'tests/*.cs', + 'src/sentencepiece4c/*.cpp', 'src/sentencepiece4c/*.cxx', 'src/sentencepiece4c/*.cc', + 'src/sentencepiece4c/*.c', 'src/sentencepiece4c/*.h', 'src/sentencepiece4c/*.hpp', + 'scripts/*.ps1', 'scripts/*.psm1', 'scripts/*.sh', 'scripts/*.py', + 'local_check.sh' +) + +function Get-RepoRoot { + <# + .SYNOPSIS + Returns the top level of the working tree this script lives in. + #> + $root = & git rev-parse --show-toplevel 2>$null + if ($LASTEXITCODE -ne 0 -or [string]::IsNullOrWhiteSpace($root)) { + throw 'comment-hygiene must run inside a git working tree.' + } + return $root.Trim() +} + +function Resolve-BaseRef { + <# + .SYNOPSIS + Resolves the ref to diff against, preferring an explicit value. + + .DESCRIPTION + The candidates are tried in order and the first one git can resolve wins. + git remote show is deliberately not used: resolving a base must not need + network access on a developer machine. + #> + param([string] $Explicit) + + $candidates = @() + if (-not [string]::IsNullOrWhiteSpace($Explicit)) { $candidates += $Explicit } + if (-not [string]::IsNullOrWhiteSpace($env:GITHUB_BASE_REF)) { $candidates += "origin/$($env:GITHUB_BASE_REF)" } + $candidates += 'origin/HEAD' + $candidates += 'origin/master' + + foreach ($candidate in $candidates) { + & git rev-parse --verify --quiet "$candidate^{commit}" > $null 2>&1 + if ($LASTEXITCODE -eq 0) { return $candidate } + } + throw "No usable base ref. Tried: $($candidates -join ', '). Fetch the base branch, or pass -BaseRef." +} + +function Get-AddedLineFilter { + <# + .SYNOPSIS + Returns a map from an in-scope path to the set of line numbers this branch adds. + + .DESCRIPTION + Diffs the working tree against the merge base so local edits are checked + before they are committed. A deleted line is never scanned, and a tracked + line that still matches the merge base is not new. An untracked in-scope + file counts entirely as added. + #> + param([Parameter(Mandatory)][string] $Base, [Parameter(Mandatory)][string] $RepoRoot) + + $mergeBase = & git merge-base $Base HEAD 2>$null + if ($LASTEXITCODE -ne 0 -or [string]::IsNullOrWhiteSpace($mergeBase)) { + throw "No merge base between $Base and HEAD. Fetch enough history (fetch-depth: 0 in CI)." + } + $mergeBase = $mergeBase.Trim() + + $filter = @{} + $currentFile = $null + $hunkStart = 0 + $hunkOffset = 0 + $diff = & git diff --unified=0 $mergeBase -- @scopedPathSpecs + foreach ($line in $diff) { + if ($line.StartsWith('+++ ')) { + $path = $line.Substring(4).Trim() + if ($path -eq '/dev/null') { $currentFile = $null; continue } + if ($path.StartsWith('b/')) { $path = $path.Substring(2) } + $currentFile = (Join-Path $RepoRoot $path) + if (-not $filter.ContainsKey($currentFile)) { + $filter[$currentFile] = New-Object 'System.Collections.Generic.HashSet[int]' + } + continue + } + if ($line -match '^@@ -\S+ \+(\d+)(?:,(\d+))? @@') { + $hunkStart = [int]$Matches[1] + $hunkOffset = 0 + continue + } + if ($null -ne $currentFile -and $line.StartsWith('+')) { + [void]$filter[$currentFile].Add($hunkStart + $hunkOffset) + $hunkOffset++ + } + } + + $untracked = & git ls-files --others --exclude-standard -- @scopedPathSpecs + foreach ($path in $untracked) { + if ([string]::IsNullOrWhiteSpace($path)) { continue } + $full = Join-Path $RepoRoot $path.Trim() + if (-not (Test-Path -LiteralPath $full)) { continue } + $set = New-Object 'System.Collections.Generic.HashSet[int]' + $count = [System.IO.File]::ReadAllLines($full, [System.Text.Encoding]::UTF8).Count + for ($i = 1; $i -le $count; $i++) { [void]$set.Add($i) } + $filter[$full] = $set + } + + return @{ MergeBase = $mergeBase; Filter = $filter } +} + +function Write-Violation { + <# + .SYNOPSIS + Prints one violation, adding a GitHub annotation when running advisory in Actions. + #> + param([Parameter(Mandatory)][psobject] $Violation, [Parameter(Mandatory)][string] $RepoRoot) + + # Join-Path normalises to the platform separator while git reports forward + # slashes, so both sides are normalised before the prefix is removed. + $relative = $Violation.File -replace '\\', '/' + $root = $RepoRoot -replace '\\', '/' + if ($relative.StartsWith($root, [System.StringComparison]::OrdinalIgnoreCase)) { + $relative = $relative.Substring($root.Length).TrimStart('/') + } + Write-Host ("{0}:{1}: {2}: {3}" -f $relative, $Violation.Line, $Violation.Category, $Violation.Text) + if ($Advisory -and $env:GITHUB_ACTIONS -eq 'true') { + Write-Host ("::warning file={0},line={1}::{2}: {3}" -f + $relative, $Violation.Line, $Violation.Category, $Violation.Text) + } +} + +function Invoke-SelfTest { + <# + .SYNOPSIS + Exercises the rules against fixtures in a temporary directory. + + .OUTPUTS + 0 when every case holds, 1 otherwise. + #> + $root = Join-Path ([System.IO.Path]::GetTempPath()) ("comment-hygiene-selftest-" + [guid]::NewGuid()) + New-Item -ItemType Directory -Path $root | Out-Null + $editorConfigContent = "[*]`nmax_line_length = 120`ntab_width = 4`n" + Set-Content -LiteralPath (Join-Path $root '.editorconfig') -Value $editorConfigContent -NoNewline + $failures = New-Object System.Collections.ArrayList + + function Test-Case { + param([string] $Name, [string] $FileName, [string[]] $ContentLines, [string[]] $ExpectedCategories) + + $content = ($ContentLines -join "`n") + "`n" + $path = Join-Path $root $FileName + Set-Content -LiteralPath $path -Value $content -NoNewline + $result = Get-CommentHygieneViolations -Files @($path) -RepoRoot $root + $found = @($result | ForEach-Object { $_.Category } | Sort-Object) + $expected = @($ExpectedCategories | Sort-Object) + if (($found -join ',') -ne ($expected -join ',')) { + [void]$failures.Add( + ("{0}: expected [{1}] but found [{2}]" -f $Name, ($expected -join ','), ($found -join ',')) + ) + } + } + + $long = 'x' * 130 + $sixty = 'y' * 60 + + Test-Case 'clean C# is clean' 'clean.cs' @('// Guards against a null stratum.', 'int a = 1;') @() + Test-Case 'doc comment is budget exempt' 'doc.cs' @( + "/// $sixty", + "/// $sixty", + "/// $sixty", + "/// $sixty", + 'int a = 1;' + ) @() + Test-Case 'block budget aggregates lines' 'block.cs' @( + "// $sixty", "// $sixty", "// $sixty", "// $sixty", 'int a = 1;' + ) @('comment-too-long') + $fifty = 'z' * 50 + Test-Case 'exactly at budget is clean' 'exact.cs' @( + "// $fifty", "// $fifty", "// $fifty", "// $fifty", 'int a = 1;' + ) @() + Test-Case 'one over budget is reported' 'over.cs' @( + "// $fifty", "// $fifty", "// $fifty", "// ${fifty}a", 'int a = 1;' + ) @('comment-too-long') + Test-Case 'blank line ends a block' 'split.cs' @( + "// $sixty", "// $sixty", '', "// $sixty", "// $sixty", 'int a = 1;' + ) @() + Test-Case 'width counts the marker and indent' 'wide.cs' @("`t`t// $long", 'int a = 1;') @('comment-line-too-long') + Test-Case 'em dash is reported' 'dash.cs' @( + ("// Uses a dash " + [char]0x2014 + " here."), 'int a = 1;' + ) @('non-ascii-punctuation') + Test-Case 'absence narration is reported' 'absence.cs' @( + '// This field is no longer used by the loader.', 'int a = 1;' + ) @('absence-narration') + Test-Case 'markdown pointer is reported' 'pointer.cs' @( + '// See design-notes.md for the rationale.', 'int a = 1;' + ) @('doc-pointer') + Test-Case 'provenance is reported' 'prov.cs' @( + '// Extracted from the old tokenizer.', 'int a = 1;' + ) @('provenance') + Test-Case 'preprocessor directive is not a comment' 'pre.cs' @( + '#nullable enable', '#region Parsing', 'int a = 1;' + ) @() + Test-Case 'shell shebang is exempt' 'run.sh' @('#!/bin/bash', 'echo hi') @() + Test-Case 'powershell help block is budget exempt' 'help.ps1' @( + '<#', '.SYNOPSIS', " $sixty", " $sixty", " $sixty", " $sixty", '#>', '$a = 1' + ) @() + Test-Case 'content rules still apply inside a help block' 'helpbad.ps1' @( + '<#', '.DESCRIPTION', " Extracted from the old tokenizer.", '#>', '$a = 1' + ) @('provenance') + Test-Case 'powershell comment uses the same budget' 'budget.ps1' @( + "# $sixty", "# $sixty", "# $sixty", "# $sixty", '$a = 1' + ) @('comment-too-long') + Test-Case 'a one-line help block ends a block' 'split.ps1' @( + "# $sixty", "# $sixty", '<# Reads one line. #>', "# $sixty", "# $sixty", '$a = 1' + ) @() + Test-Case 'a module file is scanned like a script' 'rules.psm1' @( + '# This helper is no longer used by the loader.', '$a = 1' + ) @('absence-narration') + Test-Case 'shell comment uses the same budget' 'budget.sh' @( + "# $sixty", "# $sixty", "# $sixty", "# $sixty", 'echo hi' + ) @('comment-too-long') + Test-Case 'unscoped extension is skipped' 'notes.md' @('// no longer relevant') @() + + Remove-Item -LiteralPath $root -Recurse -Force + if ($failures.Count -gt 0) { + Write-Host "comment-hygiene self-test FAILED" + $failures | ForEach-Object { Write-Host " $_" } + return 1 + } + Write-Host "comment-hygiene self-test passed" + return 0 +} + +if ($SelfTest) { exit (Invoke-SelfTest) } + +$repoRoot = Get-RepoRoot +$lineFilter = $null +$scanFiles = @() +$scopeDescription = '' + +if ($Full) { + $tracked = & git ls-files -- @scopedPathSpecs + $scanFiles = @($tracked | Where-Object { $_ } | ForEach-Object { Join-Path $repoRoot $_.Trim() }) + $scopeDescription = "all $($scanFiles.Count) tracked in-scope files" +} +else { + $base = Resolve-BaseRef -Explicit $BaseRef + $added = Get-AddedLineFilter -Base $base -RepoRoot $repoRoot + $lineFilter = $added.Filter + $scanFiles = @($lineFilter.Keys) + $scopeDescription = "lines added since $base (merge base $($added.MergeBase.Substring(0, 8)))" +} + +if ($scanFiles.Count -eq 0) { + Write-Host "comment-hygiene: no in-scope files to check ($scopeDescription)." + exit 0 +} + +$violations = Get-CommentHygieneViolations -Files $scanFiles -LineFilter $lineFilter -RepoRoot $repoRoot + +if (-not [string]::IsNullOrWhiteSpace($ReportPath)) { + $reportDir = Split-Path -Parent $ReportPath + if ($reportDir -and -not (Test-Path -LiteralPath $reportDir)) { + New-Item -ItemType Directory -Path $reportDir -Force | Out-Null + } + $payload = [PSCustomObject]@{ + scope = $scopeDescription + advisory = [bool]$Advisory + violationCount = $violations.Count + violations = $violations + } + $payload | ConvertTo-Json -Depth 5 | Set-Content -LiteralPath $ReportPath -Encoding utf8 +} + +if ($violations.Count -eq 0) { + Write-Host "comment-hygiene: clean ($scopeDescription)." + exit 0 +} + +Write-Host "comment-hygiene: $($violations.Count) violation(s) in $scopeDescription." +Write-Host '' +foreach ($violation in $violations) { Write-Violation -Violation $violation -RepoRoot $repoRoot } +Write-Host '' +Write-Host 'The standard is .claude/skills/code-comments/SKILL.md.' +Write-Host 'Reproduce locally with: pwsh ./scripts/comment-hygiene.ps1' + +if ($Advisory) { + Write-Host 'Advisory only; not failing the run.' + exit 0 +} +exit 1 From 5914ddb8c19c3a917a062f8fefb7b42472733dd8 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 17 Sep 2026 20:10:57 -0400 Subject: [PATCH 03/10] Move path-scoped review rules to docs/review This repository uses Codex and Claude Code rather than GitHub Copilot, so the Copilot-specific guidance files no longer have a reader. The review rules they carried are tool-neutral and stay. The four path-scoped rule sets and the adversarial review role move to docs/review/ as plain Markdown. Copilot front matter becomes a title, a one-line summary, and a line naming the paths the rules govern; the rules themselves are unchanged. AGENTS.md gains a table mapping path glob to rules file, so an agent that reads only AGENTS.md can still find the rules for a changed path. The pr-review skill points at the same files, and CLAUDE.md now describes .github/ as workflows and templates only. .github/copilot-instructions.md is deleted. Everything in it was already covered by AGENTS.md or the pr-review skill except one rule about search evidence, which moves into the pr-review skill's evidence section. The pr-authoring skill no longer names Copilot as the source of a review comment, and both authoring and review skills now carry the ./local_check.sh --agent-strict requirement that AGENTS.md states, so an agent following either skill alone still runs the comment scan. Co-Authored-By: Claude Opus 5 --- .claude/skills/pr-authoring/SKILL.md | 8 +++++- .claude/skills/pr-review/SKILL.md | 19 +++++++++++-- .github/agents/devils-advocate.agent.md | 21 -------------- .github/copilot-instructions.md | 28 ------------------- AGENTS.md | 17 +++++++++-- CLAUDE.md | 5 ++-- .../review/corpora-usfm.md | 11 ++++---- docs/review/devils-advocate.md | 26 +++++++++++++++++ .../review/hermitcrab.md | 11 ++++---- .../review/machine-library.md | 13 +++++---- .../review/machine-tests.md | 11 ++++---- 11 files changed, 92 insertions(+), 78 deletions(-) delete mode 100644 .github/agents/devils-advocate.agent.md delete mode 100644 .github/copilot-instructions.md rename .github/instructions/corpora-usfm-review.instructions.md => docs/review/corpora-usfm.md (81%) create mode 100644 docs/review/devils-advocate.md rename .github/instructions/hermitcrab-review.instructions.md => docs/review/hermitcrab.md (84%) rename .github/instructions/machine-library-review.instructions.md => docs/review/machine-library.md (81%) rename .github/instructions/machine-tests-review.instructions.md => docs/review/machine-tests.md (87%) diff --git a/.claude/skills/pr-authoring/SKILL.md b/.claude/skills/pr-authoring/SKILL.md index c6f21b3bb..739e072c8 100644 --- a/.claude/skills/pr-authoring/SKILL.md +++ b/.claude/skills/pr-authoring/SKILL.md @@ -114,6 +114,12 @@ or Windows CMake commands exactly as shown in ci.yml. For packaging changes, run or record the affected dotnet pack ... -c Release -o artifacts command and distinguish local results from the CI package job. +An agent must also run `./local_check.sh --agent-strict`, which adds the +comment-hygiene scan and fails on any violation in the lines the branch adds. +It is required of agents and optional for humans; do not drop it to get a run +through. The advisory `Comment hygiene` pull request check going green is not +evidence that the strict scan passed. + For documentation/skill/template-only changes, run dotnet csharpier check . only if C# files are touched; otherwise use git diff --check and available YAML/Markdown diagnostics. Always state skipped checks and why. Never claim @@ -174,7 +180,7 @@ workflow has already created one. ## 8. Review comments -For each Copilot or human comment, classify it as: +For each review comment, human or automated, classify it as: * **Fix** -- technically sound and unambiguous; make the minimum change. * **Clarify** -- missing context or a design decision; ask the specific question. diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index d0c5c2553..c16766fbe 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -34,7 +34,14 @@ explicitly authorizes that action. files, and any target-local instructions. 3. Classify changed files: shipped API/library, tests, HermitCrab, corpus/USFM, SentencePiece/native, tool, workflow/package, or documentation. -4. Apply only the review axes and path-scoped instructions relevant to that classification. +4. Apply the review axes below plus the path-scoped rules under `docs/review/` that + match the changed paths: + - `src/SIL.Machine/Corpora/**/*.cs` -> `docs/review/corpora-usfm.md` + - `src/SIL.Machine.Morphology.HermitCrab/**/*.cs` -> `docs/review/hermitcrab.md` + - `src/SIL.Machine/**/*.cs`, `src/SIL.Machine.Translation.Thot/**/*.cs`, + `src/SIL.Machine.Tokenization.SentencePiece/**/*.cs`, and + `src/SIL.Machine.Translation.TensorFlow/**/*.cs` -> `docs/review/machine-library.md` + - `tests/**/*.cs` -> `docs/review/machine-tests.md` 5. Search for public declarations, interface implementations, project references, package metadata, tests, fixtures, and downstream references before concluding that a contract changed. @@ -131,6 +138,9 @@ checklist to unrelated pure algorithms. ## Validation - Prefer `./local_check.sh` (or `bash local_check.sh`) for the full local check when the environment supports it. +- For an agent-authored branch, require `./local_check.sh --agent-strict`: it adds + the comment-hygiene scan over the added lines. A green advisory `Comment + hygiene` check is not evidence that the strict scan passed. - Otherwise run the equivalent `dotnet tool restore`, `dotnet restore`, `dotnet csharpier check .`, `dotnet build --no-restore -c Release`, and `dotnet test --verbosity normal` commands. @@ -140,7 +150,10 @@ checklist to unrelated pure algorithms. reviewed head. This workflow is push/tag-triggered, not a `pull_request`-triggered gate. - Do not treat Codecov upload as a pass/fail threshold; inspect changed-line tests directly. -- If a command cannot run, state why and what remains unverified. Never convert unavailable evidence into a pass. +- If a command cannot run, state why and what remains unverified. Never convert + unavailable evidence into a pass. +- A search that finds nothing is not proof that a concept is absent unless you + state the scope you searched. ## Output format @@ -182,5 +195,5 @@ List commands with status (`passed`, `failed`, `not run`, or `blocked`), exact f ### Suggested Review Focus Name the one to three highest-value areas for a human reviewer. If an adversarial pass -is requested, use the separate library devil's-advocate role and keep its objections +is requested, apply `docs/review/devils-advocate.md` and keep its objections evidence-linked. diff --git a/.github/agents/devils-advocate.agent.md b/.github/agents/devils-advocate.agent.md deleted file mode 100644 index 4e4d6a5ad..000000000 --- a/.github/agents/devils-advocate.agent.md +++ /dev/null @@ -1,21 +0,0 @@ ---- -name: machine-devils-advocate -description: Read-only adversarial second pass for machine PR reviews, focused on compatibility, determinism, parity, memoization safety, and evidence gaps. -tools: ['read', 'search'] ---- - -# Machine Devil's Advocate - -Act as a skeptical senior reviewer after the normal machine PR review. Read the merge-base diff, the normal review output, relevant tests, project files, and cited history. Do not edit files, commit, push, or propose an unbounded rewrite. - -Challenge the most consequential conclusion first. Ask one objection at a time and support it with `path:line`, a concrete execution scenario, a consumer/fixture contract, or a missing command/artifact. Focus on: - -- public API/source/binary compatibility and target/package changes; -- false nullable promises or inconsistent interface implementations; -- cancellation, ordering, disposal, pooling, and parallel/sequential divergence; -- culture-sensitive token/marker/USFM behavior and Unicode regressions; -- incomplete HermitCrab keys, unsafe replay, retained-memory bounds, or unsupported performance claims; -- Python-port parity claims without a checked comparison; and -- tests that pass while leaving changed decisions or failure paths unverified. - -Do not call a concern a finding unless the evidence is present. Label each item `Verified objection` or `Unverified question`. Do not supply the solution in the objection section; state the evidence needed to settle it. Finish with `Top risk`, `Evidence still needed`, and `Would this block merge?` with a reason tied to the normal review severity contract. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md deleted file mode 100644 index c608707f8..000000000 --- a/.github/copilot-instructions.md +++ /dev/null @@ -1,28 +0,0 @@ -# Copilot guidance for machine - -Read `AGENTS.md` at the repository root for operational rules and `CONTEXT.md` -for domain vocabulary. Those are the shared sources of truth; do not restate -their rules here. Path-scoped review rules live in -`.github/instructions/*.instructions.md` and attach automatically to matching -files. - -Before making or reviewing a change, inspect the current source, project files, -tests, `local_check.sh`, and `.github/workflows/ci.yml`. Use actual code and -current configuration as evidence. A search that finds nothing is not proof that -a concept is absent unless you state the scope you searched. - -Pay particular attention to: - -- `netstandard2.0` library compatibility versus `net10.0` tools and tests; -- corpus row, tokenization, Scripture-reference, and alignment semantics; -- explicit string-comparison and culture choices in marker, token, and - identifier logic; -- the SentencePiece4c native boundary and its platform artifacts; -- disposal and lifetime of engines, models, trainers, and streams; -- deterministic tests and stable public API behavior; -- the current CI workflow rather than the stale `appveyor.yml` WebApi paths. - -Keep review comments concise, actionable, and anchored to a real file and line. -Avoid unrelated refactors and speculative claims. If a source or workflow fact -contradicts the documentation, report the discrepancy and prefer the verified -current behavior until the documentation is corrected. diff --git a/AGENTS.md b/AGENTS.md index f95a60f2b..781257ffc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -118,6 +118,19 @@ pull request. - This file is the shared operational source of truth. `CLAUDE.md` imports it. - Claude-specific workflows live under `.claude/skills/`. -- `.github/copilot-instructions.md` and `.github/instructions/*.instructions.md` - exist for GitHub Copilot compatibility and must not restate rules from here. +- Path-scoped review rules live under `docs/review/`. Match the changed path to + find the rules file: + + | Path glob | Rules file | + | --- | --- | + | `src/SIL.Machine/Corpora/**/*.cs` | `docs/review/corpora-usfm.md` | + | `src/SIL.Machine.Morphology.HermitCrab/**/*.cs` | `docs/review/hermitcrab.md` | + | `src/SIL.Machine/**/*.cs` | `docs/review/machine-library.md` | + | `src/SIL.Machine.Translation.Thot/**/*.cs` | `docs/review/machine-library.md` | + | `src/SIL.Machine.Tokenization.SentencePiece/**/*.cs` | `docs/review/machine-library.md` | + | `src/SIL.Machine.Translation.TensorFlow/**/*.cs` | `docs/review/machine-library.md` | + | `tests/**/*.cs` | `docs/review/machine-tests.md` | + + For a high-risk change, `docs/review/devils-advocate.md` is an optional + adversarial second pass. - Add a nested `AGENTS.md` only when a subtree genuinely needs different rules. diff --git a/CLAUDE.md b/CLAUDE.md index a46b39ff7..e7278b2bb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,6 +4,5 @@ - Keep repository-wide standing guidance in `AGENTS.md` and import it here. - Put Claude-only workflows and task procedures under `.claude/skills/`. -- Keep `.github/` for GitHub-required compatibility files: workflows, issue and - pull request templates, `copilot-instructions.md`, and - `.github/instructions/*.instructions.md`. +- Keep `.github/` for GitHub-required files: workflows and issue and pull + request templates. Path-scoped review rules live under `docs/review/`. diff --git a/.github/instructions/corpora-usfm-review.instructions.md b/docs/review/corpora-usfm.md similarity index 81% rename from .github/instructions/corpora-usfm-review.instructions.md rename to docs/review/corpora-usfm.md index fe4300c7c..1e14c885c 100644 --- a/.github/instructions/corpora-usfm-review.instructions.md +++ b/docs/review/corpora-usfm.md @@ -1,8 +1,9 @@ ---- -name: corpora-usfm-review -description: Review corpus and USFM changes for deterministic marker/token behavior, ScriptureRef and versification correctness, Unicode handling, and safe file inputs. -applyTo: "src/SIL.Machine/Corpora/**/*.cs" ---- +# Corpora and USFM Review + +*Review corpus and USFM changes for deterministic marker/token behavior, ScriptureRef +and versification correctness, Unicode handling, and safe file inputs.* + +Governs `src/SIL.Machine/Corpora/**/*.cs`. - Trace changed behavior from source text/file or corpus row through tokenization, parsing, ScriptureRef/`ScrVers` conversion, update handling, and emitted text. diff --git a/docs/review/devils-advocate.md b/docs/review/devils-advocate.md new file mode 100644 index 000000000..28661d258 --- /dev/null +++ b/docs/review/devils-advocate.md @@ -0,0 +1,26 @@ +# Machine Devil's Advocate + +*Read-only adversarial second pass for machine PR reviews, focused on compatibility, +determinism, parity, memoization safety, and evidence gaps.* + +Act as a skeptical senior reviewer after the normal machine PR review. Read the +merge-base diff, the normal review output, relevant tests, project files, and cited +history. Do not edit files, commit, push, or propose an unbounded rewrite. + +Challenge the most consequential conclusion first. Ask one objection at a time and +support it with `path:line`, a concrete execution scenario, a consumer/fixture contract, +or a missing command/artifact. Focus on: + +- public API/source/binary compatibility and target/package changes; +- false nullable promises or inconsistent interface implementations; +- cancellation, ordering, disposal, pooling, and parallel/sequential divergence; +- culture-sensitive token/marker/USFM behavior and Unicode regressions; +- incomplete HermitCrab keys, unsafe replay, retained-memory bounds, or unsupported performance claims; +- Python-port parity claims without a checked comparison; and +- tests that pass while leaving changed decisions or failure paths unverified. + +Do not call a concern a finding unless the evidence is present. Label each item +`Verified objection` or `Unverified question`. Do not supply the solution in the +objection section; state the evidence needed to settle it. Finish with `Top risk`, +`Evidence still needed`, and `Would this block merge?` with a reason tied to the normal +review severity contract. diff --git a/.github/instructions/hermitcrab-review.instructions.md b/docs/review/hermitcrab.md similarity index 84% rename from .github/instructions/hermitcrab-review.instructions.md rename to docs/review/hermitcrab.md index 9c17331b7..20e23659d 100644 --- a/.github/instructions/hermitcrab-review.instructions.md +++ b/docs/review/hermitcrab.md @@ -1,8 +1,9 @@ ---- -name: hermitcrab-review -description: Review HermitCrab morphology changes for analysis equivalence, memoization-key completeness, retained-memory bounds, parallelism, and hot-path cost. -applyTo: "src/SIL.Machine.Morphology.HermitCrab/**/*.cs" ---- +# HermitCrab Review + +*Review HermitCrab morphology changes for analysis equivalence, memoization-key +completeness, retained-memory bounds, parallelism, and hot-path cost.* + +Governs `src/SIL.Machine.Morphology.HermitCrab/**/*.cs`. - Treat analysis output as the primary contract. A faster parse, more memo hits, or a successful build does not prove equivalent analyses. diff --git a/.github/instructions/machine-library-review.instructions.md b/docs/review/machine-library.md similarity index 81% rename from .github/instructions/machine-library-review.instructions.md rename to docs/review/machine-library.md index 25567e398..7bfb838e4 100644 --- a/.github/instructions/machine-library-review.instructions.md +++ b/docs/review/machine-library.md @@ -1,8 +1,11 @@ ---- -name: machine-library-review -description: Review shipped machine library code for compatibility, deterministic language behavior, async contracts, resources, and focused tests. -applyTo: "src/SIL.Machine/**/*.cs,src/SIL.Machine.Translation.Thot/**/*.cs,src/SIL.Machine.Tokenization.SentencePiece/**/*.cs,src/SIL.Machine.Translation.TensorFlow/**/*.cs" ---- +# Machine Library Review + +*Review shipped machine library code for compatibility, deterministic language +behavior, async contracts, resources, and focused tests.* + +Governs `src/SIL.Machine/**/*.cs`, `src/SIL.Machine.Translation.Thot/**/*.cs`, +`src/SIL.Machine.Tokenization.SentencePiece/**/*.cs`, and +`src/SIL.Machine.Translation.TensorFlow/**/*.cs`. - Treat code in these projects as shipped library code. Check public/protected API shape, overloads, optional parameters, return types, XML documentation, target diff --git a/.github/instructions/machine-tests-review.instructions.md b/docs/review/machine-tests.md similarity index 87% rename from .github/instructions/machine-tests-review.instructions.md rename to docs/review/machine-tests.md index 6a5e8810a..5db507be6 100644 --- a/.github/instructions/machine-tests-review.instructions.md +++ b/docs/review/machine-tests.md @@ -1,8 +1,9 @@ ---- -name: machine-tests-review -description: Review machine tests as evidence for changed behavior, edge cases, contracts, and resource/cancellation boundaries. -applyTo: "tests/**/*.cs" ---- +# Machine Tests Review + +*Review machine tests as evidence for changed behavior, edge cases, contracts, and +resource/cancellation boundaries.* + +Governs `tests/**/*.cs`. - A test must exercise the changed behavior, not merely execute the changed method. Identify branches, guards, ordering, error paths, cancellation, limits, and side From c8f26ef4bb9ec7b9803c898e91ab10bad74c4ae7 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 18 Sep 2026 09:36:25 -0400 Subject: [PATCH 04/10] Rewrite authoring and review skills as style guides A PR comment nobody finishes reading changes nothing. The authoring and review skills were long enough to be skimmed: pr-authoring ran 203 lines and pr-review 199, and most of that was how to conduct a review rather than how to write one up. Each skill now covers style only. pr-review asks for many short comments anchored on the line they concern, one finding each, claim first, with a five-line summary; what to look for already lives in docs/review/, keyed by changed path. pr-authoring asks for a lede naming what a caller can now do, a top zone under 200 words, and everything longer below a rule in details blocks. issue-authoring asks for one symptom in the title and scannable labelled lines under it. The pull request template matches. The operational rules those skills carried move to AGENTS.md, which is where tool-neutral guidance belongs: a branch hygiene section with the range checks, the prohibition on reset --hard and broad staging, and the .review/ convention. Every skill description now fits the 120-column .editorconfig limit; the five front matter lines ran 156 to 325 columns. Co-Authored-By: Claude Opus 5 --- .claude/skills/code-comments/SKILL.md | 2 +- .claude/skills/commit-messages/SKILL.md | 2 +- .claude/skills/issue-authoring/SKILL.md | 96 ++++------ .claude/skills/pr-authoring/SKILL.md | 222 +++++------------------- .claude/skills/pr-review/SKILL.md | 222 +++++------------------- .github/PULL_REQUEST_TEMPLATE.md | 39 ++--- AGENTS.md | 13 ++ 7 files changed, 159 insertions(+), 437 deletions(-) diff --git a/.claude/skills/code-comments/SKILL.md b/.claude/skills/code-comments/SKILL.md index a51024034..485b6bdd3 100644 --- a/.claude/skills/code-comments/SKILL.md +++ b/.claude/skills/code-comments/SKILL.md @@ -1,6 +1,6 @@ --- name: code-comments -description: MUST use before writing or editing any comment in this repository, in .cs, .cpp/.h, .ps1/.psm1, .sh, or .py alike. Covers the content contract, the banned content categories, XML documentation rules, the 200-character budget for an implementation comment block, and the 120-column width limit that CI reports on. +description: MUST use before writing or editing any comment in this repository - content rules, budget, width. --- # Machine code comments diff --git a/.claude/skills/commit-messages/SKILL.md b/.claude/skills/commit-messages/SKILL.md index 72f3d9a55..373cbe1a3 100644 --- a/.claude/skills/commit-messages/SKILL.md +++ b/.claude/skills/commit-messages/SKILL.md @@ -1,6 +1,6 @@ --- name: commit-messages -description: Use before writing a commit message in sillsdev/machine; apply the repository commit conventions and check the new commit range before pushing. +description: How to write a commit message in sillsdev/machine - imperative subject, short body. --- # Commit messages diff --git a/.claude/skills/issue-authoring/SKILL.md b/.claude/skills/issue-authoring/SKILL.md index aad7854cf..d7d49197f 100644 --- a/.claude/skills/issue-authoring/SKILL.md +++ b/.claude/skills/issue-authoring/SKILL.md @@ -1,86 +1,52 @@ --- name: issue-authoring -description: Use when creating, refining, or triaging a GitHub issue in sillsdev/machine; produce evidence-based bug, feature, or machine.py porting issues without inventing Jira requirements. +description: How to write a GitHub issue in sillsdev/machine - one symptom, short body, real evidence. argument-hint: Optional issue type, title, symptoms, acceptance criteria, or source PR user-invocable: true --- -# Issue Authoring +# Writing a machine issue -Use GitHub issues as the native issue system for sillsdev/machine. This skill -supports Bug, Feature, and Porting issues. It does not fetch, assign, transition, -or comment on Jira tickets. An LT- reference may be included as an external -reference only when supplied and verified. +A style guide for the issue text. GitHub issues are the native tracker here; +this repository has no Jira workflow. An `LT-` reference is an external link +only, and only when someone supplied it. -## Common intake and evidence gate +Search open and recently closed issues first. Say what you searched. -1. Search existing open and recently closed GitHub issues for duplicates and - related PRs before drafting. -2. Ask for the smallest concrete example distinguishing a problem from an - enhancement request. -3. Separate observed facts, reproduction/acceptance evidence, and hypotheses. -4. Remove secrets, tokens, private data, and unsanitized customer/project data. -5. Name affected version/commit, OS, architecture, runtime, and package when - known. -6. If a fact is unknown, write Unknown and identify how to verify it. +## The title -An issue is ready when another maintainer can reproduce the bug, evaluate the -feature acceptance criteria, or identify the exact source change to port. +One symptom, in the reader's words, under about 70 characters. No "investigate", +no "improve", no component prefix the labels already carry. -## Bug issue +Bad: *Tokenizer improvements* +Good: *USFM attribute is dropped when the locale is tr-TR* -Require: +## The body -* concise symptom and affected package/API; -* version or commit, OS, architecture, and .NET runtime; -* minimal input, fixture, code sample, or repository state; -* exact reproduction steps and frequency; -* expected result and actual result; -* sanitized exception/log output; -* regression range or not known; and -* a minimal regression-test idea. +Short paragraphs or bullets, never a wall. Lead with the symptom and the one +fact that makes it reproducible. Everything else is a labelled line someone can +scan. Write `Unknown` where you do not know, and say how to find out. -If automation is feasible, propose a failing test before implementation and name -the likely test project. If not, state the concrete reason--visual/manual -behavior, unavailable external service, or packaging infrastructure--and give an -alternative verification plan. +**Bug** - affected package or API; version or commit, OS, runtime; the smallest +input that shows it; expected vs actual; sanitized log or exception; when it +started, or `Unknown`; the test that would catch it. -## Feature issue +**Feature** - who is blocked and by what; the behavior proposed, with its +compatibility cost; acceptance criteria an outsider could check; non-goals. -Require: +**Porting** - the source PR or commit URL, what behavior matters here, what does +not, and the target projects if known. -* user/problem statement and affected consumers; -* use cases and non-goals; -* proposed behavior or API, including compatibility concerns; -* observable acceptance criteria; -* test strategy and representative edge cases; -* performance/resource/platform constraints; and -* deliberately excluded follow-up work. +Sanitize before posting: no secrets, tokens, customer text, or private project +data. -Do not prescribe an implementation before behavior and acceptance criteria are -clear. Use Fixes #N only when closing the issue on merge is intended. +## Ready -## machine.py porting issue +An issue is ready when another maintainer can reproduce the bug, judge the +acceptance criteria, or find the exact change to port - without asking you a +question first. -Include source repository and PR/commit URL, target behavior to port, what is -not relevant to machine, verified target projects/files if known, -compatibility/test implications, and source validation evidence or an explicit -gap. +`create-porting-issue.yml` already files the porting issue after a merge, marked +`AUTO-GENERATED-ISSUE`. Do not write a second one by hand. -The existing merged-PR workflow normally creates the opposite-repository issue -with title Port '', label porting, and this body: - - Port any relevant changes in from to . - - - -Do not duplicate that issue. If a closing issue reference already contains the -marker, the workflow skips another generated issue. If the port is not covered -by a merged PR, create a normal Porting issue using the fields above. - -## Handoff - -Return proposed title, type/labels, complete body, duplicate-search result, -evidence gaps, and links. The author decides whether to publish it. Do not -claim that a test, reproduction, or external issue was checked unless it was -actually checked. +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 739e072c8..3dc558608 100644 --- a/.claude/skills/pr-authoring/SKILL.md +++ b/.claude/skills/pr-authoring/SKILL.md @@ -1,203 +1,75 @@ --- name: pr-authoring -description: Use when preparing, opening, updating, or responding to a GitHub pull request in sillsdev/machine; verify branch hygiene and actual validation evidence, then compose a concise reviewer-ready PR body. +description: How to write a pull request in sillsdev/machine - strong lede, short body, honest evidence. argument-hint: Optional branch purpose, issue number, or PR number user-invocable: true --- -# PR Authoring +# Writing a machine PR -Use this as the single entrypoint for preparing or updating a pull request in -sillsdev/machine. It prepares evidence and copy; it does not claim that a check -ran when it did not. Do not push, create, edit, or close a PR unless the user -explicitly requested that operation or confirmed a readiness offer. - -## 1. Announce the stages - -Tell the author that this run will: - -1. inspect the branch and target diff; -2. review contracts, implementation, tests, CI/dependencies, and packaging risk - appropriate to the changed files; -3. ask about Important/Critical risks and validation gaps; -4. run or record the exact applicable checks; and -5. prepare the PR body and review handoff. - -If a finding is ambiguous, ask for the missing product or design decision. If the -author cannot explain a changed mechanism, record the area as an understanding -gap instead of silently treating the change as safe. - -## 2. Branch and working-tree gate - -Run these read-only checks first: - -~~~text -git branch --show-current -git status --short -git symbolic-ref --short refs/remotes/origin/HEAD -git fetch origin --quiet -git merge-base origin/master HEAD -git diff --name-status origin/master...HEAD -git log --check --pretty=format:"---% h% s" origin/master.. -git diff --check origin/master...HEAD -~~~ - -The verified target default is origin/master. If the remote default changes, use -the resolved remote default and state it in the report; never silently use main. -Stop if the current branch is master and the request is to open a feature PR. -Preserve unrelated changes and pre-existing untracked files. Never use -reset --hard, checkout --, broad deletion, or broad staging as a cleanup -shortcut. - -Record merge base, HEAD, changed-file count, branch purpose, and pre-existing -work. A local review summary may be written to .review/, which .gitignore -excludes. Never commit it, and keep other transient notes outside the repo. - -## 3. Review the diff against the target's actual shape - -Use the smallest relevant set of passes: - -* **Contract/API:** public signatures, serialization, compatibility, package - metadata, and behavior visible to callers. -* **Implementation:** correctness, error handling, resource bounds, - cancellation/concurrency, determinism, and edge cases. -* **Tests and fixtures:** regression coverage, expected/actual behavior, - deterministic fixtures, test isolation, and platform assumptions. -* **Build/CI/dependencies:** project references, native SentencePiece boundary, - dependency changes, CI matrix, and NuGet packing when relevant. - -For a change under src/sentencepiece4c, include the exact matrix-specific CMake -commands from .github/workflows/ci.yml in required validation. For package or -project-file changes, inspect the package job's eight explicit dotnet pack -projects and say which package outputs are affected. Do not import FieldWorks -desktop, COM, installer, localization, or Jira rules into this review. - -Verify every named type, method, project, test, path, and count against the -current tree before putting it in the body. If a draft document conflicts with -code, correct the document or report the uncertainty; do not make code fit -stale prose. - -Classify findings as Critical, Important, or Minor. Keep positive observations -and evidence gaps separate. Deduplicate only identical concerns. - -## 4. Author interview - -Ask one Critical or Important question at a time: - -* What caller-visible behavior or contract changes? -* What makes the changed path safe for existing callers? -* Which test or fixture proves the reported bug or acceptance criterion? -* Which OS/runtime/package path is at risk? -* What was deliberately left out, and what would unblock it? - -For a vague answer, ask one focused follow-up and then record the concern as -unresolved if it remains unclear. For a small low-risk diff, do not manufacture -an interview; state that no interview was needed and why. - -## 5. Validation contract - -The authoritative managed CI sequence is: - -~~~text -dotnet tool restore -dotnet restore -dotnet csharpier check . -dotnet build --no-restore -c Release -dotnet test --verbosity normal --collect:"Xplat Code Coverage" -~~~ - -Run the sequence for a normal code/project change unless the author explicitly -chooses a narrower check and the report names omitted checks. local_check.sh is -a useful shortcut, but it does not collect coverage; do not call it -CI-equivalent. For native SentencePiece changes, also run the applicable Linux -or Windows CMake commands exactly as shown in ci.yml. For packaging changes, run -or record the affected dotnet pack ... -c Release -o artifacts command and -distinguish local results from the CI package job. - -An agent must also run `./local_check.sh --agent-strict`, which adds the -comment-hygiene scan and fails on any violation in the lines the branch adds. -It is required of agents and optional for humans; do not drop it to get a run -through. The advisory `Comment hygiene` pull request check going green is not -evidence that the strict scan passed. - -For documentation/skill/template-only changes, run dotnet csharpier check . -only if C# files are touched; otherwise use git diff --check and available -YAML/Markdown diagnostics. Always state skipped checks and why. Never claim -manual validation unless it was directly performed or explicitly confirmed by -the author. - -## 6. PR body - -Write the body from a file. Keep the top zone concise: - -~~~markdown -## Quick summary - - +A style guide for the PR text. The work itself - what to check, what to run - is +in `AGENTS.md` and `docs/review/`. - +Do not push, open, or edit a PR unless the author asked for it. -## Where to look +## The lede -- -- -- -- +First sentence: what a caller can now do, or what stopped being broken. Not what +you did, not how long it took, not which files moved. -## Deliberately not included +Bad: *This PR refactors the tokenizer and adds some tests.* +Good: *USFM markers now split identically under tr-TR; they used to lose the +attribute on a Turkish locale.* -- +If the change is invisible to callers, say what it protects instead: *Agents can +no longer land a comment that narrates its own history.* -## Validation +## Body -- -- +Keep the top zone under 200 words. Sections, in order, and drop any that are +empty: -## Issue / porting context +```markdown +## Quick summary + - -~~~ +## Where to look +- -- -The top zone should normally be 200-400 words for a substantive PR, but clarity -beats an artificial count for a tiny change. Do not open with process narration, -apology, or "should be fine." Do not duplicate proof in every section. +## Deliberately not included +- -When durable reasoning matters, put it below a horizontal rule in closed details -sections: Reading this a year from now; Decisions, and why; Paths not taken; -What this does NOT authorize; Deferred, and what would unblock it; and Evidence. +## Validation +- -- -Synthesize reasoning; do not paste scratchpads. Do not automatically delete -markdown from the branch. If temporary research is intentionally removed, list -the files and obtain confirmation before deletion. +## Issue / porting context + +``` -## 7. Machine.py porting +Everything else goes below 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. It is not welcome above the +rule. -For a change ported from sillsdev/machine.py, identify the source PR/commit, -state which behavior is relevant to this repository, and include affected -tests. create-porting-issue.yml creates the opposite-repository issue after a -merged PR, with title Port '', label porting, and an -AUTO-GENERATED-ISSUE marker. Do not create a duplicate by hand when that -workflow has already created one. +No preamble, no apology, no "should be fine", no recap of the section above. -## 8. Review comments +## Validation lines -For each review comment, human or automated, classify it as: +Write the command and its result, nothing else. Never list a command you did not +run, and never call a local run CI-equivalent - CI collects coverage and +`local_check.sh` does not. If a check was skipped, say which and why. -* **Fix** -- technically sound and unambiguous; make the minimum change. -* **Clarify** -- missing context or a design decision; ask the specific question. -* **Reply only** -- explain verified behavior without changing code. -* **Defer** -- record the follow-up and why it is outside this PR. +Verify every count, path, type, and test name against the tree before it goes in +the body. A wrong number in a PR body outlives the PR. -Validate fixes with applicable commands. Reply in the anchored GitHub thread. -Resolve only an unresolved thread that the API permits resolving and that is -fully answered with no open question. Do not resolve disputed, ambiguous, -unverified, or deferred comments. +## Replying to review comments -## 9. Readiness handoff +Reply in the thread, on the line. Classify first, then act: -Before offering to publish, report branch, base, merge base, changed files, -findings/status, exact checks run/skipped/red, issue/porting links, -commit-message/whitespace status, and reviewer focus. +* **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. -Only after confirmation may the workflow stage intended files, commit, push, -create/update the PR, or update its body. Preserve unrelated staged changes and -ask before including them. +One reply per comment, two or three sentences. 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 index c16766fbe..7e4999a93 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -1,199 +1,73 @@ --- name: pr-review -description: Evidence-first review of machine pull requests and branch diffs, focused on public library compatibility, deterministic language processing, async contracts, HermitCrab performance, tests, and packaging. +description: How to write up a code review in sillsdev/machine - short line comments, evidence, severity. argument-hint: "[optional PR number, branch, or review focus]" +user-invocable: true --- -# Machine PR Review - -Review the current machine branch or named pull request. This is a read-only review. Do -not edit, commit, push, resolve comments, or open a PR unless a separate workflow -explicitly authorizes that action. - -## Review contract - -- Review the actual branch diff from the merge-base with the PR base, normally - `origin/master`, not today's commits and not filenames alone. -- Read the changed code, relevant tests, project files, repository instructions, and the - directly affected public callers or fixtures before making a finding. -- Use only findings grounded in code, a reproducible command, a test/result, a cited - history change, or a cited consumer contract. -- Every finding must include `path:line` and a concrete consequence. -- Mark each statement as `Verified` or `Unverified follow-up`. An unverified concern is never a blocking finding. -- Do not report a pre-existing issue unless the diff changes its behavior, exposes it - through a changed contract, or the review explicitly asks for a baseline audit. -- Do not request a broad modernization (nullable migration, async streaming, - language-version change, analyzer cleanup, or benchmark suite) without a changed-code - reason. - -## Establish scope - -1. Confirm the repository, branch, worktree, and base. Use `git status --short - --branch`, `git merge-base HEAD`, and `git diff --stat ...HEAD`. -2. Read `README.md`, `.editorconfig`, `.csharpierrc.yaml`, the relevant solution/project - files, and any target-local instructions. -3. Classify changed files: shipped API/library, tests, HermitCrab, corpus/USFM, - SentencePiece/native, tool, workflow/package, or documentation. -4. Apply the review axes below plus the path-scoped rules under `docs/review/` that - match the changed paths: - - `src/SIL.Machine/Corpora/**/*.cs` -> `docs/review/corpora-usfm.md` - - `src/SIL.Machine.Morphology.HermitCrab/**/*.cs` -> `docs/review/hermitcrab.md` - - `src/SIL.Machine/**/*.cs`, `src/SIL.Machine.Translation.Thot/**/*.cs`, - `src/SIL.Machine.Tokenization.SentencePiece/**/*.cs`, and - `src/SIL.Machine.Translation.TensorFlow/**/*.cs` -> `docs/review/machine-library.md` - - `tests/**/*.cs` -> `docs/review/machine-tests.md` -5. Search for public declarations, interface implementations, project references, - package metadata, tests, fixtures, and downstream references before concluding that a - contract changed. - -## Review axes - -### Public API, binary compatibility, and package contract - -For changes under shipped library projects, inspect public/protected types and members, -overloads, optional parameters, interface shape, return types, serialized models, target -frameworks, package references, assembly/package versioning, and XML documentation. A -removal, narrowing, incompatible default/behavior, or accidental target/package change -is a finding only when the diff and contract establish the consequence. Check -implementations and tests of changed interfaces. Do not infer downstream breakage from a -public-looking filename. - -### Nullable annotations - -The current shipped libraries do not opt into nullable reference types, while test -projects do. Review `#nullable`, project settings, `?`, null-forgiving operators, and -generated/public annotations when touched. Flag an annotation that promises a false null -contract or creates inconsistent implementation/interface behavior. Do not require a -repository-wide NRT migration as part of an unrelated change. - -### Async, cancellation, and concurrency - -The public API uses `Task` and optional `CancellationToken`; corpus/tokenizer APIs are -synchronous, and the current source has no `IAsyncEnumerable` surface. For changed async -code, verify token propagation to every meaningful wait/I/O/dataflow operation, ordering -and partial-result behavior, disposal/pool lifetime, and absence of sync-over-async -deadlocks. Check `ConfigureAwait(false)` where library context capture is not intended. -A new async-streaming API requires an explicit compatibility and consumer decision. - -### Determinism and culture - -For token, marker, identifier, persisted, protocol, dictionary-order, and serialization -logic, verify explicit comparison/equality/culture choices. Use ordinal comparison for -identity/search where the domain is byte/code-point identity; use invariant or -culture-aware behavior only when the semantic contract requires it. The `418ff225` -`StringComparison.Ordinal` fix and Hindi regression test are precedent, not a blanket -replacement rule. Add a non-English-culture test when the changed behavior can vary by -culture. - -### HermitCrab semantic/performance safety - -For `src/SIL.Machine.Morphology.HermitCrab/**`, verify that analysis-state keys include -every field read by rules, frozen/mutable objects cannot invalidate keys, replay -preserves analysis results and prefixes, memo entries are complete before storage, and -resource/parallelism caps remain effective. Review allocations and retained lifetimes in -inner loops when changed. Require focused semantic tests for behavior changes and a -benchmark or measured artifact when the PR claims performance improvement. Do not accept -hit-count improvement as proof of semantic equivalence. +# Writing a machine review -### Python parity +This is a style guide for the review you publish, not a method for doing the +review. What to look for lives in `docs/review/`, keyed by changed path: -For a port or algorithm explicitly shared with `machine.py`, require a cited Python -source/commit/issue and paired behavior evidence. The post-merge porting workflow is -follow-up coordination, not proof. If the Python source is unavailable, record parity as -`Unverified follow-up`; do not invent a comparison or block without a stated parity -contract. +| Path | Rules | +| --- | --- | +| `src/SIL.Machine/Corpora/**/*.cs` | `docs/review/corpora-usfm.md` | +| `src/SIL.Machine.Morphology.HermitCrab/**/*.cs` | `docs/review/hermitcrab.md` | +| other `src/**/*.cs` | `docs/review/machine-library.md` | +| `tests/**/*.cs` | `docs/review/machine-tests.md` | -### Tests and coverage - -Map changed branches, guards, error paths, ordering, resource limits, and side effects -to tests. Prefer focused tests in the affected test project, including Unicode/culture, -cancellation, malformed input, and boundary cases where relevant. Run the smallest -meaningful command first and then the repository check when practical. Report exact -commands, results, filters, and exclusions. Coverage percentage alone is not evidence -that changed decisions are covered. - -### Formatting and repository checks - -Run `dotnet csharpier check .`, `dotnet build --no-restore -c Release`, and `dotnet test ---verbosity normal` as applicable. `local_check.sh` is the canonical combined local -sequence. Respect `.editorconfig` and CSharpier's 120-column configuration. Formatting -is normally an Important/Minor issue, never a Critical issue by itself. +A review is read-only. Do not edit, commit, push, or resolve threads. -### USFM, ScriptureRef, and versification +## Shape -For `src/SIL.Machine/Corpora/**` changes involving USFM, ScriptureRef, `ScrVers`, -tokenization, or update handlers, trace input -> parse/tokenize -> reference mapping -> -output. Require paired fixtures or assertions for valid, empty, malformed, nested, -Unicode, and cross-versification cases appropriate to the change. Verify marker and -token identity comparisons are deterministic. Do not claim render parity; this library -has semantic text behavior, not FieldWorks desktop rendering. +**Many short comments, not one long one.** Anchor each finding on the line it is +about. A reviewer scrolling the diff should meet each point where it applies. -### Input, resource, and native boundary safety +**One finding per comment.** Two problems on one line are two comments. -For changed streams, archives, paths, subprocess/tool inputs, or SentencePiece native -loading, check size limits, path/entry validation, disposal, error propagation, platform -selection, and secrets. Cite the changed boundary and test/evidence. Do not apply this -checklist to unrelated pure algorithms. +**Lead with the claim.** First sentence names the defect. Evidence second, fix +third, and only if it fits. -## Validation +``` +Ordinal comparison missing: `marker.IndexOf(":")` is culture-sensitive, so +tr-TR splits this marker differently. Pass `StringComparison.Ordinal`. +``` -- Prefer `./local_check.sh` (or `bash local_check.sh`) for the full local check when the environment supports it. -- For an agent-authored branch, require `./local_check.sh --agent-strict`: it adds - the comment-hygiene scan over the added lines. A green advisory `Comment - hygiene` check is not evidence that the strict scan passed. -- Otherwise run the equivalent `dotnet tool restore`, `dotnet restore`, `dotnet - csharpier check .`, `dotnet build --no-restore -c Release`, and `dotnet test - --verbosity normal` commands. -- For focused behavior, run the relevant test project with an explicit filter and then - state whether the full matrix was run. -- Treat the visible GitHub `CI Build` result as evidence only when its commit SHA is the - reviewed head. This workflow is push/tag-triggered, not a `pull_request`-triggered - gate. -- Do not treat Codecov upload as a pass/fail threshold; inspect changed-line tests directly. -- If a command cannot run, state why and what remains unverified. Never convert - unavailable evidence into a pass. -- A search that finds nothing is not proof that a concept is absent unless you - state the scope you searched. +Three lines is a long comment. If one needs more, the finding is really a +design question - ask it in the summary instead. -## Output format +## Severity -### Contract/API Changes Summary +Prefix each comment: **Critical** (blocks merge), **Important**, or **Minor**. +Critical means demonstrated - a failing command, a broken contract, a missing +gate. A worry is not Critical. -State whether shipped public APIs, target frameworks, package metadata, native -artifacts, serialized formats, or parity contracts changed. Say `None verified` when -appropriate. +## Evidence -### Findings +Every comment carries `path:line` and a consequence. Mark anything you did not +confirm as `Unverified`, and never let an unverified concern block a merge. -#### Critical (must address) +Do not report pre-existing issues the diff does not touch. Do not ask for a +migration, a modernization, or a benchmark suite the diff gave no reason for. A +search that found nothing proves absence only if you state what you searched. -- `[Verified] path:line -- consequence; evidence/command; required correction.` +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 the +advisory `Comment hygiene` check going green does not stand in for it. A +coverage percentage is not evidence that a changed line is tested. -Use only for demonstrated correctness, security/resource, compatibility, build/package, -or test-gate failures that block a safe merge. +## Summary comment -#### Important (should address) +Five lines at most: -- `[Verified] path:line -- concrete risk or missing contract evidence; evidence/command; requested action.` +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. -#### Minor (consider) +State `None verified` where that is the honest answer. Say which public API, +target framework, package, or parity contract changed, or that none did. -- `[Verified] path:line -- scoped maintainability/style/test improvement.` - -#### Unverified follow-ups - -- `[Unverified] concern -- exact evidence still needed; why it is not a finding.` - -### Positive Observations - -List concrete safeguards or tests that the diff preserves or adds. - -### Required Validation - -List commands with status (`passed`, `failed`, `not run`, or `blocked`), exact filters, artifacts, and remaining gaps. - -### Suggested Review Focus - -Name the one to three highest-value areas for a human reviewer. If an adversarial pass -is requested, apply `docs/review/devils-advocate.md` and keep its objections -evidence-linked. +For an adversarial second pass, apply `docs/review/devils-advocate.md`. diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index dca93775b..68d1065e2 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,34 +1,31 @@ ## Quick summary - + -## Issue / porting context - - - -## What changed +## Where to look - + ## Deliberately not included - + ## Validation -- [ ] dotnet tool restore -- [ ] dotnet restore -- [ ] dotnet csharpier check . -- [ ] dotnet build --no-restore -c Release -- [ ] dotnet test --verbosity normal -- [ ] ./local_check.sh --agent-strict (required for agents; optional for humans) -- [ ] Applicable SentencePiece CMake build run, or not relevant -- [ ] Applicable package output checked, or not relevant -- [ ] git diff --check and commit-range whitespace checked + - +- `./local_check.sh` -- +- `./local_check.sh --agent-strict` -- +- `git diff --check ...HEAD` -- +- + +## Issue / porting context -## Reviewer focus + - + diff --git a/AGENTS.md b/AGENTS.md index 781257ffc..182098485 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,6 +48,19 @@ CSharpier is required for C#. The tool is pinned in `.config/dotnet-tools.json`, CI also collects coverage (`--collect:"Xplat Code Coverage"`); `local_check.sh` does not, so do not describe a local run as coverage-equivalent. +## Branch hygiene + +Before opening or updating a pull request, confirm the branch and the range: +`git status --short --branch`, `git merge-base origin/master HEAD`, +`git diff --check ...HEAD`, and `git log --check origin/master..`. +The target default is `origin/master`; if the remote default changes, use the +resolved default and say so. + +Preserve unrelated changes and pre-existing untracked files. Never use +`reset --hard`, `checkout --`, broad deletion, or broad staging as a cleanup +shortcut. A local review summary may be written to `.review/`, which +`.gitignore` excludes; keep other transient notes outside the repository. + ## Comment hygiene Agents must run `./local_check.sh --agent-strict`. It adds From db4a7f337cbc84972c048499baa1fa44659829e9 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 18 Sep 2026 09:45:29 -0400 Subject: [PATCH 05/10] Require a three-sentence lede for PRs and issues A reader decides whether to keep reading in the first three sentences, so both authoring skills now say what those three sentences must carry. For a PR: what a caller can now do, the reviewer's first unknown answered, and what the change does not touch. For an issue: the symptom, the smallest trigger, and the cost. Each under about 25 words; anything needing a subordinate clause belongs in the body. The shape comes from the FieldWorks pr-pitch skill, which opens with two or three sentences naming the concrete thing rather than the framing. The three issue forms gain a required Summary field that asks for the same three sentences, and the pull request template's Quick summary comment now names them instead of describing a paragraph. Co-Authored-By: Claude Opus 5 --- .claude/skills/issue-authoring/SKILL.md | 22 ++++++++++++++++++--- .claude/skills/pr-authoring/SKILL.md | 23 ++++++++++++++++------ .github/ISSUE_TEMPLATE/bug_report.yml | 13 ++++++++++++ .github/ISSUE_TEMPLATE/feature_request.yml | 9 +++++++++ .github/ISSUE_TEMPLATE/porting_request.yml | 9 +++++++++ .github/PULL_REQUEST_TEMPLATE.md | 5 +++-- 6 files changed, 70 insertions(+), 11 deletions(-) diff --git a/.claude/skills/issue-authoring/SKILL.md b/.claude/skills/issue-authoring/SKILL.md index d7d49197f..6e0d6f956 100644 --- a/.claude/skills/issue-authoring/SKILL.md +++ b/.claude/skills/issue-authoring/SKILL.md @@ -21,11 +21,27 @@ no "improve", no component prefix the labels already carry. Bad: *Tokenizer improvements* Good: *USFM attribute is dropped when the locale is tr-TR* +## The lede + +Three short sentences, each doing a different job. Nothing else before them. + +1. **The symptom.** What goes wrong, in the reader's terms. +2. **The trigger.** The smallest condition that produces it - input, locale, + platform, version. +3. **The cost.** Who is blocked, what is lost, or what the caller sees instead. + +Good: *A USFM attribute is dropped when the tokenizer runs under tr-TR. Any +marker containing an ASCII `i` splits at the wrong index on a Turkish locale. +Round-tripping a Turkish project silently loses the attribute.* + +For a feature, the same three: what is missing, when it bites, what it costs. + +Keep each sentence under about 25 words. Everything longer goes in the body. + ## The body -Short paragraphs or bullets, never a wall. Lead with the symptom and the one -fact that makes it reproducible. Everything else is a labelled line someone can -scan. Write `Unknown` where you do not know, and say how to find out. +Short paragraphs or bullets, never a wall. Everything is a labelled line someone +can scan. Write `Unknown` where you do not know, and say how to find out. **Bug** - affected package or API; version or commit, OS, runtime; the smallest input that shows it; expected vs actual; sanitized log or exception; when it diff --git a/.claude/skills/pr-authoring/SKILL.md b/.claude/skills/pr-authoring/SKILL.md index 3dc558608..8f05e0e21 100644 --- a/.claude/skills/pr-authoring/SKILL.md +++ b/.claude/skills/pr-authoring/SKILL.md @@ -14,16 +14,27 @@ Do not push, open, or edit a PR unless the author asked for it. ## The lede -First sentence: what a caller can now do, or what stopped being broken. Not what -you did, not how long it took, not which files moved. +Three short sentences, each doing a different job. Nothing else before them. + +1. **What it does.** What a caller can now do, or what stopped being broken. + Not what you did, not how long it took, not which files moved. +2. **The reviewer's first unknown, answered.** Usually "what breaks?" or "why is + it this big?" Answer it here; do not make them read for it. +3. **The boundary.** What the change does not touch, or the one condition that + keeps it safe. Bad: *This PR refactors the tokenizer and adds some tests.* -Good: *USFM markers now split identically under tr-TR; they used to lose the -attribute on a Turkish locale.* -If the change is invisible to callers, say what it protects instead: *Agents can +Good: *USFM markers now split identically under tr-TR, where the attribute used +to be dropped. No public signature changes - the fix is one comparison, from +culture-aware to ordinal. Nothing outside `UsfmTokenizer` is touched.* + +If the change is invisible to callers, lead with what it protects: *Agents can no longer land a comment that narrates its own history.* +Keep each sentence under about 25 words. If a sentence needs a subordinate +clause to survive, it belongs in the body. + ## Body Keep the top zone under 200 words. Sections, in order, and drop any that are @@ -31,7 +42,7 @@ empty: ```markdown ## Quick summary - + ## Where to look - -- diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml index 7ff9f52e5..83c3f06fe 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.yml +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -8,6 +8,19 @@ body: 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 under tr-TR. + Any marker containing an ASCII i splits at the wrong index on a Turkish locale. + Round-tripping a Turkish project silently loses the attribute. + validations: + required: true - type: input id: package attributes: diff --git a/.github/ISSUE_TEMPLATE/feature_request.yml b/.github/ISSUE_TEMPLATE/feature_request.yml index 801700ee6..f84b15e09 100644 --- a/.github/ISSUE_TEMPLATE/feature_request.yml +++ b/.github/ISSUE_TEMPLATE/feature_request.yml @@ -4,6 +4,15 @@ 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: diff --git a/.github/ISSUE_TEMPLATE/porting_request.yml b/.github/ISSUE_TEMPLATE/porting_request.yml index ad3b8a571..f09b8a45b 100644 --- a/.github/ISSUE_TEMPLATE/porting_request.yml +++ b/.github/ISSUE_TEMPLATE/porting_request.yml @@ -8,6 +8,15 @@ body: 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: diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 68d1065e2..fa0dbd9c7 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,7 +1,8 @@ ## Quick summary - + ## Where to look From fb939424da0dbc879b3b26f9f1cb73510ff4848e Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 18 Sep 2026 11:42:00 -0400 Subject: [PATCH 06/10] Focus review guidance on what the history shows breaks Three years of commits and review comments say this repository is USFM, Scripture references, and HermitCrab, and the guidance did not say that. Since src/SIL.Machine.AspNetCore was removed in 69796806 there have been 232 commits: 94 touch Corpora, 50 touch HermitCrab, 15 touch PunctuationAnalysis, and 5 touch every translation engine and tokenizer combined. Review comments follow the same shape, and 60 of the 232 commits are fixes. docs/review/punctuation.md is new. That area had no rules file and its history is almost entirely crash fixes: a surrogate pair split in 03621d14, an invalid chapter in 36a24b57, wrong chapter numbers in f9ba7bb7. corpora-usfm.md now names reference and versification arithmetic as the class that recurs most, with the five shas behind it. machine-library.md keeps ordinal comparison and stream ownership, each with the commits where they shipped as bugs, and drops rules with no history behind them. machine-tests.md asks for the test by name, since "where is the test" is the most common review question here. The SentencePiece CMake commands and the AppVeyor paragraph are gone from AGENTS.md; the native boundary saw one commit in twenty-six months, and ci.yml is where those commands are true. The path glob table now routes punctuation work, states that the first match wins, and is no longer repeated in the pr-review skill. The three path-scoped files no longer restate the validation commands AGENTS.md owns. The review skill now asks what happened to each finding. Of 140 review threads in the last three years, 128 have no author follow-up, so a reader cannot tell which findings changed the code. Co-Authored-By: Claude Opus 5 --- .claude/skills/pr-review/SKILL.md | 17 +++++------ AGENTS.md | 25 ++++++---------- docs/review/corpora-usfm.md | 10 +++---- docs/review/hermitcrab.md | 2 -- docs/review/machine-library.md | 47 +++++++++++++++---------------- docs/review/machine-tests.md | 7 ++--- docs/review/punctuation.md | 24 ++++++++++++++++ 7 files changed, 70 insertions(+), 62 deletions(-) create mode 100644 docs/review/punctuation.md diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 7e4999a93..b5c326466 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -8,14 +8,8 @@ user-invocable: true # Writing a machine review This is a style guide for the review you publish, not a method for doing the -review. What to look for lives in `docs/review/`, keyed by changed path: - -| Path | Rules | -| --- | --- | -| `src/SIL.Machine/Corpora/**/*.cs` | `docs/review/corpora-usfm.md` | -| `src/SIL.Machine.Morphology.HermitCrab/**/*.cs` | `docs/review/hermitcrab.md` | -| other `src/**/*.cs` | `docs/review/machine-library.md` | -| `tests/**/*.cs` | `docs/review/machine-tests.md` | +review. What to look for lives in `docs/review/`; `AGENTS.md` maps a changed +path to its rules file. A review is read-only. Do not edit, commit, push, or resolve threads. @@ -70,4 +64,11 @@ Five lines at most: State `None verified` where that is the honest answer. Say which public API, target framework, package, or parity contract changed, or that none did. +## Closing a thread + +Say what happened to each finding: **changed**, **accepted** (the author +answered and you agree), or **unverified** (nobody settled it). Of 140 review +threads in this repository's last three years, 128 have no author follow-up at +all, so the reader cannot tell which findings mattered. Leave nothing implicit. + For an adversarial second pass, apply `docs/review/devils-advocate.md`. diff --git a/AGENTS.md b/AGENTS.md index 182098485..e2341b9df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,16 +81,8 @@ strict check passed. ## Native SentencePiece boundary CI builds `src/sentencepiece4c` separately and feeds the platform-specific -artifact to the managed build and package job. The verified forms are: - -``` -cmake -S src/sentencepiece4c -B src/sentencepiece4c/build -G Ninja -DCMAKE_BUILD_TYPE=Release -cmake --build src/sentencepiece4c/build --config Release --target sentencepiece4c -``` - -Windows CI configures with `-A x64` instead of the Ninja generator. This is the -only native boundary in the repository; it is not a general native-before-managed -build policy. +artifact to the managed build and package job. If you change it, copy the +current CMake commands from `.github/workflows/ci.yml` rather than from here. ## Tests and changes @@ -114,9 +106,8 @@ native SentencePiece build, CSharpier check, Release build, tests with coverage, and tag-triggered NuGet publishing. It triggers on `push`, not on `pull_request`, so a green check on a PR reflects the pushed head rather than a PR event. -`appveyor.yml` is legacy. It still references `SIL.Machine.WebApi` projects that -are absent from the tree. Do not add projects to satisfy it, and do not treat its -VS2019 assumptions as the current target matrix. +`appveyor.yml` is legacy and describes projects that are not in the tree. +Ignore it. ## Porting to and from machine.py @@ -137,13 +128,13 @@ pull request. | Path glob | Rules file | | --- | --- | | `src/SIL.Machine/Corpora/**/*.cs` | `docs/review/corpora-usfm.md` | + | `src/SIL.Machine/PunctuationAnalysis/**/*.cs` | `docs/review/punctuation.md` | | `src/SIL.Machine.Morphology.HermitCrab/**/*.cs` | `docs/review/hermitcrab.md` | - | `src/SIL.Machine/**/*.cs` | `docs/review/machine-library.md` | - | `src/SIL.Machine.Translation.Thot/**/*.cs` | `docs/review/machine-library.md` | - | `src/SIL.Machine.Tokenization.SentencePiece/**/*.cs` | `docs/review/machine-library.md` | - | `src/SIL.Machine.Translation.TensorFlow/**/*.cs` | `docs/review/machine-library.md` | + | any other `src/**/*.cs` | `docs/review/machine-library.md` | | `tests/**/*.cs` | `docs/review/machine-tests.md` | + The first matching row wins. + For a high-risk change, `docs/review/devils-advocate.md` is an optional adversarial second pass. - Add a nested `AGENTS.md` only when a subtree genuinely needs different rules. diff --git a/docs/review/corpora-usfm.md b/docs/review/corpora-usfm.md index 1e14c885c..3de291eaa 100644 --- a/docs/review/corpora-usfm.md +++ b/docs/review/corpora-usfm.md @@ -10,13 +10,13 @@ Governs `src/SIL.Machine/Corpora/**/*.cs`. - Use ordinal comparison for marker, token, identifier, and protocol identity unless the code's contract is explicitly linguistic or user-facing. Do not blanket-replace culture-aware comparisons; justify the semantic choice. -- For USFM/versification changes, add paired input/output or reference assertions - covering the affected book/chapter/verse mapping, marker nesting, empty/malformed - input, and Unicode case relevant to the change. +- Reference and versification arithmetic is the defect class that recurs most here + (`4e889539`, `54687760`, `8d924c1a`, `f9ba7bb7`, `78350670`). For any change that + touches it, add paired input/output or reference assertions over the affected + book/chapter/verse mapping, marker nesting, empty and malformed input, and the + relevant Unicode case. - Check that missing, duplicate, or ambiguous references fail or resolve according to the existing contract. Do not treat a parser snapshot as proof of visual rendering parity. - For files, ZIPs, and streams, preserve entry/byte limits, path validation, disposal, cancellation, and actionable errors. -- Use existing corpus/USFM test helpers and run focused tests plus the normal test/build - checks. Report any fixture, full-suite, or culture/platform evidence not run. diff --git a/docs/review/hermitcrab.md b/docs/review/hermitcrab.md index 20e23659d..02ec7ee86 100644 --- a/docs/review/hermitcrab.md +++ b/docs/review/hermitcrab.md @@ -21,5 +21,3 @@ Governs `src/SIL.Machine.Morphology.HermitCrab/**/*.cs`. artifact in addition to semantic regression tests. - Exercise sequential and parallel behavior where the changed path supports both, and test cancellation/disposal if a boundary is asynchronous. -- Run focused HermitCrab tests and the normal release test/build checks. State any - unavailable benchmark, large-corpus, or platform evidence explicitly. diff --git a/docs/review/machine-library.md b/docs/review/machine-library.md index 7bfb838e4..8c69b7b5c 100644 --- a/docs/review/machine-library.md +++ b/docs/review/machine-library.md @@ -1,29 +1,26 @@ # Machine Library Review -*Review shipped machine library code for compatibility, deterministic language -behavior, async contracts, resources, and focused tests.* +*Review shipped library code for compatibility, deterministic comparison, async +contracts, and disposal.* -Governs `src/SIL.Machine/**/*.cs`, `src/SIL.Machine.Translation.Thot/**/*.cs`, -`src/SIL.Machine.Tokenization.SentencePiece/**/*.cs`, and -`src/SIL.Machine.Translation.TensorFlow/**/*.cs`. +Governs any `src/**/*.cs` no more specific rules file claims. -- Treat code in these projects as shipped library code. Check public/protected API - shape, overloads, optional parameters, return types, XML documentation, target - framework, package references, and assembly/package versioning when touched. -- The current library target is `netstandard2.0`; do not introduce a target or API that - silently removes existing consumers. Do not demand nullable reference types for the - whole library, but review any changed annotations or `#nullable` contract carefully - because test projects enable nullable while shipped libraries do not. -- Existing public asynchronous APIs use `Task` and `CancellationToken`; existing - corpus/tokenizer APIs are synchronous. Verify cancellation propagation, ordering, - disposal, and library-context behavior in changed async code. Treat a new - `IAsyncEnumerable` surface as an explicit compatibility decision. -- Use explicit culture/comparison semantics for token, marker, identifier, serialized, - and protocol logic. Preserve genuinely linguistic culture-aware behavior. Add a - culture regression test when the changed code can vary by culture. -- For file, stream, archive, process, or native-DLL changes, check bounds, paths, - disposal, platform selection, and failure propagation. -- Add or update focused tests for changed decisions and boundary cases. Report commands - and gaps; do not use a coverage percentage as the sole proof. -- Run `dotnet csharpier check .`, the relevant test project, and the release build as - appropriate. Respect `.editorconfig` and CSharpier's 120-column width. +- Treat this as shipped library code. Check public and protected API shape, + overloads, optional parameters, return types, and XML documentation when they + change. The target is `netstandard2.0`; do not introduce an API that silently + drops existing consumers. +- Use ordinal comparison for markers, tokens, identifiers, and protocol text. + Reserve culture-sensitive comparison for genuinely linguistic operations. This + is the defect class review misses most often here: `075c6ea1`, `dac2d895`, and + `418ff225` all shipped it. +- Existing async APIs use `Task` and `CancellationToken`; corpus and tokenizer + APIs are synchronous. In changed async code verify cancellation propagation, + ordering, and disposal. +- Dispose engines, models, trainers, and streams according to their contracts, + and check who owns a stream that is passed in - `5a488c01` and `37e13b79` were + both ownership bugs. +- Test projects enable nullable reference types and the shipped libraries do + not. Review a changed annotation for a false promise; do not ask for a + repository-wide migration. +- 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 5db507be6..04d41c4b9 100644 --- a/docs/review/machine-tests.md +++ b/docs/review/machine-tests.md @@ -18,8 +18,5 @@ Governs `tests/**/*.cs`. - For HermitCrab changes, compare analysis semantics, not only memo-hit counts or execution success. Exercise key completeness, replay, resource caps, and parallel/sequential equivalence when touched. -- For public API changes, include compile/use coverage for the changed signature and - document any consumer or target-framework evidence that was not available. -- Run the focused test command, `dotnet test --verbosity normal`, and coverage - collection when useful. Record filters, skipped tests, failures, and unverified - changed lines. A global coverage percentage is not changed-line evidence. +- Name the test that proves the change. "Where is the test?" is the single most + common review question in this repository; answer it before it is asked. diff --git a/docs/review/punctuation.md b/docs/review/punctuation.md new file mode 100644 index 000000000..0395605a0 --- /dev/null +++ b/docs/review/punctuation.md @@ -0,0 +1,24 @@ +# Punctuation Analysis Review + +*Review quotation-mark and punctuation analysis for Unicode safety, malformed +input, and chapter/verse bookkeeping.* + +Governs `src/SIL.Machine/PunctuationAnalysis/**/*.cs`. + +This area's history is almost entirely crash fixes, 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 `char`. A surrogate pair, a combining mark, or a + multi-byte quotation mark must not split - `03621d14` fixed a crash from + exactly that. +- Assume the chapter or verse is missing, out of range, or unparsable. The + resolver runs over real Paratext projects: `36a24b57` fixed a crash on an + invalid chapter and `f9ba7bb7` fixed chapter numbers that came back wrong. +- 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 ordinal. Denormalization maps one code point + to another; a culture-aware comparison here is a bug. +- Add a fixture for the malformed case with the fix, in + `tests/SIL.Machine.Tests/PunctuationAnalysis/`. Every fix above was reported + from live data, not found by review. From 0324adf721e06e29927ba00b04aa16659efffaca Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 18 Sep 2026 11:48:24 -0400 Subject: [PATCH 07/10] Cut the vocabulary, comment, and adversarial docs to what is used Guidance that is not read is not guidance, and these three files carried 1,320, 885, and 211 words for what an agent needs in a glance. Together they are now 942, and the whole guidance corpus is 4,441 words rather than 5,854. CONTEXT.md keeps the words this codebase overloads and drops the tour of the interfaces. Corpus, row, segment, token, word, reference, model, engine, analysis, grammar, shape and stratum are ambiguous here and each one now gets a line and a path under src/ to read. Every anchor was checked against git ls-files. The catalogue of what each interface declares is gone: the code says that, and says it accurately. code-comments keeps the judgement the checker cannot make and stops restating the checker. The banned categories stay, because a violation message names them, and so do the 200-character budget and the 120-column width. The scope list, the flag reference, and the longer XML documentation rules are gone; the script defines the first two. devils-advocate keeps its evidence threshold and names the three claims that have actually failed review here: an unmeasured HermitCrab performance win, a USFM change tested only on the happy path, and an unchecked machine.py parity claim. Co-Authored-By: Claude Opus 5 --- .claude/skills/code-comments/SKILL.md | 146 +++++----------- CONTEXT.md | 237 ++++---------------------- docs/review/devils-advocate.md | 35 ++-- 3 files changed, 93 insertions(+), 325 deletions(-) diff --git a/.claude/skills/code-comments/SKILL.md b/.claude/skills/code-comments/SKILL.md index 485b6bdd3..e3d19d4f4 100644 --- a/.claude/skills/code-comments/SKILL.md +++ b/.claude/skills/code-comments/SKILL.md @@ -5,119 +5,59 @@ description: MUST use before writing or editing any comment in this repository - # Machine code comments -Write comments for the next reader. A comment must explain a current contract, -invariant, constraint, compatibility requirement, performance tradeoff, or -non-obvious reason. If the code and its names already make the behaviour -obvious, delete the comment. - -`scripts/comment-hygiene.ps1` enforces the mechanical parts of this standard -over the lines a branch adds. It cannot judge whether a comment is accurate or -worth keeping; that is still the author's and reviewer's job. - -## Scope - -The checker examines whole-line comments in: - -- C# under `src/` and `tests/`; -- the C/C++ wrapper under `src/sentencepiece4c/`; -- PowerShell, shell, and Python under `scripts/`, plus `local_check.sh`. - -It examines `//`, `///`, and whole-line `#` comments. It does not examine -trailing comments, `/* ... */` blocks, Python docstrings, string literals, or a -shebang line. A bare `#` in a C# file is a preprocessor directive, not a -comment. - -## Content rules - -- Be accurate before being brief. Three or four sentences is usually enough. -- Explain WHAT the code guarantees and WHY the choice matters. Do not narrate - HOW the current implementation works; the comment should survive an equivalent - rewrite. -- A member summary describes that member's own contract, not a caller's or a - collaborator's. -- A comment must stand on its own. Delete restatements of the adjacent code. -- Private comments are for non-obvious behaviour, invariants, compatibility, - performance, or a subtle bug fix. -- Keep a compatibility comment about behaviour that must stay true, for example - "Matches the legacy normalization so persisted data stays interoperable". Do - not say that code was ported, changed in a commit, or used to behave - differently. -- Use ASCII punctuation: `--` for an em dash, `->` for an arrow, `...` for an - ellipsis, `-` for a bullet or en dash, `x` for a multiplication sign, and - plain quotes. This is about typography only; comments may contain any script, - IPA, or orthography the language data requires. -- Prefer a clear word to an abbreviation the next reader cannot resolve. - -## Banned comment content - -Do not write: - -- process framing such as `Phase 1`, `later we'll`, or `we'll eventually`; -- pointers to a Markdown document, a numbered section, or a review note; -- historical narration such as `it used to`, `used to be`, `previously - returned`, `was removed`, `renamed from`, `first shipped`, or `no longer - used`; -- provenance claims such as `shared by X and Y`, `the only caller`, or - `extracted from`; -- cross-file pointers such as `see X's note` or `as documented in X`. - -A present-tense statement about current state is fine: "Returns null when the -stratum has no rules" is a contract, not history. A GitHub issue reference that -is part of the current contract may stay. +Write for the next reader. A comment earns its place by explaining a contract, +an invariant, a compatibility requirement, a performance tradeoff, or a +non-obvious reason. If the code and its names already say it, delete it. -## XML documentation +Say WHAT the code guarantees and WHY it matters, in the present tense. Do not +narrate HOW it works - the comment should survive an equivalent rewrite. A +member summary describes that member's own contract, not its caller's. + +## Banned content -The shipped library projects set `GenerateDocumentationFile` and suppress -CS1591/CS1573, so the compiler does not require documentation. This standard -supplies the contract the compiler does not enforce. +`scripts/comment-hygiene.ps1` fails on these over the lines a branch adds: -- Put one `` directly above the documented member. Give a public - constructor with parameters a summary too. -- Omit `` and `` when they only restate a name or a type. Keep - them when they add units, nullability, ownership, constraints, or real result - semantics. -- Be all-or-nothing for parameters: document every parameter, or fold the - explanation into the summary. -- Use `` for a property contract, `` for errors a caller is - expected to handle, and `` for contract-relevant symbols. -- Do not repeat the same fact in the summary, the parameters, and the returns. -- Do not add decorative file headers or section-divider comments. +- **Process framing** - `Phase 1`, `later we'll`, `we'll eventually`. +- **History** - `it used to`, `previously returned`, `was removed`, + `renamed from`, `no longer used`. +- **Provenance** - `extracted from`, `shared by X and Y`, `the only caller`. +- **Pointers** - to a Markdown file, a numbered section, a review note, or + another file's comment. +- **Non-ASCII punctuation** - use `--`, `->`, `...`, `-`, `x`, and plain quotes. + Typography only; comment text may use any script the language data needs. -## Length and width +A present-tense statement of current state is not history: "Returns null when +the stratum has no rules" is a contract. A compatibility note about behavior +that must stay true is welcome. An issue reference that is part of the current +contract may stay. -An implementation-comment block is a consecutive run of whole-line `//` or `#` -comments; a blank line, a line of code, or a doc comment ends it. **The combined -trimmed text of one block must be at most 200 characters.** This is a single -aggregate budget, not 200 characters per line, and neither the indentation nor -the `//` marker counts toward it. +## Budget and width -A `///` block and a PowerShell block comment are exempt from that budget. They -are exempt from how much they may say, not from how they are written: the -content rules and the width limit still apply. +A run of consecutive whole-line `//` or `#` comments is one block, ended by a +blank line, code, or a doc comment. **One block gets 200 characters total**, +markers and indentation excluded. `///` blocks and PowerShell block comments are +exempt from the budget, not from the content rules or the width limit. -Every comment line must fit `max_line_length` from `.editorconfig`, currently -120 display columns, counting indentation and the marker. A tab advances to the -next four-column stop. +Every comment line fits 120 display columns, per `.editorconfig`. -If a block needs more than 200 characters, the usual answer is that it belongs -in an XML summary on the member, or that it is explaining something the code -should express directly. +A block that wants more than 200 characters usually belongs in an XML summary, +or is explaining something the code should express directly. + +## XML documentation -## Tests +One `` above the member, including a public constructor with +parameters. Omit `` and `` that only restate a name or type; +keep them for units, nullability, ownership, or real result semantics. Document +every parameter or none. No file headers, no divider comments, no fact repeated +in both the summary and the parameters. -A test comment should explain a non-obvious fixture, fake, or setup constraint. -It should not restate the test name or say that the test exists for coverage. +A test comment explains a non-obvious fixture or setup constraint. It does not +restate the test name. ## Running the check -``` -pwsh ./scripts/comment-hygiene.ps1 # lines this branch adds; fails on a violation -pwsh ./scripts/comment-hygiene.ps1 -Advisory # same scan, always exits 0 -pwsh ./scripts/comment-hygiene.ps1 -Full -Advisory # size the existing debt -pwsh ./scripts/comment-hygiene.ps1 -SelfTest # verify the rules themselves -``` - -Agents must run `./local_check.sh --agent-strict`, which runs the blocking scan -before the format, build, and test steps. Do not drop the flag to get a run -through; fix the comments instead. The pull request workflow is advisory, so a -clean CI check is not evidence that the strict check passed. +`pwsh ./scripts/comment-hygiene.ps1` scans the lines your branch adds; add +`-Full -Advisory` to size existing debt, or `-SelfTest` 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 is not evidence +that the strict scan passed. diff --git a/CONTEXT.md b/CONTEXT.md index 638f3c489..3034c1e14 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -1,207 +1,40 @@ # machine shared domain context -## Scope and non-goals - -This file defines the shared language for machine's corpora, tokenization, -alignment, translation, Scripture references, and morphology code. It is a -terminology and relationship layer, not an architecture manual, an API reference, -or a release checklist. Operational rules belong in `AGENTS.md`. - -Prefer terms that a type under `src/` actually represents. If a term has no -current source or test anchor, label it external, historical, or proposed rather -than presenting it as current implementation. - -## Product scope - -machine is a natural-language-processing library, aimed in part at resource-poor -languages. The repository provides corpus abstractions, tokenizers and -detokenizers, Scripture-aware text corpora, word alignment and translation -abstractions, Thot SMT, TensorFlow SavedModel translation, and HermitCrab -morphology. `README.md` also documents the statistical methods, NuGet packages, -command-line tools, and tutorial notebooks. - -## Corpora and rows - -`ICorpus` is an enumerable corpus of rows where `T` implements `IRow` -(`src/SIL.Machine/Corpora/ICorpus.cs`). `Count` can include or exclude empty rows. - -`IText` is a corpus-backed text with an `Id` and a `SortKey` -(`src/SIL.Machine/Corpora/IText.cs`). `ITextCorpus` adds a collection of texts, an -`IsTokenized` state, Scripture versification, and row access by text id -(`src/SIL.Machine/Corpora/ITextCorpus.cs`). - -`IParallelTextCorpus` pairs a source and target side with separate tokenization -states (`src/SIL.Machine/Corpora/IParallelTextCorpus.cs`). `IAlignmentCorpus` -provides alignment rows (`src/SIL.Machine/Corpora/IAlignmentCorpus.cs`). - -`TextRow` carries a text id, a Scripture reference, a content type, flags, and a -segment held as `IReadOnlyList` (`src/SIL.Machine/Corpora/TextRow.cs`). -Its `Text` property joins the segment tokens with spaces. A row is empty when its -segment has no tokens. - -`ParallelTextRow` holds source and target segments, references, flags, and -aligned word pairs (`src/SIL.Machine/Corpora/ParallelTextRow.cs`). It is empty if -either side is empty; `Invert` swaps the sides and the alignment. -`NParallelTextRow` generalizes this to several parallel segments -(`src/SIL.Machine/Corpora/NParallelTextRow.cs`). `AlignmentRow` stores aligned -word pairs (`src/SIL.Machine/Corpora/AlignmentRow.cs`). +The words this codebase overloads, and what each one means here. Read the code +for structure; read this to avoid using a term for the wrong thing. Operational +rules live in `AGENTS.md`. + +Anchor any term you add to a current path under `src/`. If it has no anchor, +label it external, historical, or proposed. + +## The ambiguous ones + +| Term | Here it means | Anchor | +| --- | --- | --- | +| Corpus | A collection that produces rows - not a file, tokenizer, or model | `Corpora/ICorpus.cs` | +| Row | One corpus or alignment record, not a token | `Corpora/TextRow.cs` | +| Segment | A row's tokens, or the unit given to an engine; in morphology, phonological | `Corpora/TextRow.cs` | +| Token | Tokenizer output or a USFM token; not always a whitespace word | `Corpora/UsfmToken.cs` | +| Word | Say which: corpus word position, `WordAnalysis`, or HermitCrab `Word` | `Morphology/WordAnalysis.cs` | +| Reference | A Scripture or row location, never object identity | `Corpora/ScriptureRef.cs` | +| Versification | The numbering system a reference is read in | `Scripture/ScriptureRangeParser.cs` | +| Model | Learned or saved translation or alignment state | `Translation/ITranslationModel.cs` | +| Engine | The object that translates; name the backend when it matters | `Translation/ITranslationEngine.cs` | +| Trainer | The object that trains and saves model state | `Translation/ITrainer.cs` | +| Alignment | A relation between source and target positions, not a translation | `Translation/WordAlignmentMatrix.cs` | +| Analysis | Morphological decomposition, unless you name another domain | `Morphology/IMorphologicalAnalyzer.cs` | +| Synthesis | Generating surface forms from morphemes and features | `HermitCrab/Morpher.cs` | +| Grammar | The HermitCrab configuration as a whole; no `Grammar` type exists | `HermitCrab/Language.cs` | +| Stratum | One stage of the HermitCrab pipeline, not a data layer | `HermitCrab/Stratum.cs` | +| Shape | A HermitCrab phonological form, not geometry | `HermitCrab/Segments.cs` | +| Allomorph | A conditioned realization of a morpheme | `HermitCrab/Allomorph.cs` | +| SMT | Statistical machine translation, currently `ThotSmtModel` | `SIL.Machine.Translation.Thot/ThotSmtModel.cs` | + +## Two traps A corpus is not automatically a list of tokens. Its rows may be tokenized, -untokenized, empty, parallel, alignment-bearing, or Scripture-aware. Always say -which representation a method expects. - -## Tokenization and detokenization - -`ITokenizer` tokenizes whole data or a range -(`src/SIL.Machine/Tokenization/ITokenizer.cs`); `IDetokenizer` rebuilds -data from tokens (`src/SIL.Machine/Tokenization/IDetokenizer.cs`). - -`StringTokenizer` is the base for string tokenizers. `WhitespaceTokenizer` -handles whitespace, zero-width space, and byte-order-mark boundaries. -`LatinWordTokenizer` adds URL, punctuation, inner-punctuation, abbreviation, and -apostrophe rules. `StringDetokenizer` defines no-op and merge-left, merge-right, -and merge-both behaviors with a configurable separator. - -Tokenization and detokenization are not guaranteed inverses for every rule set. -Preserve the tokenizer and detokenizer pair that an engine was configured with. - -USFM is the Scripture text format the corpus code handles. `UsfmToken` has token -types for book, chapter, verse, text, paragraph, character, note, end, milestone, -attribute, and unknown, plus marker, text, data, and source position -(`src/SIL.Machine/Corpora/UsfmToken.cs`). `UsfmTag` carries text type, style, and -property information. `UsfmTokenizer` uses a stylesheet and right-to-left order -and can preserve whitespace. `UsfmFileTextCorpus` reads `.SFM` files and -`UsxFileTextCorpus` reads `.usx` files. - -## Scripture references and versification - -`ScriptureTextCorpus` and `ScriptureText` carry a `ScrVers` versification and -build rows from Scripture references and ranges. - -`ScriptureRef` is a verse reference with a nested path -(`src/SIL.Machine/Corpora/ScriptureRef.cs`). `ScriptureRangeParser` -(`src/SIL.Machine/Scripture/ScriptureRangeParser.cs`) parses -chapters, verses, and ranges, defaulting to `ScrVers.Original` unless another -versification is supplied. - -"Reference" in corpus code means a Scripture location or row location, not object -identity. "Versification" is the numbering system used to interpret references. - -## Translation engines, models, and training - -`ITranslationEngine` translates strings or token lists, synchronously and -asynchronously, supports batches, and may return n-best results -(`src/SIL.Machine/Translation/ITranslationEngine.cs`). A segment here is the unit -submitted to an engine: a string or an ordered token list, depending on the -overload. - -`ITranslationModel` extends the engine abstraction and creates a trainer from an -`IParallelTextCorpus`. `ITrainer` trains, saves, and reports statistics. - -A model is learned or saved state. An engine is the object that performs -translation. A trainer creates or updates model state. A model may expose an -engine-like interface, but model and engine are not synonyms when discussing -lifecycle or persistence. - -`ThotSmtModel` is the Thot-backed statistical model. It owns direct, inverse, and -symmetrized word alignment models and exposes tokenizer and detokenizer -configuration. Thot's documented alignment methods are IBM 1-4, HMM, and -FastAlign. `SavedModelNmtEngine` loads a TensorFlow SavedModel using its -configured signature keys and defaults to whitespace tokenization. - -HuggingFace is not a current in-repo engine or adapter; treat it as an external -or future term only. - -## Word alignment - -`IWordAligner` aligns one token pair or a batch and returns a -`WordAlignmentMatrix`. `IWordAlignmentMethod` supplies the score-selection -policy. `IWordAlignmentModel` exposes vocabularies, training, scores, and best -aligned pairs. - -`ITransductiveWordAlignmentModel` exposes the training alignment count and -retrieval of a training alignment by index. Transductive here means access to the -alignments of the training examples, not a general claim about a learning -technique. - -`WordAlignmentMatrix` is a boolean matrix whose rows are source word positions -and columns are target word positions -(`src/SIL.Machine/Translation/WordAlignmentMatrix.cs`). It supports union, -intersection, priority symmetrization, and conversion to aligned word pairs. An -aligned word pair is a relation between positions, not a dictionary entry. - -## Morphology - -The neutral API is `IMorpheme`, `WordAnalysis`, `IMorphologicalAnalyzer`, and -`IMorphologicalGenerator` under `src/SIL.Machine/Morphology/`. `IMorpheme` -describes a stem or affix. `WordAnalysis` is an ordered morpheme analysis with a -root index and category; it is a public value, not HermitCrab's internal `Word`. - -HermitCrab's integration object is `Language`, which owns strata, feature -systems, lexicon and rule configuration, and analysis and synthesis compilation -(`src/SIL.Machine.Morphology.HermitCrab/Language.cs`). "Grammar" is acceptable as -an umbrella term for the configured morphology system; there is no `Grammar.cs` -type in this tree. - -`Stratum` holds a character definition table, morphological rules, and a lexicon -for one stage of the pipeline. A stratum is a pipeline stage, not a data layer. - -`Allomorph` is a conditioned realization of a morpheme; `RootAllomorph` is the -lexical-root specialization. `Segments` stores a representation through a -`CharacterDefinitionTable` and can expose a frozen `Shape`. Shape here is a -phonological form, not geometry. HermitCrab `Word` is internal morphology state -holding allomorphs, root, shape, rules, features, range, and stratum. -`Morpher.AnalyzeWord` produces `WordAnalysis`; `Morpher.GenerateWords` produces -surface forms. - -Analysis means decomposing a word into morphemes and features. Synthesis means -generating surface forms from morphemes and features. Do not say "analysis" for a -word alignment without naming the domain. - -## Architecture language - -A source project is a `.csproj` under `src/`, not every namespace or directory. A -test project is a current `.csproj` under `tests/` with a solution or CI entry; a -`bin` or `obj` directory is not evidence of a project. - -The `netstandard2.0` libraries are the reusable package boundary. `net10.0` -projects host tools, plugin behavior, and tests. - -SentencePiece4c is a native build and runtime boundary beneath the managed -SentencePiece project. It is the only such boundary; most of the repository is -platform-independent managed code. - -machine.py is a sibling repository coordinated through post-merge porting issues. -Serval is an external consumer. Neither is a verified in-repo dependency. - -## Disambiguation rules - -- **Corpus**: a row-producing collection; not a file, a tokenizer, or a model. -- **Row**: one corpus or alignment record; not a token. -- **Segment**: an ordered token sequence for a row, or the unit submitted to a - translation engine. In morphology, say phonological segment. -- **Token**: tokenizer output or a structured USFM token; not necessarily a - whitespace-delimited word. -- **Word**: qualify as corpus word position, `WordAnalysis`, or HermitCrab - `Word`. These are three different abstractions. -- **Model**: learned or saved translation or alignment state. -- **Engine**: the object that performs translation; name the backend when it - matters. -- **Trainer**: the object that trains and saves model state. -- **Alignment**: a relation between source and target positions; not a - translation. -- **Analysis**: morphological decomposition, unless another domain is named. -- **Grammar**: the HermitCrab configuration as a whole. Prefer `Language`, - `Stratum`, rules, lexicon, and features in code discussion. -- **Shape**: a HermitCrab phonological form. -- **Stratum**: one HermitCrab pipeline stage. -- **Reference**: a `ScriptureRef` or row location in corpus context. -- **Versification**: the Scripture numbering system. -- **SMT**: statistical machine translation, currently `ThotSmtModel`. - -## Maintenance +untokenized, empty, parallel, alignment-bearing, or Scripture-aware; say which +representation a method expects. -When you add a term, anchor it to a current source or test path. When a type or -workflow changes, update the smallest relevant section. Do not copy command lists -from `AGENTS.md` into this file. Record uncertainty rather than promoting an -unverified relationship into architecture. +Tokenization and detokenization are not guaranteed inverses. Keep the tokenizer +and detokenizer pair an engine was configured with. diff --git a/docs/review/devils-advocate.md b/docs/review/devils-advocate.md index 28661d258..18ec5bf67 100644 --- a/docs/review/devils-advocate.md +++ b/docs/review/devils-advocate.md @@ -1,26 +1,21 @@ # Machine Devil's Advocate -*Read-only adversarial second pass for machine PR reviews, focused on compatibility, -determinism, parity, memoization safety, and evidence gaps.* +*Optional adversarial second pass for a high-risk change.* -Act as a skeptical senior reviewer after the normal machine PR review. Read the -merge-base diff, the normal review output, relevant tests, project files, and cited -history. Do not edit files, commit, push, or propose an unbounded rewrite. +Read the merge-base diff, the normal review output, and the tests. Change +nothing. -Challenge the most consequential conclusion first. Ask one objection at a time and -support it with `path:line`, a concrete execution scenario, a consumer/fixture contract, -or a missing command/artifact. Focus on: +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. -- public API/source/binary compatibility and target/package changes; -- false nullable promises or inconsistent interface implementations; -- cancellation, ordering, disposal, pooling, and parallel/sequential divergence; -- culture-sensitive token/marker/USFM behavior and Unicode regressions; -- incomplete HermitCrab keys, unsafe replay, retained-memory bounds, or unsupported performance claims; -- Python-port parity claims without a checked comparison; and -- tests that pass while leaving changed decisions or failure paths unverified. +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: -Do not call a concern a finding unless the evidence is present. Label each item -`Verified objection` or `Unverified question`. Do not supply the solution in the -objection section; state the evidence needed to settle it. Finish with `Top risk`, -`Evidence still needed`, and `Would this block merge?` with a reason tied to the normal -review severity contract. +- a HermitCrab performance win asserted without a measured artifact, or a memo + key that omits a field a rule reads; +- a USFM or reference change whose test proves the happy path only; +- a parity claim about `machine.py` with no checked comparison. + +Close with `Top risk`, `Evidence still needed`, and `Would this block merge?`. From b06742b5e2a486e1727a4c0dd5c80d42b8f85745 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 18 Sep 2026 11:54:38 -0400 Subject: [PATCH 08/10] Keep only what an agent cannot read off the tree An agent that wants the target frameworks runs grep; an agent that reads them here gets whatever was true when someone last edited this file. So the guidance now carries rules and traps, and nothing an ls or a cat would answer. The corpus is 3,609 words, down from 5,854 at the start of this branch. AGENTS.md drops the repository layout, the target framework lists, the local_check.sh command block, the CSharpier settings, the ci.yml job description, the SentencePiece section, and the porting workflow's mechanics. It keeps the four places the tree misleads a reader: ci.yml triggers on push so a green check is not a pull request gate, the comment hygiene check is advisory, appveyor.yml is legacy and must not be repaired, and a directory is not a project without a project file. CONTEXT.md is deleted. Naming what each interface declares duplicated the code, which says it more accurately. The part that earned its place was the handful of words where research misleads - three different things called Word, a grammar with no Grammar type, a shape that is not geometry - and those are now four lines in AGENTS.md. hermitcrab.md no longer quotes the memo bounds, which live in the code; the rule is not to weaken one without evidence. machine-library.md says netstandard2.0 once, as the compatibility constraint it is. commit-messages.md keeps the two traps, the squash suffix and the historical Jira identifiers, and drops the whitespace command. Co-Authored-By: Claude Opus 5 --- .claude/skills/commit-messages/SKILL.md | 39 ++--- AGENTS.md | 184 ++++++++---------------- CLAUDE.md | 7 +- CONTEXT.md | 40 ------ docs/review/hermitcrab.md | 5 +- docs/review/machine-library.md | 7 +- 6 files changed, 82 insertions(+), 200 deletions(-) delete mode 100644 CONTEXT.md diff --git a/.claude/skills/commit-messages/SKILL.md b/.claude/skills/commit-messages/SKILL.md index 373cbe1a3..947abe48b 100644 --- a/.claude/skills/commit-messages/SKILL.md +++ b/.claude/skills/commit-messages/SKILL.md @@ -5,37 +5,22 @@ description: How to write a commit message in sillsdev/machine - imperative subj # Commit messages -Write a concise, imperative, sentence-case subject that names the actual change. -Recent history is descriptive and GitHub-native, for example: +A concise, imperative, sentence-case subject naming the actual change, under +about 72 characters, with no terminal punctuation: - `Fix bug in MergeEquivalentAnalyses (#493)` -- `Use StringComparison.Ordinal when locating token indices in PlaceMarkersUsfmUpdateBlockHandler (#496)` - `Port changes from sillsdev/machine.py#336 (#498)` -## Conventions +If there is a body, leave a blank line after the subject, wrap at about 80 +columns, and say what changed and why. Reference a GitHub issue when one exists. -- Keep the subject under about 72 characters when you can. This is not enforced - by CI, and existing history contains longer subjects, so do not rewrite shared - history to satisfy it. -- No trailing period or other terminal punctuation on the subject. -- No leading, trailing, or interior tab and trailing-whitespace damage. -- If a body is present, leave one blank line after the subject and wrap body - lines at about 80 characters. -- Explain what changed and why. Reference a GitHub issue when one exists. -- The `(#N)` suffix is added by GitHub when a pull request is squashed. Do not - add it by hand to an ordinary local commit. -- Older commits use Jira identifiers such as `LT-22605`. That convention is - historical; use a GitHub issue reference for new work unless a maintainer asks - otherwise. +Two things the history will mislead you about: -## Check the range before pushing +- The `(#N)` suffix is added by GitHub when a pull request is squashed. Never + type it into a local commit. +- Older commits carry Jira identifiers such as `LT-22605`. That is historical; + use a GitHub issue reference for new work. -``` -git fetch origin --quiet -git log --check --pretty=format:'--- %h %s' origin/master..HEAD -``` - -`git log --check` reports whitespace damage in the commits you are about to -push. A failed check is not a pass. Do not rewrite a pushed or shared branch to -fix a message; add a corrective commit unless the author explicitly authorizes -the rewrite. +The 72-character limit is not enforced, and longer subjects 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/AGENTS.md b/AGENTS.md index e2341b9df..6e2e7194d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,140 +1,80 @@ # machine contributor and agent guide -Repository-wide operational guidance for contributors and coding agents. Keep it -short and factual. Shared domain vocabulary lives in `CONTEXT.md`; do not -duplicate that glossary here. - -Check the current tree before relying on a rule. If this file disagrees with -`local_check.sh`, `.github/workflows/ci.yml`, a project file, or the code you are -changing, say so and prefer the current executable behavior until this file is -updated. - -## Repository shape - -- `Machine.sln` contains the source and test projects. -- Production code is under `src/`; tests are under `tests/`. -- Native SentencePiece sources are under `src/sentencepiece4c/`. -- Samples and notebooks are under `samples/`; repository scripts under `scripts/`. -- The published libraries target `netstandard2.0`: `SIL.Machine`, - `SIL.Machine.Translation.Thot`, `SIL.Machine.Translation.TensorFlow`, - `SIL.Machine.Morphology.HermitCrab`, and `SIL.Machine.Tokenization.SentencePiece`. -- `SIL.Machine.Tool`, `SIL.Machine.Morphology.HermitCrab.Tool`, - `SIL.Machine.Plugin`, and the test projects target `net10.0`. -- Shared assembly and package metadata comes from `src/AssemblyInfo.props`. - There is no `Directory.Build.props` in this repository. -- A directory under `src/` or `tests/` is not an active project unless a current - project file, solution entry, or CI step references it. +What an agent must do here, and where looking at the tree will mislead you. +Everything else - layout, target frameworks, what a workflow runs - read from +the tree; it is accurate and this file would only rot. -## Local validation +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. -`local_check.sh` is the canonical sequence: +## Validation -``` -dotnet tool restore -dotnet restore -dotnet csharpier check . -dotnet build --no-restore -c Release -dotnet test --verbosity normal -``` +Run `./local_check.sh` from the repository root: it restores, checks CSharpier +formatting, builds Release, and tests. Do not skip a failing step or report +success without fresh output. -Run it from the repository root. Do not skip a failing format, build, or test -step, and do not report success without fresh output. +Agents must also run `./local_check.sh --agent-strict`, which makes +`scripts/comment-hygiene.ps1` blocking over the lines the branch adds. The +standard it enforces is `.claude/skills/code-comments/SKILL.md`. Do not drop the +flag to get a run through. -CSharpier is required for C#. The tool is pinned in `.config/dotnet-tools.json`, -`.csharpierrc.yaml` sets `printWidth: 120`, and `.editorconfig` sets -`max_line_length = 120` plus the repository's analyzer severities. -`.csharpierignore` excludes config, project, props, targets, and XML files. +CI collects coverage and `local_check.sh` does not, so a local run is never +coverage-equivalent. -CI also collects coverage (`--collect:"Xplat Code Coverage"`); `local_check.sh` -does not, so do not describe a local run as coverage-equivalent. +## Where the tree misleads -## Branch hygiene +- `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. +- `appveyor.yml` is legacy and names projects that do not exist. Ignore it; do + not repair it. +- A directory under `src/` or `tests/` is not an active project unless a current + project file, solution entry, or CI step references it. +- Three different things are called `Word` here: a corpus word position, + `WordAnalysis` (`src/SIL.Machine/Morphology/WordAnalysis.cs`), and HermitCrab's + internal `Word`. Say which. Likewise "grammar" is the HermitCrab configuration + as a whole and has no `Grammar` type - prefer `Language` or `Stratum`; "shape" + is a phonological form, not geometry; "analysis" is morphological decomposition + unless you name another domain; and a "reference" is a Scripture or row + location, never object identity. + +## Changing code + +- Add or update focused tests with every behavior change. Keep fixtures + deterministic and platform assumptions explicit. +- Use ordinal comparison for markers, tokens, identifiers, and protocol text; + culture-sensitive only where the operation is genuinely linguistic. This is the + defect class that ships here most often. +- Dispose engines, models, trainers, and streams according to their contracts, + and be explicit about who owns a stream that is passed in. +- Preserve public API semantics unless the change intends otherwise and updates + the tests. The published libraries are consumed as `netstandard2.0`. +- Prefer an existing abstraction to a parallel one. -Before opening or updating a pull request, confirm the branch and the range: -`git status --short --branch`, `git merge-base origin/master HEAD`, -`git diff --check ...HEAD`, and `git log --check origin/master..`. -The target default is `origin/master`; if the remote default changes, use the -resolved default and say so. +## Branch hygiene Preserve unrelated changes and pre-existing untracked files. Never use `reset --hard`, `checkout --`, broad deletion, or broad staging as a cleanup -shortcut. A local review summary may be written to `.review/`, which -`.gitignore` excludes; keep other transient notes outside the repository. - -## Comment hygiene - -Agents must run `./local_check.sh --agent-strict`. It adds -`scripts/comment-hygiene.ps1` to the sequence above and fails the run on any -comment violation in the lines the branch adds, so you fix your own comments -before they reach review. This flag is required of agents and optional for -humans. Do not drop it to get a run through. +shortcut. Local review notes go in `.review/`, which `.gitignore` excludes. -The standard is `.claude/skills/code-comments/SKILL.md`: a 200-character -aggregate budget for a block of implementation comments, the 120-column -`.editorconfig` width for every comment line, ASCII punctuation, and no process -framing, document pointers, historical narration, or provenance claims. +When a change ports work from `sillsdev/machine.py`, link the source pull +request. A workflow files the porting issue after merge; do not hand-file a +duplicate. -The `Comment hygiene` pull request workflow runs the same scan in advisory mode. -It annotates and never fails, so a green check there is not evidence that the -strict check passed. +## Agent guidance -## Native SentencePiece boundary +`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: -CI builds `src/sentencepiece4c` separately and feeds the platform-specific -artifact to the managed build and package job. If you change it, copy the -current CMake commands from `.github/workflows/ci.yml` rather than from here. +| Path glob | Rules file | +| --- | --- | +| `src/SIL.Machine/Corpora/**/*.cs` | `docs/review/corpora-usfm.md` | +| `src/SIL.Machine/PunctuationAnalysis/**/*.cs` | `docs/review/punctuation.md` | +| `src/SIL.Machine.Morphology.HermitCrab/**/*.cs` | `docs/review/hermitcrab.md` | +| any other `src/**/*.cs` | `docs/review/machine-library.md` | +| `tests/**/*.cs` | `docs/review/machine-tests.md` | -## Tests and changes - -- Add or update focused tests with every behavior change. -- Keep test data deterministic and make platform assumptions explicit. -- When a public API or package boundary changes, consider both `netstandard2.0` - consumer compatibility and `net10.0` tool and test behavior. -- Dispose engines, models, trainers, and streams according to their contracts. -- Preserve public API semantics unless the change intends otherwise and updates - tests and documentation. -- Use string comparisons that match the domain: ordinal for markers, tokens, and - other protocol identity; culture-sensitive only where the operation is genuinely - linguistic. -- Prefer existing abstractions over parallel ones. Keep terminology consistent - with `CONTEXT.md`. - -## CI and legacy CI - -`.github/workflows/ci.yml` is the current reference: Ubuntu and Windows, .NET 10, -native SentencePiece build, CSharpier check, Release build, tests with coverage, -and tag-triggered NuGet publishing. It triggers on `push`, not on `pull_request`, -so a green check on a PR reflects the pushed head rather than a PR event. - -`appveyor.yml` is legacy and describes projects that are not in the tree. -Ignore it. - -## Porting to and from machine.py - -`.github/workflows/create-porting-issue.yml` files a porting issue in the sibling -repository when a pull request merges: `machine.py` when running in `machine`, and -the reverse in `machine.py`. It labels the issue `porting` and marks the body -`AUTO-GENERATED-ISSUE`. This is issue-level coordination, not a runtime -dependency. When a change ports work from the sibling repository, link the source -pull request. - -## Working with agent guidance - -- This file is the shared operational source of truth. `CLAUDE.md` imports it. -- Claude-specific workflows live under `.claude/skills/`. -- Path-scoped review rules live under `docs/review/`. Match the changed path to - find the rules file: - - | Path glob | Rules file | - | --- | --- | - | `src/SIL.Machine/Corpora/**/*.cs` | `docs/review/corpora-usfm.md` | - | `src/SIL.Machine/PunctuationAnalysis/**/*.cs` | `docs/review/punctuation.md` | - | `src/SIL.Machine.Morphology.HermitCrab/**/*.cs` | `docs/review/hermitcrab.md` | - | any other `src/**/*.cs` | `docs/review/machine-library.md` | - | `tests/**/*.cs` | `docs/review/machine-tests.md` | - - The first matching row wins. - - For a high-risk change, `docs/review/devils-advocate.md` is an optional - adversarial second pass. -- Add a nested `AGENTS.md` only when a subtree genuinely needs different rules. +`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 e7278b2bb..ed5df484d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,7 +2,6 @@ ## Claude Code -- Keep repository-wide standing guidance in `AGENTS.md` and import it here. -- Put Claude-only workflows and task procedures under `.claude/skills/`. -- Keep `.github/` for GitHub-required files: workflows and issue and pull - request templates. Path-scoped review rules live under `docs/review/`. +- 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/CONTEXT.md b/CONTEXT.md deleted file mode 100644 index 3034c1e14..000000000 --- a/CONTEXT.md +++ /dev/null @@ -1,40 +0,0 @@ -# machine shared domain context - -The words this codebase overloads, and what each one means here. Read the code -for structure; read this to avoid using a term for the wrong thing. Operational -rules live in `AGENTS.md`. - -Anchor any term you add to a current path under `src/`. If it has no anchor, -label it external, historical, or proposed. - -## The ambiguous ones - -| Term | Here it means | Anchor | -| --- | --- | --- | -| Corpus | A collection that produces rows - not a file, tokenizer, or model | `Corpora/ICorpus.cs` | -| Row | One corpus or alignment record, not a token | `Corpora/TextRow.cs` | -| Segment | A row's tokens, or the unit given to an engine; in morphology, phonological | `Corpora/TextRow.cs` | -| Token | Tokenizer output or a USFM token; not always a whitespace word | `Corpora/UsfmToken.cs` | -| Word | Say which: corpus word position, `WordAnalysis`, or HermitCrab `Word` | `Morphology/WordAnalysis.cs` | -| Reference | A Scripture or row location, never object identity | `Corpora/ScriptureRef.cs` | -| Versification | The numbering system a reference is read in | `Scripture/ScriptureRangeParser.cs` | -| Model | Learned or saved translation or alignment state | `Translation/ITranslationModel.cs` | -| Engine | The object that translates; name the backend when it matters | `Translation/ITranslationEngine.cs` | -| Trainer | The object that trains and saves model state | `Translation/ITrainer.cs` | -| Alignment | A relation between source and target positions, not a translation | `Translation/WordAlignmentMatrix.cs` | -| Analysis | Morphological decomposition, unless you name another domain | `Morphology/IMorphologicalAnalyzer.cs` | -| Synthesis | Generating surface forms from morphemes and features | `HermitCrab/Morpher.cs` | -| Grammar | The HermitCrab configuration as a whole; no `Grammar` type exists | `HermitCrab/Language.cs` | -| Stratum | One stage of the HermitCrab pipeline, not a data layer | `HermitCrab/Stratum.cs` | -| Shape | A HermitCrab phonological form, not geometry | `HermitCrab/Segments.cs` | -| Allomorph | A conditioned realization of a morpheme | `HermitCrab/Allomorph.cs` | -| SMT | Statistical machine translation, currently `ThotSmtModel` | `SIL.Machine.Translation.Thot/ThotSmtModel.cs` | - -## Two traps - -A corpus is not automatically a list of tokens. Its rows may be tokenized, -untokenized, empty, parallel, alignment-bearing, or Scripture-aware; say which -representation a method expects. - -Tokenization and detokenization are not guaranteed inverses. Keep the tokenizer -and detokenizer pair an engine was configured with. diff --git a/docs/review/hermitcrab.md b/docs/review/hermitcrab.md index 02ec7ee86..da050372b 100644 --- a/docs/review/hermitcrab.md +++ b/docs/review/hermitcrab.md @@ -13,9 +13,8 @@ Governs `src/SIL.Machine.Morphology.HermitCrab/**/*.cs`. - Memoized results must represent fully expanded subtrees. Check replay prefixes, deduplication, empty/nogood entries, in-flight recursion, and the separation between sequential and parallel scopes. -- Preserve the per-parse scope rule and inspect the - 100,000-entry/1,000,000-retained-word backstops when changing storage or result lists. - Do not weaken a bound without measured evidence and tests. +- Do not weaken an existing memo or retained-word bound without measured evidence + and a test. Read the current limits from the code. - Inspect allocations and retained object lifetimes only in changed inner loops. If the change claims a performance improvement, require a reproducible benchmark or measured artifact in addition to semantic regression tests. diff --git a/docs/review/machine-library.md b/docs/review/machine-library.md index 8c69b7b5c..8b22beaef 100644 --- a/docs/review/machine-library.md +++ b/docs/review/machine-library.md @@ -5,10 +5,9 @@ contracts, and disposal.* Governs any `src/**/*.cs` no more specific rules file claims. -- Treat this as shipped library code. Check public and protected API shape, - overloads, optional parameters, return types, and XML documentation when they - change. The target is `netstandard2.0`; do not introduce an API that silently - drops existing consumers. +- This is shipped library code, consumed as `netstandard2.0`. Check public and + protected API shape, overloads, optional parameters, and return types when they + change; do not introduce an API that silently drops existing consumers. - Use ordinal comparison for markers, tokens, identifiers, and protocol text. Reserve culture-sensitive comparison for genuinely linguistic operations. This is the defect class review misses most often here: `075c6ea1`, `dac2d895`, and From c498f79cd68d4095af219e0c9dd3285748499090 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 18 Sep 2026 11:58:49 -0400 Subject: [PATCH 09/10] Shape the skills for a reader who skims Each skill opened by explaining itself. A reader who stops after the first line now has the action instead: write the body to a file and edit the PR with it, post one comment per finding, search for duplicates first, delete the comment before writing it. The bodies are numbered steps rather than named topics, because the work is sequential and a numbered list tells a skimmer where they are. No group runs past five items. Every hedge, every recap of the section above, and every sentence describing what the document is were cut. The rules are unchanged. This is the same guidance in a shape that survives being skimmed, which is how it will be read. Skills are 1,833 words, down from 2,530 when the branch opened, and the guidance corpus is 3,447. Co-Authored-By: Claude Opus 5 --- .claude/skills/code-comments/SKILL.md | 62 ++++++++++----------- .claude/skills/commit-messages/SKILL.md | 24 ++++---- .claude/skills/issue-authoring/SKILL.md | 64 +++++++++------------- .claude/skills/pr-authoring/SKILL.md | 73 ++++++++++++------------- .claude/skills/pr-review/SKILL.md | 72 ++++++++++++------------ 5 files changed, 138 insertions(+), 157 deletions(-) diff --git a/.claude/skills/code-comments/SKILL.md b/.claude/skills/code-comments/SKILL.md index e3d19d4f4..ba32d0f9e 100644 --- a/.claude/skills/code-comments/SKILL.md +++ b/.claude/skills/code-comments/SKILL.md @@ -5,43 +5,42 @@ description: MUST use before writing or editing any comment in this repository - # Machine code comments -Write for the next reader. A comment earns its place by explaining a contract, -an invariant, a compatibility requirement, a performance tradeoff, or a -non-obvious reason. If the code and its names already say it, delete it. +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 it matters, in the present tense. Do not -narrate HOW it works - the comment should survive an equivalent rewrite. A -member summary describes that member's own contract, not its caller's. +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 member summary +describes that member's own contract, not its caller's. -## Banned content +## Never write these -`scripts/comment-hygiene.ps1` fails on these over the lines a branch adds: +`scripts/comment-hygiene.ps1` fails on them over the lines your branch adds: -- **Process framing** - `Phase 1`, `later we'll`, `we'll eventually`. -- **History** - `it used to`, `previously returned`, `was removed`, - `renamed from`, `no longer used`. -- **Provenance** - `extracted from`, `shared by X and Y`, `the only caller`. -- **Pointers** - to a Markdown file, a numbered section, a review note, or - another file's comment. -- **Non-ASCII punctuation** - use `--`, `->`, `...`, `-`, `x`, and plain quotes. - Typography only; comment text may use any script the language data needs. +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. -A present-tense statement of current state is not history: "Returns null when -the stratum has no rules" is a contract. A compatibility note about behavior -that must stay true is welcome. An issue reference that is part of the current -contract may stay. +Present tense about current state is not history: "Returns null when the stratum +has no rules" 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. -## Budget and width +## Fit the budget -A run of consecutive whole-line `//` or `#` comments is one block, ended by a -blank line, code, or a doc comment. **One block gets 200 characters total**, -markers and indentation excluded. `///` blocks and PowerShell block comments are -exempt from the budget, not from the content rules or the width limit. +One block - a run of whole-line `//` or `#` comments, ended by a blank line, +code, or a doc comment - gets **200 characters total**, markers and indentation +excluded. Every line fits **120 display columns**. -Every comment line fits 120 display columns, per `.editorconfig`. +`///` blocks and PowerShell block comments are exempt from the budget, not from +the content rules or the width limit. -A block that wants more than 200 characters usually belongs in an XML summary, -or is explaining something the code should express directly. +Over budget? It belongs in an XML summary, or the code should express it +directly. ## XML documentation @@ -54,10 +53,9 @@ in both the summary and the parameters. A test comment explains a non-obvious fixture or setup constraint. It does not restate the test name. -## Running the check +## Run the check -`pwsh ./scripts/comment-hygiene.ps1` scans the lines your branch adds; add +`pwsh ./scripts/comment-hygiene.ps1` scans the lines your branch adds. Add `-Full -Advisory` to size existing debt, or `-SelfTest` 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 is not evidence -that the strict scan passed. +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 947abe48b..ea25a4d1a 100644 --- a/.claude/skills/commit-messages/SKILL.md +++ b/.claude/skills/commit-messages/SKILL.md @@ -5,22 +5,22 @@ description: How to write a commit message in sillsdev/machine - imperative subj # Commit messages -A concise, imperative, sentence-case subject naming the actual change, under -about 72 characters, with no terminal punctuation: +Name the change in an imperative, sentence-case subject under about 72 +characters, with no terminal punctuation: - `Fix bug in MergeEquivalentAnalyses (#493)` - `Port changes from sillsdev/machine.py#336 (#498)` -If there is a body, leave a blank line after the subject, wrap at about 80 -columns, and say what changed and why. Reference a GitHub issue when one exists. +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 things the history will mislead you about: +## Two traps in the history -- The `(#N)` suffix is added by GitHub when a pull request is squashed. Never - type it into a local commit. -- Older commits carry Jira identifiers such as `LT-22605`. That is historical; - use a GitHub issue reference for new work. +1. The `(#N)` suffix is added by GitHub when a pull request is squashed. Never + type it into a local commit. +2. Older commits carry Jira identifiers such as `LT-22605`. That is historical; + use a GitHub issue reference. -The 72-character limit is not enforced, and longer subjects 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. +The 72-character limit is not enforced and longer subjects 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 index 6e0d6f956..dd38791ed 100644 --- a/.claude/skills/issue-authoring/SKILL.md +++ b/.claude/skills/issue-authoring/SKILL.md @@ -7,62 +7,52 @@ user-invocable: true # Writing a machine issue -A style guide for the issue text. GitHub issues are the native tracker here; -this repository has no Jira workflow. An `LT-` reference is an external link -only, and only when someone supplied it. +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. -Search open and recently closed issues first. Say what you searched. +GitHub issues are the tracker here. An `LT-` reference is an external link, and +only when someone supplied it. -## The title +## 1. Title: one symptom -One symptom, in the reader's words, under about 70 characters. No "investigate", -no "improve", no component prefix the labels already carry. +Under about 70 characters, in the reader's words. No "investigate", no +"improve", no component prefix the labels already carry. Bad: *Tokenizer improvements* Good: *USFM attribute is dropped when the locale is tr-TR* -## The lede +## 2. Lede: three sentences -Three short sentences, each doing a different job. Nothing else before them. +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. -1. **The symptom.** What goes wrong, in the reader's terms. -2. **The trigger.** The smallest condition that produces it - input, locale, - platform, version. -3. **The cost.** Who is blocked, what is lost, 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: *A USFM attribute is dropped when the tokenizer runs under tr-TR. Any marker containing an ASCII `i` splits at the wrong index on a Turkish locale. Round-tripping a Turkish project silently loses the attribute.* -For a feature, the same three: what is missing, when it bites, what it costs. +## 3. Body: labelled lines, never a wall -Keep each sentence under about 25 words. Everything longer goes in the body. +Write `Unknown` where you do not know, and say how to find out. -## The body +- **Bug** - affected API; version, OS, runtime; the smallest input that shows + it; expected vs actual; sanitized log; 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. -Short paragraphs or bullets, never a wall. Everything is a labelled line someone -can scan. Write `Unknown` where you do not know, and say how to find out. +Sanitize first: no secrets, tokens, customer text, or private project data. -**Bug** - affected package or API; version or commit, OS, runtime; the smallest -input that shows it; expected vs actual; sanitized log or exception; when it -started, or `Unknown`; the test that would catch it. +## 4. Check it is ready -**Feature** - who is blocked and by what; the behavior proposed, with its -compatibility cost; acceptance criteria an outsider could check; non-goals. +Ready means another maintainer can reproduce the bug, judge the acceptance +criteria, or find the change to port - without asking you a question. -**Porting** - the source PR or commit URL, what behavior matters here, what does -not, and the target projects if known. - -Sanitize before posting: no secrets, tokens, customer text, or private project -data. - -## Ready - -An issue is ready when another maintainer can reproduce the bug, judge the -acceptance criteria, or find the exact change to port - without asking you a -question first. - -`create-porting-issue.yml` already files the porting issue after a merge, marked -`AUTO-GENERATED-ISSUE`. Do not write a second one by hand. +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 8f05e0e21..53160bcc4 100644 --- a/.claude/skills/pr-authoring/SKILL.md +++ b/.claude/skills/pr-authoring/SKILL.md @@ -7,21 +7,22 @@ user-invocable: true # Writing a machine PR -A style guide for the PR text. The work itself - what to check, what to run - is -in `AGENTS.md` and `docs/review/`. +Write the body to a file, then `gh pr edit --body-file`. Do not push, open, +or edit a PR unless the author asked. -Do not push, open, or edit a PR unless the author asked for it. +What to check and what to run is in `AGENTS.md` and `docs/review/`. This is the +write-up. -## The lede +## 1. Write the lede -Three short sentences, each doing a different job. Nothing else before them. +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. - Not what you did, not how long it took, not which files moved. -2. **The reviewer's first unknown, answered.** Usually "what breaks?" or "why is - it this big?" Answer it here; do not make them read for it. -3. **The boundary.** What the change does not touch, or the one condition that - keeps it safe. +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 tokenizer and adds some tests.* @@ -29,20 +30,16 @@ Good: *USFM markers now split identically under tr-TR, where the attribute used to be dropped. No public signature changes - the fix is one comparison, from culture-aware to ordinal. Nothing outside `UsfmTokenizer` is touched.* -If the change is invisible to callers, lead with what it protects: *Agents can -no longer land a comment that narrates its own history.* - -Keep each sentence under about 25 words. If a sentence needs a subordinate -clause to survive, it belongs in the body. +Invisible to callers? Lead with what it protects: *Agents can no longer land a +comment that narrates its own history.* -## Body +## 2. Fill the top zone -Keep the top zone under 200 words. Sections, in order, and drop any that are -empty: +Under 200 words. Drop any section that would be empty. ```markdown ## Quick summary - + ## Where to look - -- @@ -57,30 +54,30 @@ empty: ``` -Everything else goes below 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. It is not welcome above the -rule. +## 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 of the section above. +No preamble, no apology, no "should be fine", no recap. -## Validation lines +## 4. Check the claims -Write the command and its result, nothing else. Never list a command you did not -run, and never call a local run CI-equivalent - CI collects coverage and -`local_check.sh` does not. If a check was skipped, say which and why. +Every count, path, type, and test name must match the tree. A wrong number in a +PR body outlives the PR. -Verify every count, path, type, and test name against the tree before it goes in -the body. 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 `local_check.sh` does not. Name any check you skipped. ## Replying to review comments -Reply in the thread, on the line. Classify first, then act: +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. +- **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. -One reply per comment, two or three sentences. Resolve only a thread that is -fully answered and that you did not dispute. +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 index b5c326466..3701b384c 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -7,53 +7,52 @@ user-invocable: true # Writing a machine review -This is a style guide for the review you publish, not a method for doing the -review. What to look for lives in `docs/review/`; `AGENTS.md` maps a changed -path to its rules file. +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. -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. -## Shape +## 1. One finding, one comment -**Many short comments, not one long one.** Anchor each finding on the line it is -about. A reviewer scrolling the diff should meet each point where it applies. +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. -**One finding per comment.** Two problems on one line are two comments. +## 2. Lead with the claim -**Lead with the claim.** First sentence names the defect. Evidence second, fix -third, and only if it fits. +First sentence names the defect. Evidence second, fix third, if it fits. ``` Ordinal comparison missing: `marker.IndexOf(":")` is culture-sensitive, so tr-TR splits this marker differently. Pass `StringComparison.Ordinal`. ``` -Three lines is a long comment. If one needs more, the finding is really a -design question - ask it in the summary instead. +Three lines is long. A finding needing more is a design question - raise it in +the summary instead. -## Severity +## 3. Label the severity -Prefix each comment: **Critical** (blocks merge), **Important**, or **Minor**. -Critical means demonstrated - a failing command, a broken contract, a missing -gate. A worry is not Critical. +**Critical** blocks merge, then **Important**, then **Minor**. Critical means +demonstrated: a failing command, a broken contract, a missing gate. A worry is +not Critical. -## Evidence +## 4. Carry the evidence -Every comment carries `path:line` and a consequence. Mark anything you did not -confirm as `Unverified`, and never let an unverified concern block a merge. +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, a modernization, or a benchmark suite the diff gave no reason for. A -search that found nothing proves absence only if you state what you searched. +- 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. +- A coverage percentage is not evidence that a changed line is tested. -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 the -advisory `Comment hygiene` check going green does not stand in for it. A -coverage percentage is not evidence that a changed line is tested. - -## Summary comment - -Five lines at most: +## 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`. @@ -61,14 +60,11 @@ Five lines at most: 4. What you ran, and its result. 5. What you could not verify. -State `None verified` where that is the honest answer. Say which public API, -target framework, package, or parity contract changed, or that none did. - -## Closing a thread +Say which public API, target framework, package, or parity contract changed, or +`None verified`. -Say what happened to each finding: **changed**, **accepted** (the author -answered and you agree), or **unverified** (nobody settled it). Of 140 review -threads in this repository's last three years, 128 have no author follow-up at -all, so the reader cannot tell which findings mattered. Leave nothing implicit. +Then mark each finding **changed**, **accepted**, or **unverified**. Of 140 +review threads here in three years, 128 have no follow-up, so nobody can tell +which findings mattered. Leave nothing implicit. For an adversarial second pass, apply `docs/review/devils-advocate.md`. From 907c8b90c4fc3326ac60346b2829ce419bee5b56 Mon Sep 17 00:00:00 2001 From: Damien Daspit Date: Fri, 18 Sep 2026 17:23:29 -0400 Subject: [PATCH 10/10] Prefer no XML documentation and correct misleading guidance State the preference against XML documentation rather than requiring a summary on every member, including constructors. `SIL.Machine.Morphology.HermitCrab`, `SIL.Machine.Tokenization.SentencePiece`, and `SIL.Machine.Translation.TensorFlow` do not set `GenerateDocumentationFile` at all, so a `///` block in those three ships nothing to a consumer. "Over budget? It belongs in an XML summary" pointed the other way and, since `///` is exempt from the budget, made laundering a long comment into a doc comment the cheapest way to comply. Shorten it instead. Record that the over-budget comments already in the tree are known debt rather than a convention to copy or an errand to sweep, and that a comment should match the placement, density, and idiom of the file it lands in. Also note that the devil's advocate pass prunes the first review rather than sweeping again, that `PULL_REQUEST_TEMPLATE.md` wins if it drifts from the skill that mirrors it, and drop an unsupported timing claim from the workflow. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/skills/code-comments/SKILL.md | 28 ++++++++++++++++++++------- .claude/skills/pr-authoring/SKILL.md | 3 +++ .github/workflows/comment-hygiene.yml | 4 ++-- docs/review/devils-advocate.md | 6 +++++- 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/.claude/skills/code-comments/SKILL.md b/.claude/skills/code-comments/SKILL.md index ba32d0f9e..9bc0653a6 100644 --- a/.claude/skills/code-comments/SKILL.md +++ b/.claude/skills/code-comments/SKILL.md @@ -13,6 +13,9 @@ 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 member summary describes that member'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.ps1` fails on them over the lines your branch adds: @@ -39,16 +42,27 @@ excluded. Every line fits **120 display columns**. `///` blocks and PowerShell block comments are exempt from the budget, not from the content rules or the width limit. -Over budget? It belongs in an XML summary, or the code should express it -directly. +Over budget? Shorten it, or let the code express it directly. Do not convert a +`//` block to `///` 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. ## XML documentation -One `` above the member, including a public constructor with -parameters. Omit `` and `` that only restate a name or type; -keep them for units, nullability, ownership, or real result semantics. Document -every parameter or none. No file headers, no divider comments, no fact repeated -in both the summary and the parameters. +Prefer none. An undocumented public type is the norm here, several projects use +`///` nowhere at all, and HermitCrab's heavier use is not a model to copy. +`SIL.Machine.Morphology.HermitCrab`, `SIL.Machine.Tokenization.SentencePiece`, +and `SIL.Machine.Translation.TensorFlow` do not set `GenerateDocumentationFile` +at all, so a `///` block there ships nothing to a consumer. + +Write one only when a caller needs a contract the signature cannot state: units, +nullability, ownership, an exception they must handle. Then one `` +above the member. Omit `` and `` that only restate a name or +type; keep them for real result semantics. Document every parameter or none. No +file headers, no divider comments, no fact repeated in both the summary and the +parameters. A test comment explains a non-obvious fixture or setup constraint. It does not restate the test name. diff --git a/.claude/skills/pr-authoring/SKILL.md b/.claude/skills/pr-authoring/SKILL.md index 53160bcc4..98c325774 100644 --- a/.claude/skills/pr-authoring/SKILL.md +++ b/.claude/skills/pr-authoring/SKILL.md @@ -54,6 +54,9 @@ Under 200 words. Drop any section that would be empty. ``` +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 diff --git a/.github/workflows/comment-hygiene.yml b/.github/workflows/comment-hygiene.yml index 91da9d628..01e8cfdd9 100644 --- a/.github/workflows/comment-hygiene.yml +++ b/.github/workflows/comment-hygiene.yml @@ -12,8 +12,8 @@ concurrency: cancel-in-progress: true jobs: - # Separate from CI Build so the report arrives in about a minute instead of - # waiting on the native build, the Release build, and the test matrix. + # Separate from CI Build so the report does not wait on the native build, the + # Release build, and the test matrix. comment_hygiene: name: Report comment hygiene (${{ matrix.os }}) runs-on: ${{ matrix.os }} diff --git a/docs/review/devils-advocate.md b/docs/review/devils-advocate.md index 18ec5bf67..047d0c5e5 100644 --- a/docs/review/devils-advocate.md +++ b/docs/review/devils-advocate.md @@ -5,9 +5,13 @@ 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. +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