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