From ca1fbba2a7c9819776c2a62799ef4ffe8d28a0b5 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Mon, 17 Aug 2026 15:21:51 -0700 Subject: [PATCH 1/4] feat: post advisory comment-hygiene violations to the pull request A new CommentHygiene.yml job scans the lines a pull request adds and posts one sticky comment listing them, updated in place on each push and replaced with a clean note once they are fixed. Nothing in it can fail a check. comment-hygiene.ps1 gains -ReportPath to write the scan result as JSON, and Build-CommentHygieneComment.ps1 renders that into the comment body. - CI.yml presets FW_COMMENT_HYGIENE_REPORTED so the new workflow is the only place that annotates, which moves the fetch-depth: 0 requirement out of the build job, and runs the composer smoke test under both engines. - comment-hygiene.ps1 falls back to origin/main when origin/HEAD is unset: git rev-parse wrote to stderr, which 5.1 turns into a terminating error, so the documented fallback was unreachable. - Wording says check rather than gate, and the agent requirement lives in AGENTS.md alone rather than repeated in help text that does not enforce it. - powershell-compat.ps1 walks Build/Agent instead of naming six files, so a new script is covered when it lands. Co-Authored-By: Claude Opus 5 --- .../fieldworks-code-commenting/SKILL.md | 23 +- .github/instructions/build.instructions.md | 4 +- .github/instructions/testing.instructions.md | 1 - .github/workflows/CI.yml | 17 +- .github/workflows/CommentHygiene.yml | 69 ++++++ AGENTS.md | 5 +- Build/Agent/Build-CommentHygieneComment.ps1 | 213 ++++++++++++++++++ Build/Agent/CommentHygiene.psm1 | 6 +- .../Agent/Test-BuildCommentHygieneComment.ps1 | 136 +++++++++++ Build/Agent/comment-hygiene.ps1 | 80 ++++++- Build/Agent/powershell-compat.ps1 | 20 +- build.ps1 | 5 +- test.ps1 | 5 +- 13 files changed, 535 insertions(+), 49 deletions(-) create mode 100644 .github/workflows/CommentHygiene.yml create mode 100644 Build/Agent/Build-CommentHygieneComment.ps1 create mode 100644 Build/Agent/Test-BuildCommentHygieneComment.ps1 diff --git a/.claude/skills/fieldworks-code-commenting/SKILL.md b/.claude/skills/fieldworks-code-commenting/SKILL.md index 4e2c0bfd21..2fca35870d 100644 --- a/.claude/skills/fieldworks-code-commenting/SKILL.md +++ b/.claude/skills/fieldworks-code-commenting/SKILL.md @@ -22,6 +22,12 @@ A C-style `/* */` block comment is not scanned -- only whole-line rules (accuracy, WHAT-not-HOW, standalone clarity) are not mechanically checked and still apply while authoring. +In CI a violation fails nothing. Each one lands as a warning annotation on +the diff, and the whole set as a single pull request comment that updates +in place on every push. `build.ps1 -CommentHygiene` and +`test.ps1 -CommentHygiene` are what make the same violations blocking, and +an agent passes one of them on every run. + ## The standard 1. **Accuracy first, then brevity.** A wrong comment is worse than none. @@ -115,9 +121,9 @@ Place above the nesting level the code spans. **Line width is separate, and applies to every comment line**, doc comments included: no comment line may exceed `.editorconfig`'s `max_line_length` -(98 columns today), counting a tab as `tab_width` columns. The gate reads -those two values from `.editorconfig` itself, so the limit can never drift -from the one the rest of the repo follows. Enforced as +(98 columns today), counting a tab as `tab_width` columns. +`comment-hygiene` reads those two values from `.editorconfig` itself, so the +limit can never drift from the one the rest of the repo follows. Enforced as `comment-line-too-long`; a local run re-wraps the line for you, CI only reports it. @@ -125,12 +131,11 @@ reports it. introducing a region whose decision-point count reaches 10 (McCabe complexity 11 -- the classic "high" threshold) gets the larger budget automatically, because a reader there needs the invariants spelled out and -200 characters buys about two sentences. Nothing opts in by hand: the gate -measures the code the comment introduces, stopping at the end of the -enclosing block or 40 lines. This fires on roughly 2% of the comments -already over 200 characters, and is meant to stay that rare -- if a comment -in ordinary straight-line code will not fit, shorten it rather than looking -for a way to qualify. +200 characters buys about two sentences. `comment-hygiene` measures the code +the comment introduces, stopping at the end of the enclosing block or 40 +lines. This fires on roughly 2% of the comments already over 200 characters, +and is meant to stay that rare -- if a comment in ordinary straight-line code +will not fit, shorten it rather than looking for a way to qualify. **Exemptions from the length cap:** a C#/C/C++/IDL `///` doc comment; a PowerShell comment-based help block (`<# ... #>`); and, in a project file or diff --git a/.github/instructions/build.instructions.md b/.github/instructions/build.instructions.md index 7d5ca4ec65..5d24fbc1dd 100644 --- a/.github/instructions/build.instructions.md +++ b/.github/instructions/build.instructions.md @@ -23,9 +23,7 @@ FieldWorks is **Windows-first** and **x64-only**. Use the repo scripts so build - Use `.\build.ps1` for builds and `.\test.ps1` for tests. - Pass `-CommentHygiene` on every build and test run. It fails the run on any comment-hygiene violation in the lines your branch adds, so you fix your own - comments before review. Humans omit it and never see the gate; CI reports - violations as warning annotations without failing. Never drop the flag to get - a run to pass. + comments before review. Never drop the flag to get a run to pass. - Avoid ad-hoc `msbuild`/`dotnet build` invocations unless you are explicitly debugging build infrastructure. - Do not change COM/registry behavior without an explicit plan and tests. diff --git a/.github/instructions/testing.instructions.md b/.github/instructions/testing.instructions.md index 894126e9a6..2393f5f302 100644 --- a/.github/instructions/testing.instructions.md +++ b/.github/instructions/testing.instructions.md @@ -17,7 +17,6 @@ Guidance for writing and running deterministic unit and integration tests for Fi Use `.\test.ps1` for all managed (C#) tests. Always pass `-CommentHygiene`: it fails the run on any comment-hygiene violation in the lines your branch adds. -Humans omit it and never see the gate; CI annotates without failing. ```powershell # Run all tests (builds first) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index c1b2d5c4e5..030f3edd7b 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -19,15 +19,18 @@ jobs: debug_build_and_test: name: Build Debug and run tests runs-on: windows-2022 + env: + # CommentHygiene.yml owns comment-hygiene reporting, so the build and test steps + # below stay quiet rather than annotating every violation a second time. Clearing + # this makes comment-hygiene.ps1 diff against origin/ again, which + # needs fetch-depth: 0 on the checkout below or it fails with "bad revision". Set + # unconditionally: a job-level value overrides one a step writes to GITHUB_ENV, so + # anything conditional here would also clobber that in-job marker. + FW_COMMENT_HYGIENE_REPORTED: 1 steps: - name: Checkout Files uses: actions/checkout@v7 id: checkout - with: - # comment-hygiene.ps1 diffs against origin/; a - # shallow, single-branch checkout leaves that ref unresolvable - # and the gate fails every build with "bad revision". - fetch-depth: 0 # Real-runtime check: this repo is authored on PowerShell 7 but # build.ps1/test.ps1 run under Windows PowerShell 5.1 below. A script @@ -42,6 +45,8 @@ jobs: run: | Build/Agent/CommentHygiene.Tests.ps1 if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + Build/Agent/Test-BuildCommentHygieneComment.ps1 + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } - name: Comment hygiene fixture tests (Windows PowerShell 5.1) id: comment-hygiene-tests-winps @@ -49,6 +54,8 @@ jobs: run: | Build\Agent\CommentHygiene.Tests.ps1 if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + Build\Agent\Test-BuildCommentHygieneComment.ps1 + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } - name: Build with tests id: build diff --git a/.github/workflows/CommentHygiene.yml b/.github/workflows/CommentHygiene.yml new file mode 100644 index 0000000000..37636c612c --- /dev/null +++ b/.github/workflows/CommentHygiene.yml @@ -0,0 +1,69 @@ +name: Comment hygiene check +on: + pull_request: + +permissions: + contents: read + # pull-requests: write lets the sticky-comment steps post and clear the + # comment-hygiene comment on the PR. + pull-requests: write + +# Avoid unnecessary runs +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + # Separate from the build so the report lands on the pull request in about a + # minute instead of waiting on Debug plus tests, and still arrives when the + # build fails or is superseded. + comment_hygiene: + name: Report comment hygiene + runs-on: windows-2022 + steps: + - name: Checkout repository + uses: actions/checkout@v7 + with: + # comment-hygiene.ps1 diffs against origin/; a shallow, + # single-branch checkout leaves that ref unresolvable. + fetch-depth: 0 + + # Windows PowerShell 5.1 on windows-2022: the engine and platform the + # comment-hygiene scripts are exercised under everywhere else in CI. + - name: Scan the lines this pull request adds + shell: powershell + continue-on-error: true + run: | + .\Build\Agent\comment-hygiene.ps1 -Advisory -ReportPath .\Output\CommentHygiene\report.json + + - name: Compose the pull request comment + id: comment + shell: powershell + continue-on-error: true + env: + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + .\Build\Agent\Build-CommentHygieneComment.ps1 -ReportPath .\Output\CommentHygiene\report.json + + # Advisory to the end: a comment this workflow cannot post -- a pull request + # from a fork gets a read-only token -- must not read as a failed check. + - name: Post the comment-hygiene summary + if: ${{ steps.comment.outputs.has_violations == 'true' }} + continue-on-error: true + uses: marocchino/sticky-pull-request-comment@0ea0beb66eb9baf113663a64ec522f60e49231c0 # v3.0.4 + with: + header: fieldworks-comment-hygiene + path: ${{ steps.comment.outputs.comment_path }} + skip_unchanged: true + + # only_update: a clean pull request has nothing to say, so this replaces an + # earlier summary where one exists and posts nothing where none does. + - name: Clear the comment-hygiene summary + if: ${{ steps.comment.outputs.has_violations == 'false' }} + continue-on-error: true + uses: marocchino/sticky-pull-request-comment@0ea0beb66eb9baf113663a64ec522f60e49231c0 # v3.0.4 + with: + header: fieldworks-comment-hygiene + only_update: true + path: ${{ steps.comment.outputs.comment_path }} + skip_unchanged: true diff --git a/AGENTS.md b/AGENTS.md index e7dabcfbaf..4231d2516f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,9 +9,8 @@ Minimal, high-signal guidance for coding agents in this repository. - Test with `.\test.ps1 -CommentHygiene`. - `-CommentHygiene` is required of agents and not of humans: it fails the run on any comment-hygiene violation in the lines your branch adds, so you fix your - own comments before they reach review. A developer build omits it and never - runs the gate; CI reports violations as warning annotations without failing. - Do not drop the flag to get a build through. + own comments before they reach review. Do not drop the flag to get a build + through. - Do not bypass repository scripts for normal build/test work. - Commit messages must pass `gitlint` (CI: `.github/workflows/CommitMessage.yml`): title <=72 characters, body lines <=80 characters, blank line between diff --git a/Build/Agent/Build-CommentHygieneComment.ps1 b/Build/Agent/Build-CommentHygieneComment.ps1 new file mode 100644 index 0000000000..27c185ce0a --- /dev/null +++ b/Build/Agent/Build-CommentHygieneComment.ps1 @@ -0,0 +1,213 @@ +<# +.SYNOPSIS + Renders the comment-hygiene pull request comment from a scan report. + +.DESCRIPTION + Turns the report comment-hygiene.ps1 writes under -ReportPath into a markdown + body for a sticky pull request comment, and publishes comment_path and + has_violations as GitHub step outputs so the workflow can pick between posting + and clearing. Violations are listed as a table of file links, categories, and + comment text, capped at -MaxListed rows. + +.PARAMETER ReportPath + The JSON report comment-hygiene.ps1 writes. A missing file is an error, not an + empty report: a scan always writes one. + +.PARAMETER CommentPath + Where to write the markdown body. Defaults beside the report. + +.PARAMETER MaxListed + How many violations the table carries before it summarizes the rest as a + count. + +.PARAMETER Sha + Commit the file links resolve against. Pass a pull request's head commit; + links fall back to plain text when this or Repository is unset. + +.EXAMPLE + Build/Agent/Build-CommentHygieneComment.ps1 -ReportPath Output/CommentHygiene/report.json +#> +[CmdletBinding()] +param( + [string]$CommentPath, + [string]$GitHubOutputPath = $env:GITHUB_OUTPUT, + [int]$MaxListed = 25, + [string]$ReportPath, + [string]$Repository = $env:GITHUB_REPOSITORY, + [string]$RunId = $env:GITHUB_RUN_ID, + [string]$ServerUrl = $env:GITHUB_SERVER_URL, + [string]$Sha = $env:HEAD_SHA +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +$repoRoot = (Resolve-Path (Join-Path $PSScriptRoot '..\..')).Path +if ([string]::IsNullOrWhiteSpace($ReportPath)) { + $ReportPath = Join-Path (Join-Path $repoRoot 'Output\CommentHygiene') 'comment-hygiene-report.json' +} +if ([string]::IsNullOrWhiteSpace($CommentPath)) { + $CommentPath = Join-Path (Split-Path -Path $ReportPath -Parent) 'comment-hygiene-comment.md' +} +if ([string]::IsNullOrWhiteSpace($ServerUrl)) { + $ServerUrl = 'https://github.com' +} +if ([string]::IsNullOrWhiteSpace($Sha)) { + $Sha = $env:GITHUB_SHA +} + +# PowerShell's current location and .NET's working directory can differ; this script +# reads through the first and writes through the second, so anchor both paths once. +if (-not [System.IO.Path]::IsPathRooted($ReportPath)) { + $ReportPath = Join-Path (Get-Location).Path $ReportPath +} +if (-not [System.IO.Path]::IsPathRooted($CommentPath)) { + $CommentPath = Join-Path (Get-Location).Path $CommentPath +} + +$SkillPath = '.claude/skills/fieldworks-code-commenting/SKILL.md' + +function Write-GitHubOutputValue { + param( + [Parameter(Mandatory = $true)] + [string]$Name, + [Parameter(Mandatory = $true)] + [string]$Value + ) + + if ([string]::IsNullOrWhiteSpace($GitHubOutputPath)) { + return + } + + # UTF8Encoding($false): a byte-order mark part way through the output file breaks + # the runner's parse of every value after it. + $encoding = New-Object System.Text.UTF8Encoding($false) + [System.IO.File]::AppendAllText($GitHubOutputPath, "$Name=$Value$([System.Environment]::NewLine)", $encoding) +} + +function Format-TableCell { + <# + .SYNOPSIS + Renders comment text as a single-line markdown code span, truncated to + -MaxLength characters. + #> + param( + [Parameter(Mandatory = $true)] + [AllowEmptyString()] + [string]$Value, + [int]$MaxLength = 90 + ) + + $collapsed = ($Value -replace '\s+', ' ').Trim() + if ([string]::IsNullOrEmpty($collapsed)) { + return '' + } + if ($collapsed.Length -gt $MaxLength) { + $collapsed = $collapsed.Substring(0, $MaxLength).TrimEnd() + '...' + } + + # GitHub splits a table row on every unescaped pipe, inside a code span as much + # as outside one. A fence has to outnumber any backtick it wraps. + $escaped = $collapsed -replace '\|', '\|' + $fence = if ($escaped.Contains('`')) { '``' } else { '`' } + if ($escaped.StartsWith('`') -or $escaped.EndsWith('`')) { + $escaped = " $escaped " + } + + return "$fence$escaped$fence" +} + +function Format-FileLink { + param( + [Parameter(Mandatory = $true)] + [string]$Path, + [Parameter(Mandatory = $true)] + [int]$Line + ) + + $label = "${Path}:$Line" + if ([string]::IsNullOrWhiteSpace($Repository) -or [string]::IsNullOrWhiteSpace($Sha)) { + return "``$label``" + } + + return "[$label]($ServerUrl/$Repository/blob/$Sha/$Path#L$Line)" +} + +function Get-RunReference { + if ([string]::IsNullOrWhiteSpace($Repository) -or [string]::IsNullOrWhiteSpace($RunId)) { + return 'this check''s log' + } + + return "[this check's log]($ServerUrl/$Repository/actions/runs/$RunId)" +} + +if (-not (Test-Path -LiteralPath $ReportPath)) { + throw "Comment-hygiene report not found: $ReportPath" +} + +$report = Get-Content -LiteralPath $ReportPath -Raw | ConvertFrom-Json +foreach ($field in @('base', 'violationCount', 'violations')) { + if ($null -eq $report.PSObject.Properties[$field]) { + throw "Comment-hygiene report is missing the '$field' field: $ReportPath" + } +} + +$violationCount = [int]$report.violationCount +$baseLabel = [string]$report.base +$hasViolations = $violationCount -gt 0 + +if ($hasViolations) { + $listed = @(@($report.violations) | Select-Object -First $MaxListed) + $commentLines = @( + '### Comment hygiene (advisory)' + '' + "$violationCount comment-style violation(s) in the lines this branch adds since ``$baseLabel``." + 'Advisory only -- no check fails on these, and the same violations appear as inline warnings on the Files changed tab.' + '' + '| File | Category | Comment |' + '| --- | --- | --- |' + ) + + foreach ($violation in $listed) { + $commentLines += ('| {0} | `{1}` | {2} |' -f + (Format-FileLink -Path ([string]$violation.file) -Line ([int]$violation.line)), + ([string]$violation.category), + (Format-TableCell -Value ([string]$violation.text))) + } + + $commentLines += '' + if ($violationCount -gt $listed.Count) { + $commentLines += "$($violationCount - $listed.Count) more not listed here -- see $(Get-RunReference)." + $commentLines += '' + } + + $commentLines += "Fix them per ``$SkillPath``." + $commentLines += 'Running `.\build.ps1 -CommentHygiene` (or `.\test.ps1 -CommentHygiene`) enforces them locally, and' + $commentLines += 're-wraps over-wide lines and repairs non-ASCII punctuation as it goes.' +} +else { + $commentLines = @( + '### Comment hygiene (advisory)' + '' + "No comment-style violations in the lines this branch adds since ``$baseLabel``." + ) +} + +$commentDirectory = Split-Path -Path $CommentPath -Parent +if (-not [string]::IsNullOrWhiteSpace($commentDirectory) -and -not (Test-Path -LiteralPath $commentDirectory)) { + New-Item -ItemType Directory -Path $commentDirectory -Force | Out-Null +} + +$utf8NoBom = New-Object System.Text.UTF8Encoding($false) +[System.IO.File]::WriteAllText($CommentPath, ($commentLines -join [System.Environment]::NewLine), $utf8NoBom) + +$resolvedCommentPath = [System.IO.Path]::GetFullPath($CommentPath) +Write-GitHubOutputValue -Name 'comment_path' -Value $resolvedCommentPath +Write-GitHubOutputValue -Name 'has_violations' -Value ($hasViolations.ToString().ToLowerInvariant()) +Write-GitHubOutputValue -Name 'violation_count' -Value ([string]$violationCount) + +Write-Output ([pscustomobject]@{ + CommentPath = $resolvedCommentPath + HasViolations = $hasViolations + ViolationCount = $violationCount +}) diff --git a/Build/Agent/CommentHygiene.psm1 b/Build/Agent/CommentHygiene.psm1 index f06f3aeb2f..e9b502c85a 100644 --- a/Build/Agent/CommentHygiene.psm1 +++ b/Build/Agent/CommentHygiene.psm1 @@ -77,7 +77,7 @@ function Get-CommentHygieneEditorConfig { whatever .editorconfig already declares for every file, so the two can never drift apart. Only the [*] section is read, since that is where this repo declares both values. Results are cached per root -- this runs - once per gate invocation, not once per file. + once per scan, not once per file. .OUTPUTS A hashtable with MaxLineLength and TabWidth. Falls back to 98 and 4 when @@ -560,7 +560,7 @@ function Get-CommentLineClassification { else { # Inlined from Get-CommentBody: this loop runs every line of every diffed file on # every build, where a per-line function call plus its own redundant Trim measurably - # slows the gate down. + # slows the scan down. if ($trimmed.StartsWith('///')) { $kinds[$i] = 'exempt' $bodies[$i] = $trimmed.Substring(3) @@ -633,7 +633,7 @@ function Get-CommentHygieneViolations { $allowedLines = $LineFilter[$file] } - # File.ReadAllLines, not Get-Content: ~50x faster on a large file, and this gate runs + # File.ReadAllLines, not Get-Content: ~50x faster on a large file, and this scan runs # every build. Still BOM-safe: StreamReader sniffs a real BOM even given an explicit # encoding. $lines = [System.IO.File]::ReadAllLines($file, [System.Text.Encoding]::UTF8) diff --git a/Build/Agent/Test-BuildCommentHygieneComment.ps1 b/Build/Agent/Test-BuildCommentHygieneComment.ps1 new file mode 100644 index 0000000000..02aef3f8a8 --- /dev/null +++ b/Build/Agent/Test-BuildCommentHygieneComment.ps1 @@ -0,0 +1,136 @@ +<# +.SYNOPSIS + Smoke test for Build-CommentHygieneComment.ps1. Run directly under both + PowerShell 7 and Windows PowerShell 5.1. +#> +[CmdletBinding()] +param() + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +$scriptPath = Join-Path $PSScriptRoot 'Build-CommentHygieneComment.ps1' +$workspace = Join-Path ([System.IO.Path]::GetTempPath()) ('BuildCommentHygieneCommentTest-' + [guid]::NewGuid().ToString('N')) + +function Assert-True { + param( + [Parameter(Mandatory = $true)] + [bool]$Condition, + [Parameter(Mandatory = $true)] + [string]$Message + ) + + if (-not $Condition) { + throw $Message + } +} + +function New-Report { + param( + [Parameter(Mandatory = $true)] + [string]$Path, + [Parameter(Mandatory = $true)] + [AllowEmptyCollection()] + [object[]]$Violations + ) + + $report = [ordered]@{ + base = 'origin/main' + advisory = $true + violationCount = $Violations.Count + violations = @($Violations) + } + + $directory = Split-Path -Path $Path -Parent + if (-not (Test-Path -LiteralPath $directory)) { + New-Item -ItemType Directory -Path $directory -Force | Out-Null + } + + $encoding = New-Object System.Text.UTF8Encoding($false) + [System.IO.File]::WriteAllText($Path, ($report | ConvertTo-Json -Depth 5), $encoding) +} + +try { + $violationsDirectory = Join-Path $workspace 'with-violations' + $cleanDirectory = Join-Path $workspace 'clean' + + # A pipe and a backtick in the flagged text: both would break the table row or + # its code span if the renderer passed them through. + $awkward = 'Chooses `Name` when the flag is set | otherwise the abbreviation' + $violations = @( + [ordered]@{ file = 'Src/xWorks/RecordEditView.cs'; line = 42; category = 'comment-too-long'; text = $awkward } + [ordered]@{ file = 'Build/Agent/Sample.ps1'; line = 7; category = 'doc-pointer'; text = 'See the design note' } + [ordered]@{ file = 'Src/xWorks/xWorks.csproj'; line = 3; category = 'xml-illegal-double-hyphen'; text = 'Keeps the pack step honest' } + ) + + $violationsReportPath = Join-Path $violationsDirectory 'report.json' + $violationsCommentPath = Join-Path $violationsDirectory 'comment.md' + $violationsOutputPath = Join-Path $violationsDirectory 'github-output.txt' + New-Report -Path $violationsReportPath -Violations $violations + + $violationsResult = & $scriptPath -ReportPath $violationsReportPath -CommentPath $violationsCommentPath ` + -GitHubOutputPath $violationsOutputPath -MaxListed 2 -Repository 'sillsdev/FieldWorks' ` + -RunId '123456' -ServerUrl 'https://github.com' -Sha '0123456789abcdef0123456789abcdef01234567' + + Assert-True (Test-Path -LiteralPath $violationsCommentPath) 'Expected the helper to write the comment markdown file.' + Assert-True ($violationsResult.HasViolations -eq $true) 'Expected the helper to report violations.' + Assert-True ($violationsResult.ViolationCount -eq 3) 'Expected the helper to report every violation in the count.' + + $comment = Get-Content -LiteralPath $violationsCommentPath -Raw + Assert-True ($comment.Contains('3 comment-style violation(s) in the lines this branch adds since `origin/main`.')) ` + 'Expected the comment to open with the violation count and the base ref.' + Assert-True ($comment.Contains('[Src/xWorks/RecordEditView.cs:42](https://github.com/sillsdev/FieldWorks/blob/0123456789abcdef0123456789abcdef01234567/Src/xWorks/RecordEditView.cs#L42)')) ` + 'Expected each row to link the file and line at the head commit.' + Assert-True ($comment.Contains('otherwise the abbreviation')) 'Expected the flagged comment text in the row.' + Assert-True (-not ($comment -match '(?m)^\| \[Src/xWorks/xWorks\.csproj')) 'Expected -MaxListed to cap the table rows.' + Assert-True ($comment.Contains('1 more not listed here -- see [this check''s log](https://github.com/sillsdev/FieldWorks/actions/runs/123456).')) ` + 'Expected the capped remainder to point at the run log.' + + # Every table row has to keep exactly the four pipes its three columns need, so + # a pipe inside the flagged text stays escaped rather than splitting the row. + foreach ($line in ($comment -split '\r?\n')) { + if (-not $line.StartsWith('| [')) { continue } + $barePipes = ([regex]::Matches($line, '(?$null + # Reads local origin/HEAD, not `git remote show origin`, which hits the network + # every run. --quiet keeps git's stderr out of PowerShell's error stream, where + # it terminates instead of falling through. + $originHead = git rev-parse --verify --quiet --abbrev-ref origin/HEAD if ($LASTEXITCODE -eq 0 -and -not [string]::IsNullOrWhiteSpace($originHead)) { return $originHead.Trim() } return 'origin/main' } @@ -162,20 +170,74 @@ function Write-Violation { } } +function Write-ScanReport { + <# + .SYNOPSIS + Writes the scan result to -ReportPath as JSON. Does nothing when the + caller asked for no report. + + .PARAMETER Base + The ref the scan diffed against, recorded verbatim in the report. + #> + param( + [object[]] $Violations, + [string] $Base + ) + + if ([string]::IsNullOrWhiteSpace($ReportPath)) { return } + + # PowerShell's current location and .NET's working directory can differ; New-Item + # reads the first and WriteAllText the second, so anchor a relative path once. + $path = $ReportPath + if (-not [System.IO.Path]::IsPathRooted($path)) { + $path = Join-Path (Get-Location).Path $path + } + + $directory = Split-Path -Path $path -Parent + if (-not [string]::IsNullOrWhiteSpace($directory) -and -not (Test-Path -LiteralPath $directory)) { + New-Item -ItemType Directory -Path $directory -Force | Out-Null + } + + $records = New-Object System.Collections.ArrayList + foreach ($v in $Violations) { + [void]$records.Add([ordered]@{ + file = ($v.File.Substring($repoRoot.Length + 1) -replace '\\', '/') + line = $v.Line + category = $v.Category + text = $v.Text + }) + } + + $report = [ordered]@{ + base = $Base + advisory = [bool]$Advisory + violationCount = $records.Count + violations = @($records.ToArray()) + } + + # UTF8Encoding($false): a byte-order mark ahead of the opening brace makes the file + # unparsable to a plain JSON reader. + [System.IO.File]::WriteAllText($path, ($report | ConvertTo-Json -Depth 5), + (New-Object System.Text.UTF8Encoding($false))) + Write-Host "comment-hygiene: wrote $($records.Count) violation record(s) to $path" +} + if ($Full) { $files = git ls-files $scopedGlobs | ForEach-Object { ConvertTo-RepoPath $_ } | Where-Object { -not (Test-ExcludedPath $_) } $violations = Get-CommentHygieneViolations -Files $files Write-Host "comment-hygiene -Full: $($violations.Count) violation(s) across $($files.Count) file(s)" foreach ($v in $violations) { Write-Violation $v } + Write-ScanReport -Violations $violations -Base 'HEAD' exit 0 } -# The build and test CI steps both invoke this gate over the same tree, which would -# annotate every violation twice. The first run marks the job so the rest skip. +# Several steps invoke this script over one tree, which would annotate every violation +# once per step. The first run marks the job, and presetting the marker suppresses +# reporting for a whole job. if ($env:GITHUB_ACTIONS -eq 'true') { if ($env:FW_COMMENT_HYGIENE_REPORTED -eq '1') { - Write-Host 'comment-hygiene: already reported earlier in this job.' + Write-Host 'comment-hygiene: reporting suppressed for this job.' exit 0 } if (-not [string]::IsNullOrWhiteSpace($env:GITHUB_ENV)) { @@ -192,6 +254,7 @@ Write-Host "comment-hygiene: scanning lines added since $base" $lineFilter = Get-AddedLineFilter -Base $base if ($lineFilter.Count -eq 0) { Write-Host 'comment-hygiene: no added lines in scope to check.' + Write-ScanReport -Violations @() -Base $base exit 0 } @@ -265,6 +328,7 @@ foreach ($group in ($pending | Group-Object File)) { } $violations = $remainingViolations.ToArray() +Write-ScanReport -Violations $violations -Base $base if ($fixedCount -gt 0 -or $wrappedCount -gt 0) { Write-Host ("comment-hygiene: auto-fixed {0} punctuation and re-wrapped {1} over-long comment line(s) in {2} file(s) (review and include in your commit)." -f $fixedCount, $wrappedCount, $fixedFiles.Keys.Count) -ForegroundColor Yellow diff --git a/Build/Agent/powershell-compat.ps1 b/Build/Agent/powershell-compat.ps1 index 97e51f1423..dac023085e 100644 --- a/Build/Agent/powershell-compat.ps1 +++ b/Build/Agent/powershell-compat.ps1 @@ -1,10 +1,11 @@ <# .SYNOPSIS - Static PowerShell-version-compatibility gate for the comment-hygiene tooling. + Static compatibility check for PowerShell that must run under both 5.1 and 7. .DESCRIPTION - Two independent static layers, neither of which requires more than one - PowerShell engine to actually be installed: + Applies to any script this repo ships to developers or CI. Two independent + static layers, neither of which requires more than one PowerShell engine to + actually be installed: 1. A dependency-free regex scan for known gotchas where 5.1 and 7 both parse the same text successfully but disagree on its meaning, so no @@ -47,14 +48,11 @@ $ErrorActionPreference = 'Stop' Import-Module (Join-Path $PSScriptRoot 'CommentHygiene.psm1') -Force $repoRoot = (Resolve-Path (Join-Path $PSScriptRoot '../..')).Path -$targetFiles = @( - 'Build/Agent/CommentHygiene.psm1', - 'Build/Agent/CommentHygiene.Tests.ps1', - 'Build/Agent/comment-hygiene.ps1', - 'Build/Agent/comment-hygiene-repair.ps1', - 'Build/Agent/comment-hygiene-blame.ps1', - 'Build/Agent/powershell-compat.ps1' -) | ForEach-Object { Join-Path $repoRoot $_ } | Where-Object { Test-Path -LiteralPath $_ } +# Every PowerShell file under Build/Agent, so a new script is covered the day it +# lands rather than when someone remembers to name it here. +$targetFiles = @(Get-ChildItem -LiteralPath $PSScriptRoot -Recurse -File | + Where-Object { $_.Extension -eq '.ps1' -or $_.Extension -eq '.psm1' } | + ForEach-Object { $_.FullName } | Sort-Object) $violations = New-Object System.Collections.ArrayList diff --git a/build.ps1 b/build.ps1 index 2907c3b50e..abb7b75fe9 100644 --- a/build.ps1 +++ b/build.ps1 @@ -119,9 +119,8 @@ Defaults to the FW_BUILD_STARTED_BY environment variable when set, otherwise 'unknown'. .PARAMETER CommentHygiene - Enforce the comment-hygiene gate, failing the build on any violation in the lines this - branch adds. Required of coding agents; a developer build leaves it off and never runs - the gate. CI reports violations as warning annotations either way. + Enforce the comment-hygiene check, failing the build on any violation in the lines this + branch adds. .PARAMETER SkipWorktreeLock Internal switch used when build.ps1 is invoked from test.ps1 while the parent test workflow diff --git a/test.ps1 b/test.ps1 index 28126417e0..ac38280659 100644 --- a/test.ps1 +++ b/test.ps1 @@ -37,9 +37,8 @@ Equivalent environment variable: FW_TEST_ALLOW_ASSERT_DIALOGS=1. .PARAMETER CommentHygiene - Enforce the comment-hygiene gate, failing the run on any violation in the lines this - branch adds. Required of coding agents; a developer run leaves it off and never runs - the gate. + Enforce the comment-hygiene check, failing the run on any violation in the lines this + branch adds. .PARAMETER StartedBy Optional actor label written to worktree lock metadata (for example: user or agent). From 67bd09a84bf2e244b66de4f1a2a8f4ca2b18bd33 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Tue, 18 Aug 2026 08:39:19 -0700 Subject: [PATCH 2/4] chore: add a temporary comment-hygiene probe Four deliberate violations in one throwaway script, so the advisory comment on this pull request has something to show. Reverted once the comment has been reviewed. The subject is kept short on purpose: gitlint caps a title at 72 characters, and the revert of this commit inherits it inside Revert "...". Co-Authored-By: Claude Opus 5 --- Build/Agent/CommentHygieneProbe.ps1 | 30 +++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 Build/Agent/CommentHygieneProbe.ps1 diff --git a/Build/Agent/CommentHygieneProbe.ps1 b/Build/Agent/CommentHygieneProbe.ps1 new file mode 100644 index 0000000000..983dadd767 --- /dev/null +++ b/Build/Agent/CommentHygieneProbe.ps1 @@ -0,0 +1,30 @@ +<# +.SYNOPSIS + Temporary probe. Every comment below trips comment-hygiene on purpose, so the + advisory pull request comment has something to report. +#> + +Set-StrictMode -Version Latest + +# Phase 3 note: chooses `Name` when the flag is set | otherwise the abbreviation +function Test-ProbeProcessFraming { + return $true +} + +# design-notes.md D1: a doc pointer carrying a finding code, both of which are flagged +function Test-ProbeDocPointer { + return $true +} + +# This comment runs well past the two hundred character budget that an implementation +# comment gets in straight-line code, which is the whole point of it being here. It +# keeps talking long after it has stopped saying anything a reader would need, so the +# scan reports the whole run as one comment-too-long violation. +function Test-ProbeTooLong { + return $true +} + +# This one line is deliberately wider than the ninety-eight display columns that .editorconfig declares for every file. +function Test-ProbeLineTooLong { + return $true +} From 39c2f11d0724e9ac27f826a21b4d033b28efafe0 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Tue, 18 Aug 2026 08:52:40 -0700 Subject: [PATCH 3/4] Revert "chore: add a temporary comment-hygiene probe" This reverts commit 67bd09a84bf2e244b66de4f1a2a8f4ca2b18bd33. Co-Authored-By: Claude Opus 5 --- Build/Agent/CommentHygieneProbe.ps1 | 30 ----------------------------- 1 file changed, 30 deletions(-) delete mode 100644 Build/Agent/CommentHygieneProbe.ps1 diff --git a/Build/Agent/CommentHygieneProbe.ps1 b/Build/Agent/CommentHygieneProbe.ps1 deleted file mode 100644 index 983dadd767..0000000000 --- a/Build/Agent/CommentHygieneProbe.ps1 +++ /dev/null @@ -1,30 +0,0 @@ -<# -.SYNOPSIS - Temporary probe. Every comment below trips comment-hygiene on purpose, so the - advisory pull request comment has something to report. -#> - -Set-StrictMode -Version Latest - -# Phase 3 note: chooses `Name` when the flag is set | otherwise the abbreviation -function Test-ProbeProcessFraming { - return $true -} - -# design-notes.md D1: a doc pointer carrying a finding code, both of which are flagged -function Test-ProbeDocPointer { - return $true -} - -# This comment runs well past the two hundred character budget that an implementation -# comment gets in straight-line code, which is the whole point of it being here. It -# keeps talking long after it has stopped saying anything a reader would need, so the -# scan reports the whole run as one comment-too-long violation. -function Test-ProbeTooLong { - return $true -} - -# This one line is deliberately wider than the ninety-eight display columns that .editorconfig declares for every file. -function Test-ProbeLineTooLong { - return $true -} From 3abeba994758566b0b3489c6391c9b73fc455f20 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Tue, 18 Aug 2026 09:07:51 -0700 Subject: [PATCH 4/4] refactor: drop the CI auto-run and its annotation dedup marker comment-hygiene.ps1 no longer auto-runs from build.ps1 and test.ps1 in CI, so CommentHygiene.yml is the only thing that scans and annotates. With nothing running the scan twice, FW_COMMENT_HYGIENE_REPORTED and its GITHUB_ENV write go with it. build.ps1 and test.ps1 invoke the scan only for -CommentHygiene, and the installer workflows stop scanning on push. Co-Authored-By: Claude Opus 5 --- .github/workflows/CI.yml | 8 -------- Build/Agent/comment-hygiene.ps1 | 19 +------------------ build.ps1 | 9 +++------ test.ps1 | 9 +++------ 4 files changed, 7 insertions(+), 38 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 030f3edd7b..26c4b91960 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -19,14 +19,6 @@ jobs: debug_build_and_test: name: Build Debug and run tests runs-on: windows-2022 - env: - # CommentHygiene.yml owns comment-hygiene reporting, so the build and test steps - # below stay quiet rather than annotating every violation a second time. Clearing - # this makes comment-hygiene.ps1 diff against origin/ again, which - # needs fetch-depth: 0 on the checkout below or it fails with "bad revision". Set - # unconditionally: a job-level value overrides one a step writes to GITHUB_ENV, so - # anything conditional here would also clobber that in-job marker. - FW_COMMENT_HYGIENE_REPORTED: 1 steps: - name: Checkout Files uses: actions/checkout@v7 diff --git a/Build/Agent/comment-hygiene.ps1 b/Build/Agent/comment-hygiene.ps1 index 7887e49ec0..c8fc378733 100644 --- a/Build/Agent/comment-hygiene.ps1 +++ b/Build/Agent/comment-hygiene.ps1 @@ -24,8 +24,7 @@ .PARAMETER Advisory Report violations and exit 0 instead of failing. Under GitHub Actions each violation is also emitted as a warning annotation, so it lands on the pull - request's diff without breaking the build. build.ps1 and test.ps1 pass this - whenever -CommentHygiene was not requested. + request's diff without breaking the build. .PARAMETER ReportPath Write the scan result to this path as JSON: the base ref, the advisory @@ -232,22 +231,6 @@ if ($Full) { exit 0 } -# Several steps invoke this script over one tree, which would annotate every violation -# once per step. The first run marks the job, and presetting the marker suppresses -# reporting for a whole job. -if ($env:GITHUB_ACTIONS -eq 'true') { - if ($env:FW_COMMENT_HYGIENE_REPORTED -eq '1') { - Write-Host 'comment-hygiene: reporting suppressed for this job.' - exit 0 - } - if (-not [string]::IsNullOrWhiteSpace($env:GITHUB_ENV)) { - # UTF8Encoding($false): appending a BOM mid-file would corrupt the - # environment file the runner parses when the step ends. - [System.IO.File]::AppendAllText($env:GITHUB_ENV, "FW_COMMENT_HYGIENE_REPORTED=1`n", - (New-Object System.Text.UTF8Encoding($false))) - } -} - $base = Resolve-BaseRef -Explicit $BaseRef Write-Host "comment-hygiene: scanning lines added since $base" diff --git a/build.ps1 b/build.ps1 index abb7b75fe9..3ff6eaa985 100644 --- a/build.ps1 +++ b/build.ps1 @@ -209,13 +209,10 @@ if (-not $runningOnWindows) { exit 1 } -# Comment hygiene is opt-in: with -CommentHygiene it blocks the build, in CI it -# only annotates the pull request, and an ordinary developer build is silent. -$commentHygieneInCI = ($env:GITHUB_ACTIONS -eq 'true') -or ($env:CI -eq 'true') -if ($CommentHygiene -or $commentHygieneInCI) { +if ($CommentHygiene) { $commentHygienePath = Join-Path $PSScriptRoot "Build/Agent/comment-hygiene.ps1" - & $commentHygienePath -Advisory:(-not $CommentHygiene) - if ($CommentHygiene -and $LASTEXITCODE -ne 0) { + & $commentHygienePath + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } } diff --git a/test.ps1 b/test.ps1 index ac38280659..10f99419ef 100644 --- a/test.ps1 +++ b/test.ps1 @@ -111,13 +111,10 @@ param( $ErrorActionPreference = 'Stop' -# Comment hygiene is opt-in: with -CommentHygiene it blocks the run, in CI it -# only annotates the pull request, and an ordinary developer run is silent. -$commentHygieneInCI = ($env:GITHUB_ACTIONS -eq 'true') -or ($env:CI -eq 'true') -if ($CommentHygiene -or $commentHygieneInCI) { +if ($CommentHygiene) { $commentHygienePath = Join-Path $PSScriptRoot "Build/Agent/comment-hygiene.ps1" - & $commentHygienePath -Advisory:(-not $CommentHygiene) - if ($CommentHygiene -and $LASTEXITCODE -ne 0) { + & $commentHygienePath + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } }