Skip to content

ci: closing-keyword gate, informational wire-exemption verdict, AGENTS.md - #176

Merged
juemerson-at-purestorage merged 10 commits into
dmann000:mainfrom
juemerson-at-purestorage:feat/ci-pr-gates
Sep 28, 2026
Merged

juemerson-at-purestorage merged 10 commits into
dmann000:mainfrom
juemerson-at-purestorage:feat/ci-pr-gates

Conversation

@juemerson-at-purestorage

Copy link
Copy Markdown
Collaborator

Summary

Two PR-level checks and a starting-point doc. None of it touches the module:

  1. tools/Test-PfbWireExemption.ps1: decides whether a diff can change what the module sends or parses.
    • It classifies every changed line in Public/, Private/, the .psd1 and the .psm1, on both sides of the diff, with the PowerShell tokeniser.
    • It exits 0 exempt, 1 not exempt, 2 undecided, and emits one verdict object (Decision, Files, Basis, Reason) that a caller can read instead of parsing text.
    • -BaseRef and -HeadRef let it judge any pair of revisions without a checkout.
  2. verify-wire-exemption.yml runs that classifier on every PR and writes the verdict to the job summary. Informational only: it never fails on the verdict, only if the classifier itself returns nothing.
  3. tools/Test-PfbClosingKeywords.ps1 finds issue references that a closing keyword does not actually close. In Fixes #1, #2, GitHub closes Add demote support to Update-PfbFileSystem (-DiscardNonSnapshottedData, -RequestedPromotionState) #1 only. For each finding it prints the corrected form.
  4. verify-closing-keywords.yml runs it over the PR body and every commit message on the branch, and fails the PR on a finding.
  5. AGENTS.md: a tool-neutral entry point that points at the three CLAUDE.md files and lists what CI checks on a pull request.

The two new test files are registered in Tests/coverage-baseline.psd1:

  • RequiredDescribes gains the new Describes. The closing-keyword ones are required on both editions; the wire-exemption ones on pwsh 7 only, since they are PS7-gated.
  • Test-PfbWireExemption.Tests.ps1 is pinned at 41 skips on 5.1.

Design notes

  • Why the wire-exemption job is informational. Its verdict decides whether a change needs verification against a real array, and that decision belongs to the maintainer. A red check on every module PR would be noise. A summary line that can be read, or recomputed, is not.
  • The verdict is recomputable. The classifier runs on any two revisions and returns a single object. Anything downstream that acts on the verdict can recompute it from the PR's own base and head, instead of trusting text someone pasted.
  • Why the job runs the base branch's copy. A PR could otherwise edit the classifier to call itself exempt. The workflow extracts tools/Test-PfbWireExemption.ps1 from the base revision into a temp folder and runs that.
  • This PR's own run is a special case. The base has no copy yet, so on this PR the job skips with a ::notice.
  • What the classifier treats as undecided. An unplanned git failure partway through is reported as Undecided (exit 2) with a Reason, never as a plain exit 1. That way "not exempt" and "the tool broke" never look the same.
  • Renames are not special. A move is diffed as a delete plus an add (--no-renames), so moving a cmdlet out of Public/ still counts as an in-scope change.
  • Which commits the closing-keyword job reads. It reads git rev-list --no-merges <head> --not <merge-base> origin/<base>, so commits that are already on the base branch, such as a merge from main into the branch, are not re-judged.
  • It checks each keyword's own reference. It does not check whether a referenced issue exists or is the intended one, and it does not read the PR title or comments.
  • The two scripts must agree. Test-PfbClosingKeywords.ps1 is a line-for-line port of an existing JavaScript hook, and the two must give identical results.
    • Its letter matching is spelled out as explicit ASCII classes rather than IgnoreCase. .NET's case-insensitive matching on PowerShell 7 folds the Kelvin sign (U+212A) into K, which JavaScript and Windows PowerShell 5.1 do not.
    • A test pins this.
  • The incident this guards against is PR fix: five wire-contract defects in the syslog, replication, quota, object-store and SMTP cmdlets #108's body, Fixes #100, #101, #103, #87, and #80, which closed only All three group-quota write cmdlets fail on the wire: New-PfbQuotaGroup puts identity in the body, Update-/Remove-PfbQuotaGroup send an illegal names+file_system_names combination (user siblings fixed in #8) #100.

Verification

Wire-exempt: The diff leaves the module source and the manifest entirely untouched, so nothing here can alter a request the module sends or a response it parses.

Expected on this PR's first run:

  • Verify Wire Exemption skips with a ::notice, because the base has no classifier yet.
  • Verify Closing Keywords passes on this body and these commits. The incident text above is in backticks, which the checker ignores, so it is not read as a real reference.

Both scripts were run locally on this branch:

  • The classifier reports Exempt, exit 0.
  • The closing-keyword checker reports nothing on this body, on every commit message, or on the PR template.

actionlint 1.7.12 reports 0 findings on both new workflows. actions/checkout is pinned to the same v7.0.1 commit as every other workflow.

Tests

The scoped run covered every test file this branch touches, plus CiCoverageGate.Tests.ps1, under both editions (Pester 6.0.1):

Edition Passed Failed Skipped
pwsh 7 132 0 0
Windows PowerShell 5.1 91 0 41
  • The 41 skips on 5.1 are Test-PfbWireExemption.Tests.ps1. It is gated wholesale because the classifier is PowerShell 7-only tooling.
  • The closing-keyword tests run on both editions; the checker itself has no #Requires.
  • Several cases are negative-control pairs, each checked against a mutated copy of the code: a rename out of scope, a #Requires edit, a git failure, and the Kelvin-sign fold.
  • PSScriptAnalyzer: 0 Error, 0 Warning. Derived artifacts: all 11 up to date.

🤖 Generated with Claude Code

…to Pester

Adds the rename, #Requires-edited, '#'-in-here-string and root-module cases, and commits here-string setup on main so the '<#' case can fail for the right reason.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rdict object

Moved from local tooling. Adds -HeadRef, returns one Decision/Files/Basis object with exit codes unchanged, and documents how a gate must recompute the verdict from origin/main rather than trust a pull_request run.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…d tighten test coverage

A git failure after revision resolution now yields one Undecided verdict with a path-free Reason and exit 2, instead of an uncaught error that exits 1 and reads as NotExempt. Adds tests for that path, the no-in-scope Exempt object, an inert file record and a base-side-only first executable line, and rewords comments that pointed at material outside the repo.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
test(coverage): pin Test-PfbWireExemption.Tests.ps1 at 40 winps51 skips.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Single implementation of the per-issue closing-keyword rule, transliterated from the local hook with JavaScript's \b, \s and $ semantics spelled out so the two agree; parity checked on every case.

Case is spelled out as explicit [Xx] classes rather than IgnoreCase: on pwsh 7 .NET IgnoreCase folds the Kelvin sign into K, so a keyword right after one was missed there while Windows PowerShell 5.1 and JavaScript both find it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The wire-exemption workflow was written before main required every remote action to be SHA-pinned; pin its checkout to the same v7.0.1 commit the other workflows use. Also word its no-base-copy notice so it does not assume this is the PR that adds the classifier.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The closing-keyword Describes and its workflow's Describe are ungated, so both editions require them; the wire-exemption Describes are PS7-gated, so only pwsh 7 does. The header count is recomputed from git ls-files: 24 of 219 files skip on 5.1.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…eletion

With git's rename detection on, --name-status reports only the new path of a rename, so a cmdlet moved from Public/ to an out-of-scope folder was never seen and the diff read as exempt. Pass --no-renames so a move arrives as a delete plus an add; the rename branch is now unreachable and is removed. A new case moves a cmdlet out of Public/ and expects exit 1 (it fails with the flag removed).

test(coverage): Test-PfbWireExemption.Tests.ps1 now skips 41 on 5.1 (total 550).

docs: AGENTS.md says the local scripts run under PowerShell 7.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@juemerson-at-purestorage
juemerson-at-purestorage merged commit 02b5ba4 into dmann000:main Sep 28, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant