Add a diff-scoped comment-hygiene ratchet for build.ps1/test.ps1 and CI - #1061
Merged
Conversation
johnml1135
force-pushed
the
comment-enforcement
branch
from
August 13, 2026 11:53
5d614d3 to
7026fa0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1061 +/- ##
==========================================
+ Coverage 38.04% 38.07% +0.02%
==========================================
Files 1499 1499
Lines 350127 350145 +18
Branches 40239 40239
==========================================
+ Hits 133215 133306 +91
+ Misses 187625 187560 -65
+ Partials 29287 29279 -8
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
Enforces the fieldworks-code-commenting standard's banned-content rules and a non-ascii-punctuation check against lines a diff adds, wired into build.ps1, test.ps1, and CI. Includes a triage tool that attributes existing violations to their introducing commit, and a stray-docs CI gate blocking brainstorming/planning markdown from surviving into a merged PR. The comment-length rule is a 200-character budget rather than a hard 1-line cap, since the cap forced every explanation onto one line that then broke .editorconfig's line-length limit once genuinely non-trivial. This retroactively surfaced 127 pre-existing over-long comments, all tracing via git blame to the already-merged Avalonia foundation commit (#964); all 127 are reformatted into multi-line prose here. Adds a powershell-compat gate (regex scan plus best-effort PSScriptAnalyzer) and dual-shell CI test runs, after an independent review surfaced three functional bugs specific to Windows PowerShell 5.1 vs 7: a Set-StrictMode crash in local base-ref resolution, a CI checkout missing fetch-depth that left origin/main unresolvable, and a PowerShell 6+-only escape sequence that silently disabled ASCII detection under 5.1 -- the exact engine CI's build.ps1/test.ps1 steps run under. All three are fixed and verified by direct reproduction under both engines, not just reasoned about. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
"ASCII-only" was misleading -- the rule only bans a specific punctuation set (em-dash, arrows, smart quotes), not all non-ASCII content, which legitimately appears in comments quoting real script or IPA text. Also condenses fieldworks-code-commenting/SKILL.md from 203 to 129 lines: cuts the worked-examples section and tightens every other section's prose. No rule content is removed, only reworded shorter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Widens the diff-scoped gate from C#/PowerShell to also cover C/C++/IDL (//, ///) and the <!-- --> comments in project files and Avalonia views, per fieldworks-code-commenting. Adds a first-block doc-comment exemption for Xml, mirroring /// and PowerShell's help block, since Xml has no other syntax to mark a file/type-level summary -- without it, sixteen dialog and project-file overview comments were losing real content to the 200-character budget. Fixes two real bugs found while doing this: the gate was scanning every line of every changed file with Get-Content, ~50x slower than File.ReadAllLines, plus a network round-trip in local base-ref resolution via git remote show origin -- both fixed, dropping the gate from 5.4s to ~2.2s. Separately, the XML spec forbids a literal "--" anywhere in comment content, so the existing ASCII-punctuation replacement (em dash to "--") produces invalid XML inside <!-- --> comments; added a mechanical xml-illegal-double-hyphen check and use a single "-" there instead. Blamed the newly in-scope violations to their introducing commits: 87 trace to AI-authored commits and are fixed directly; the remaining ~5000 are pre-existing legacy debt, left for the diff-scoped ratchet to grandfather like the existing C#/PowerShell backlog. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Audited every "bash" reference in the repo and git-blamed each one. Nearly all traced to one commit, 5711bf6 ("enable AI workflows"), which set the powershell skill's allowed-tools to Bash(pwsh:*) instead of a native PowerShell tool -- the actual mechanism behind agents defaulting to Bash across worktrees -- plus five .sh scripts duplicating existing, already cross-platform-safe .ps1 equivalents. Fixes: the skill now grants PowerShell directly; the five duplicate .sh scripts are deleted (verified check-whitespace.sh and its .ps1 twin produce identical output first); the three ubuntu-latest CI workflows (CommitMessage, check-whitespace, stray-docs) now run shell: pwsh instead of bash, with the sed/GITHUB_ENV-heredoc logic ported to PowerShell and verified locally; and ~44 bash-labeled doc code fences for shell-agnostic commands are relabeled powershell. Left alone: those workflows' ubuntu-latest runner choice, vendored graphite2/update.sh, the 2018 Bin/list-assembly-references script, WavConverter.cs's runtime /bin/bash call (real Linux code path, not tooling), and one git rebase --exec example whose inner string must stay POSIX syntax regardless of shell, since git always runs --exec through its own bundled sh. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The build job failed: powershell-compat.ps1 still called Get-CommentLineClassification with the old -IsPowerShell boolean parameter, which the comment-hygiene scope-expansion commit replaced with -Language. Missed because that call site was never exercised locally this session -- only the module's own fixture suite was run, not build.ps1 end to end. Fixed and verified directly under both PowerShell engines. The commit-message job failed separately: Tee-Object never creates (or even truncates) its target file when the piped command emits zero objects, unlike bash's tee, which always creates the file. A clean gitlint run -- the common case -- produces no output, so check_results.log never existed for the next step to read. Fixed by pre-creating the file before the pipeline runs; verified the gap and the fix directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Narrows the PR's comment edits to the five commits that introduced the bulk of them: the Avalonia conversion foundation (#964), the Views render optimizations, the .NET tooling modernization (#678), the Charis test font change (#945), and the blinking-caret snapshot fix (#943). The eight files reverted here carried one to five edited comment lines each, traced by git blame to unrelated commits going back to 2012. Each file is restored to its merge-base content; all eight diffs were comment-only, so no code changes are affected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The style section told agents to prefer PowerShell Core syntax, which is backwards for this repo: CI runs build.ps1 and test.ps1 under Windows PowerShell 5.1, and powershell-compat.ps1 exists because 6+-only syntax passes a parse on 7 and then misbehaves there. It now requires both engines and points at that script. Adds three traps hit while auditing this branch, each reproduced in isolation first: an operator after an unparenthesized function call binds as an argument and is silently ignored; Measure-Object -Sum over an empty collection returns null and throws on property access under StrictMode; and returning a collection unrolls it unless prefixed with a comma. Also notes that --% consumes the rest of the line, so it cannot sit inside a subexpression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a comment-line-too-long rule that reads max_line_length and tab_width from .editorconfig, so the comment limit is the one the rest of the repo already follows. It counts display columns rather than characters, since this repo indents with tabs, and it applies to doc comments too: a doc comment is exempt from the content budget because of what it says, not because it may run off the screen. A local run re-wraps the line, CI only reports it. The 200-character content budget rises to 600 where the code a comment introduces scores at least 10 decision points. Measure-CodeComplexity counts them, stopping at the end of the enclosing block or after 40 lines. Measured across the whole repo, that extends 2 percent of the comments already over 200, which is the intended rarity. The gate then re-wrapped this branch's own over-long comments across 210 files. Six needed shortening by hand, and XML comment indentation is normalized to tabs so the new lines pass git's indent-with-non-tab check. Also seeds two script-scoped variables before they are read: Set-StrictMode throws on an unset variable, which powershell-compat.ps1 hit on every run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jasonleenaylor
force-pushed
the
comment-enforcement
branch
from
August 17, 2026 14:59
093ba5f to
8a9c8fc
Compare
comment-hygiene.ps1 built its added-line filter from the committed diff while the scan reads each file from disk. Once a local auto-fix rewrote a file the filter pointed at stale line numbers, so the gate checked the wrong lines and reported clean. CI, whose tree matches HEAD, failed on 24 violations instead. The filter now diffs the merge base against the working tree. With that fixed the gate reports 20 over-budget comment blocks. Five wrapped automatically; the rest are rewritten. Four become /// doc comments, which keeps their full text: they sit above a declaration and describe its own contract, and a doc comment is exempt from the content budget. Most of the shortening drops duplication rather than reasoning. Several comments restated a value or symbol that a neighbouring declaration already owned. One test comment explained that no other test covered the case, which the section divider above it repeated; that divider is deleted too. Also drops touch vocabulary from the rich-text comment: a keyboard shortcut stages an edit, it is not a gesture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment length and width are a discipline for generated code, not something a developer should be blocked on. build.ps1 and test.ps1 take -CommentHygiene, which agents are now required to pass: it fails the run on any violation in the lines the branch adds, so an agent fixes its own comments before review. A developer build omits the flag and never runs the gate at all. CI keeps reporting without failing. Under GitHub Actions the gate runs in a new -Advisory mode that prints every violation, emits it as a ::warning annotation so it lands on the pull request's diff, and exits 0. powershell-compat stays a hard failure for everyone, since it catches code that breaks at runtime under Windows PowerShell 5.1 rather than a style preference. Also closes a gap in the added-line filter: git diff reports tracked files only, so a new file an agent had not staged yet went unscanned. Untracked in-scope files are now folded in with every line treated as added, verified by probe both ways. The agent instructions carry the flag on every documented build and test command, including an explicit note not to drop it to get a run to pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deliberately trips both comment-hygiene categories on a single line so CI has something real to annotate. To be reverted as soon as the run confirms the advisory path emits warning annotations without failing the build. Locally the probe behaves as designed: advisory mode reports two violations, emits two annotations and exits 0, while enforced mode exits 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…otations" This reverts commit b9d16cd.
The build step runs build.ps1 and the test step runs test.ps1, so the gate ran twice over the same tree and GitHub registered every violation twice. Verified on a probe commit: two real violations produced four annotations. The first run in a job now records a marker in GITHUB_ENV and later steps skip reporting. Scoped to GitHub Actions on purpose. A marker in the process environment would persist across runs in one interactive session, so an agent fixing a comment and re-running would get a false pass on the second run. Local runs always scan. The marker is appended with UTF8Encoding($false); a BOM part way through the environment file would break the parse the runner does when the step ends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jasonleenaylor
enabled auto-merge (squash)
August 17, 2026 20:31
jasonleenaylor
disabled auto-merge
August 17, 2026 20:31
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
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.
Adds a mechanical, diff-scoped comment-hygiene gate enforcing the
fieldworks-code-commentingstandard, wired intobuild.ps1,test.ps1, and CI. It covers C#/C/C++/IDL, PowerShell, and the<!-- -->comments in project files and Avalonia views. Alongside the banned-content categories it now enforces a per-line width taken from.editorconfigand a content budget that rises in densely branching code. Also fixes a repo-wide bash/PowerShell inconsistency traced back to a misconfigured skill permission that was letting agents default to Bash instead of PowerShell.The gate blocks coding agents and no one else. Comment length and width are a discipline for generated code, not something a developer should be stopped on:
.\build.ps1 -CommentHygiene.\build.ps1.\build.ps1AGENTS.mdand the.github/instructions/*build and test docs now carry the flag on every documented command, including an explicit instruction not to drop it to get a run to pass.powershell-compat.ps1remains a hard failure for everyone: it catches code that breaks at runtime under Windows PowerShell 5.1, which is a defect rather than a style preference.265 of the 266 changed
.csfiles are comment-only -- re-verified at the current tip by stripping comment and blank lines from each file'sorigin/mainandHEADversions and diffing the remainder: zero non-comment differences. The one exception is a new characterization test. The real review surface is the tooling, CI, and skill files.Where to look:
Build/Agent/comment-hygiene.ps1's added-line filter -- it diffs the merge base against the working tree, not againstHEAD, and folds in untracked in-scope files. Both matter: the scan reads each file from disk, so a filter built from committed content goes stale the moment an auto-fix rewrites a file, andgit diffnever reports a file the author has not staged yet.-Advisorymode -- reports violations, emits each as a::warning file=,line=annotation, and exits 0. The first advisory run in a CI job records a marker inGITHUB_ENVso the build and test steps do not annotate the same violation twice.CommentHygiene.psm1'scomment-line-too-longcheck -- it readsmax_line_lengthandtab_widthfrom.editorconfigitself rather than carrying its own number, and measures display columns, not characters, because this repo indents with tabs. It applies to doc comments too: those are exempt from the content budget because of what they say, not because they may run off the screen.Measure-CodeComplexityand the extended budget -- a comment introducing a region that scores 10 or more decision points gets 600 characters instead of 200. Nothing opts in by hand; the threshold was calibrated against the corpus rather than guessed.--anywhere inside a<!-- -->comment, so the standard's usual em-dash-to---fix is invalid there. A mechanical check catches it and a single-is used instead..github/workflows/{CommitMessage,check-whitespace,stray-docs}.yml-- switched fromshell: bashtoshell: pwsh.check_results.logis pre-created before thegitlintpipe becauseTee-Object, unlike bash'stee, never creates its target file when the piped command emits zero objects, which is the common case on a clean run.Deliberately not here: C-style
/* */block comments still are not scanned, in any language. Five now-redundant.shscripts are deleted rather than kept as a bash fallback. The extended budget keys on measured branching only, so a genuinely non-obvious problem/solution write-up above straight-line code is not detectable and still gets 200 characters. And advisory violations surface on the pull request's Files-changed tab and check-run page, but not on its conversation page; putting them there needs a sticky comment and is left for a follow-up.Verification: All CI checks pass on the current head, including the full Debug build and test run. That build is the first to compile the 266 changed
.csfiles and to exercise the four///doc-comment conversions, which matters because this repo treats compiler warnings as errors. The fixture suite passes under both PowerShell 7 and Windows PowerShell 5.1. The diff-scoped gate is clean,git diff --checkis clean,gitlintis clean, and all 19 changed project-file/Avalonia-view files parse as XML. The advisory path was verified against a deliberately bad comment on a throwaway commit, then reverted (see Evidence). Full-repo scan: 22,335 pre-existing violations across 2,836 files, all grandfathered by the ratchet. Not stacked.Reading this a year from now -- start here
This branch originally shipped with a design spec and implementation plan under
Docs/superpowers/. Both are deleted here: the plan was a task-by-task build log now fully superseded by the code, and the design doc's factual claims (PowerShell out of scope, a singlenon-asciicategory flagging any non-ASCII byte, no comment-length redesign) no longer match what shipped. The durable reasoning from both is preserved below instead.Decisions, and why
motifrepo'stools/comment-hygiene.ps1, but adapted: new and touched comments must comply, and the existing corpus is not blocked on a repo-wide sweep. A-Fullreport-only mode exists separately for finding what a file already carries before you touch it..editorconfigmeans there is exactly one line-length policy in the repo, and it cannot drift.///rather than shortened. Four sat above a declaration and described that member's own contract, which is what a doc comment is for;//was the original mistake. Doc comments are budget-exempt, so their full text survives. This is not applied to group headers or in-method notes, where it would be a dodge.non-ascii-punctuation, notnon-ascii. The category flags a specific Western-typography punctuation set (em dash, arrows, smart quotes), not all non-ASCII content -- this codebase legitimately quotes real script and IPA text in comments.<!-- -->block is exempt from the length budget. It plays the same file/type-summary role as a C#///or a PowerShell help block, but Xml has no separate doc-comment syntax to mark it by.git blame, not guesses. The bash/PowerShell audit started from grepping every "bash" mention in the repo and blaming each one. Nearly all traced to a single commit that set the PowerShell skill's tool permission to route through Bash; that permission, not habit, was the mechanism.Paths not taken
<!--!-style marker for "this is a doc comment": rejected as inventing syntax the XML/MSBuild/XAML ecosystem does not have. The first-comment-in-document convention was already how every affected file was structured..shscripts as a bash fallback: rejected once their.ps1twins were confirmed to have#!/usr/bin/env pwshshebangs and cross-platform-safe implementations, verified by runningcheck-whitespace.shandcheck-whitespace.ps1side by side and confirming identical output before deleting the original.Reversals
.csonly, explicitly excluding PowerShell. PowerShell scanning was added after the tooling's own comments shipped unscanned.-CommentHygieneis passed.powershellskill advised preferring PowerShell Core syntax "where possible". That is backwards for this repo -- CI runsbuild.ps1andtest.ps1under Windows PowerShell 5.1 -- and is now a dual-engine requirement pointing atpowershell-compat.ps1.WelcomeToFieldWorksDlg.cs,LexOptionsDlg.cs, andFwXWindow.csare reverted.mainhad independently ASCII-fixed those same comments while keeping more of their text, and had rewritten the code underLexOptionsDlg.cs, so this branch's comment described logic that no longer exists. All three files now matchmainexactly.git blameto unrelated commits spread from 2012 to 2026, rather than to the Avalonia conversion foundation and the four other commits this branch's churn actually belongs to.Evidence
.csfile, stripped comment and blank lines from both theorigin/mainandHEADversions and diffed the remainder -- zero non-comment differences across all but one, a genuinely new test file. Re-run at the current tip, not carried over from an earlier state.HEAD, failed on 24 violations at the same commit. Proved by line count:CommentHygiene.psm1was 430 lines at the commit the local run saw and 748 at the tip, and one reported violation sat at line 706 -- a line that could not exist in a 430-line filter.Src/xWorks/xWorksTests/Search/BulkEditReplaceCharacterizationTests.cs:5with titles naming both categories. That run also exposed duplicate annotations -- two violations produced four -- because the build and test steps each ran the gate; hence theGITHUB_ENVmarker.comment-too-longcount drops from 4,020 to 3,943 with the extension enabled..editorconfig's 98 columns once tabs are counted as display columns, with a maximum of 205.git blameover every changed comment line resolved 1,621 of 1,742 (93%) to31c0a6157, the Avalonia conversion foundation (LT-22625: Add the WinForms to Avalonia conversion foundation #964), then six days old. The eight files whose comments traced elsewhere are the ones dropped above.git diff --checkrejects under this repo'sindent-with-non-tabsetting. Xml comment indentation is normalised to tabs, and the check is clean.pwshand 7 (the literal text) underpowershell.exe.Get-Contentmeasured 51ms versusFile.ReadAllLinesat 1ms on a 5446-line file, andgit remote show originabout 515ms versusgit rev-parse --abbrev-ref origin/HEADat about 31ms with no network call. End to end: 5.4s before, about 2.2s after.git blameplus aCo-Authored-By: Claudetrailer check on all 1635 violations newly in scope from the C/C++/IDL/Xml extension found 87 across 27 files tracing to AI-authored commits, mostly LT-22625: Add the WinForms to Avalonia conversion foundation #964; all 87 fixed and re-verified clean.powershell-compat.ps1read a$Global:variable before assigning it, which errors on every run underSet-StrictMode -Version Latest. Two more of the same shape were introduced and caught while building the width rule.This change is