Add agent guidance, review rules, and a comment-hygiene gate - #514
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #514 +/- ##
=======================================
Coverage 74.09% 74.09%
=======================================
Files 456 456
Lines 38107 38107
Branches 5221 5221
=======================================
Hits 28237 28237
Misses 8712 8712
Partials 1158 1158 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
johnml1135
force-pushed
the
ai-tooling/comment-hygiene
branch
from
September 18, 2026 02:19
2bcc944 to
f3c34ae
Compare
ddaspit
approved these changes
Sep 18, 2026
ddaspit
left a comment
ddaspit
left a comment
Contributor
There was a problem hiding this comment.
@ddaspit reviewed 24 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on johnml1135).
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 6979680 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 03621d1, an invalid chapter in 36a24b5, wrong chapter numbers in f9ba7bb. 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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) <noreply@anthropic.com>
ddaspit
force-pushed
the
ai-tooling/comment-hygiene
branch
from
September 18, 2026 21:36
041d178 to
907c8b9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Quick summary
An agent working in this repository now gets the rules and the traps it cannot
read off the tree, and cannot land a comment that narrates its own history. The
gate does not block existing work: a full scan finds 73 violations across the
727 files in scope, all in code this branch leaves alone, so the checker only
reads the lines a branch adds. No C# changes,
ci.ymluntouched, and the CIscan is advisory - only
local_check.sh --agent-strictfails on a violation.Where to look
scripts/comment-hygiene.ps1checks only lines addedagainst the merge base;
-SelfTestcovers C#, shell, and PowerShell.scripts/CommentHygiene.psm1holds them.///andPowerShell blocks are budget-exempt only, still content- and width-checked.
AGENTS.mdkeeps rules and the four places the treemisleads a reader. Anything an
lsor agrepanswers was cut, not copied.docs/review/rule is tied to a commit or areview comment from the last three years; rules with no such evidence are gone.
Deliberately not included
CONTEXT.mdwent with it; the vocabularytraps that earned their place are four lines in
AGENTS.md.Validation
./local_check.sh --agent-strict- exit 0: branch scan clean, csharpierclean, Release build 0/0, 1,024 tests (1,021 passed, 3 pre-existing skips).
pwsh ./scripts/comment-hygiene.ps1 -SelfTest- passed.pwsh ./scripts/comment-hygiene.ps1 -Full -Advisory- 73 violations across727 in-scope files: 22 width, 49 budget, 2 content, all pre-existing.
git diff --check,git log --check- clean. Three issue forms and the newworkflow parse as YAML.
Issue / porting context
No issue: repository tooling, not ported behavior.
Reading this a year from now
The gate exists because agents write comments that narrate their own work -
provenance ("ported from"), absence ("no longer does X"), and restatement of the
line below. Those comments are invisible to CI, survive review, and accumulate.
The checker encodes the standard so an agent fixes its own output before a human
sees it, and it is diff-scoped because the tree already carries 73 violations
nobody is going to fix in one pass.
The guidance around it was written twice. The first version described the
repository: layout, target frameworks, what each interface declared, what CI
ran. Three years of history then said which of those rules had ever caught
anything, and most had not - so the second version keeps only what an agent
cannot derive by looking, and the corpus fell from 5,854 words to 3,447.
Decisions, and why
contributor; an agent has no excuse for adding to it. The self-test step stays
blocking, because a checker that fails its own self-test is broken.
enforced scope have to be the same set, or the rule is whatever the code does.
docs/review/over.github/instructions/. Copilot'sapplyTo:frontmatter only means something to Copilot. Plain Markdown plus a glob table in
AGENTS.mdworks for any agent that can read a repository.and the issue. What to check lives in
AGENTS.mdanddocs/review/.that will be wrong later. Layout, target frameworks, CI job contents and the
interface tour were all cut for this reason.
What the history said
The cuts and the two additions came from reading the repository rather than
guessing: 232 commits and 336 human review comments since
src/SIL.Machine.AspNetCorewas removed in69796806.15, and every translation engine and tokenizer combined 5.
defect class that recurs most, followed by ordinal comparison and stream
ownership - each now cited in the rules file that governs it.
PunctuationAnalysis/had no rules file and a history of nothing but crashfixes, so
docs/review/punctuation.mdis new.asks what happened to each finding.
Deferred, and what would unblock it
budget is wrong for a real hot path.
commit-messagesskill documents theconvention; no gitlint gate was added.
🤖 Generated with Claude Code
This change is