feat(scripts): add lint:py:fix to apply ruff autofixes across Python skills#1450
feat(scripts): add lint:py:fix to apply ruff autofixes across Python skills#1450
Conversation
…skills Closes #886 - add Invoke-PythonLintFix.ps1 to discover Python skills via pyproject.toml and run ruff check --fix per skill - add npm run lint:py:fix script wrapping the runner - add Pester unit tests covering parameter validation, tool availability, skill discovery, lint execution, and output persistence 🐍 - Generated by Copilot
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1450 +/- ##
==========================================
- Coverage 87.70% 86.83% -0.87%
==========================================
Files 65 63 -2
Lines 10198 9550 -648
==========================================
- Hits 8944 8293 -651
- Misses 1254 1257 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Advisory review, this PR is from a maintainer. Findings are informational only.
Overview
This PR adds npm run lint:py:fix backed by a new Invoke-PythonLintFix.ps1 script and a comprehensive Pester test suite (23 tests covering parameter validation, tool availability, skill discovery, lint execution paths, and output persistence). The implementation is clean, well-structured, and consistent with the existing Invoke-PythonLint.ps1 pattern.
✅ Issue Alignment
Issue #886 requires four items; all are confirmed satisfied:
| Acceptance Criterion | Status |
|---|---|
npm run lint:py |
✅ Pre-existing in package.json |
npm run lint:py:fix |
✅ Added by this PR |
npm run test:py |
✅ Pre-existing in package.json |
lint:all includes lint:py |
✅ Confirmed in package.json |
The intentional exclusion of lint:py:fix from lint:all (due to file mutation) is well-documented in the PR description. This is a reasonable design decision.
✅ PR Template Compliance
- Description: clear and complete
- Related issue:
Closes #886✅ - Type of Change checkboxes: correctly checked (New feature, Linting configuration, Script/automation) ✅
- Testing: thorough — Pester unit tests,
lint:psstatic analysis, and smoke test documented ✅ - Security considerations: all checked and appropriate ✅
Minor note: Several Required Automated Checks are unchecked (markdown linting, spell checking, frontmatter, etc.). For a pure PowerShell + package.json change these are arguably N/A, but marking them explicitly as N/A (or checking them if they were run and passed) in the checklist would help reviewers confirm their scope was considered.
🔍 Coding Standards
Two advisory findings are posted as inline comments:
-
Missing
[OutputType([hashtable])]onInvoke-PythonLintFix(line 26) — perPSUseOutputTypeCorrectlyrule in the PowerShell instructions. The same omission exists in the existingInvoke-PythonLint.ps1, so both could be aligned in a follow-up. -
ruff check . --fixvsruff format .(line 86) — the issue's example specifiedruff format(formatting), while the implementation appliesruff check --fix(lint rule autofixes). These are distinct operations; clarifying the scope or combining both would strengthen the command.
✅ Code Quality
- Skill discovery (
pyproject.tomlfind +node_modulesexclusion): correct ✅ - Ruff resolution chain (venv → global fallback): matches
Invoke-PythonLint.ps1✅ Push-Location/Pop-Locationpairing withtry/finally: correct ✅$LASTEXITCODEcapture for external process exit: correct (unaffected by$ErrorActionPreference = 'Stop') ✅- JSON output persistence: consistent with sibling scripts ✅
- Test coverage: comprehensive, uses established Pester patterns for this repo ✅
No security concerns identified.
katriendg
left a comment
katriendg
left a comment
There was a problem hiding this comment.
Approving, two small comments by AW reviewer worth checking.
There was a problem hiding this comment.
Advisory review, this PR is from a maintainer. Findings are informational only.
Review Summary
Overall the PR is clean, well-tested, and consistent with the existing Invoke-PythonLint.ps1 sibling. The implementation is correct and the 23 Pester tests give good coverage of the meaningful behavioral paths. Two inline comments flag areas worth addressing; additional notes follow.
Issue Alignment
✅ Issue #886 is legitimately closeable. Reviewing the base package.json:
lint:pyalready exists and is already wired intolint:alltest:pyalready exists- This PR supplies the missing
lint:py:fixentry
All seven acceptance criteria from #886 are now met.
i️ Informational — ruff check --fix vs ruff format
Issue #886 used ruff format in its example snippet for lint:py:fix. The implementation instead uses ruff check . --fix, which auto-corrects lint-rule violations; ruff format applies code formatting (PEP 8 / Black-style). These are complementary but distinct operations. The current choice is reasonable for a lint-fix command and the PR description describes it accurately. It may be worth adding a one-liner to the Additional Notes in the description (or a # See also: ruff format comment in the script) to help contributors who expected formatter-style output.
PR Template Compliance
✅ Description, issue reference, and type-of-change checkboxes are all correctly filled.
The remaining unchecked automated checks (lint:md, lint:frontmatter, validate:skills, lint:md-links, plugin:generate, docs:test) are all legitimately N/A for a pure .ps1 / package.json change. It would be helpful to annotate each unchecked item with (N/A — no markdown/frontmatter/skill/plugin changes) per the template guidance, but this is minor.
spell-check is technically applicable to English text in PS comments and is not marked N/A — low risk but worth a quick run.
Coding Standards
✅ Copyright header, SPDX identifier, #Requires -Version 7.0, [CmdletBinding()], $ErrorActionPreference = 'Stop', #region/#endregion, and the $MyInvocation.InvocationName -ne '.' invocation guard are all present and correct.
The script intentionally mirrors the sibling's header style (inline # comments rather than a <# .SYNOPSIS #> block). PSScriptAnalyzer passes, confirming the rule configuration accepts this pattern.
Test Quality
✅ 23 tests across parameter validation, tool availability, skill discovery, ruff execution, and output persistence. Exception handling and the node_modules exclusion are explicitly covered.
i️ Test naming convention — Per pester.instructions.md, the repo standard is given_context_when_action_then_expected (e.g., given_ruff_missing_when_fixing_then_returns_failure). Current names use plain English like 'Returns failure when ruff not available'. This matches the sibling Invoke-PythonLint.Tests.ps1 exactly, so it is a pre-existing pattern. Worth standardizing in a future pass when both test files are touched.
Action Items (informational)
- Duplication — See inline comment at line 25 of
Invoke-PythonLintFix.ps1. Extracting aPythonLintHelpers.psm1module would prevent the two scripts from diverging silently over time. - OutputPath parent-dir guard — See inline comment at line 126. The same gap exists in
Invoke-PythonLint.ps1and would be cleanly resolved alongside the shared-module extraction above. ruff formatcoverage — Consider whether a futurelint:py:format/lint:py:format:checkentry is desired to give contributors Black-style formatting viaruff format.
|
Pushed a revision addressing all five review threads — ready for re-review:
Validation locally on
|
…ormat in lint:py:fix - extract Get-PythonSkill / Resolve-RuffCommand / Write-PythonLintResults into scripts/linting/Modules/PythonLintHelpers.psm1 - add [OutputType([hashtable])] and Purpose/Author header to Invoke-PythonLint(Fix).ps1 - chain ruff check --fix then ruff format . in Invoke-PythonLintFix.ps1, capture both exit codes - guard results-output parent directory before Out-File - cover format chain, formatExitCode propagation, and PythonLintHelpers module in scripts/tests/linting/ 🐍 - Generated by Copilot
There was a problem hiding this comment.
Advisory review — this PR is from a maintainer. Findings are informational only.
Review Summary
This is a well-structured PR. The shared module extraction into PythonLintHelpers.psm1 is the right call, the test coverage across PythonLintHelpers.Tests.ps1 and Invoke-PythonLintFix.Tests.ps1 is thorough, and the dual ruff check --fix + ruff format chain correctly satisfies the acceptance criterion from #886. With lint:all already including lint:py and test:py already present, this PR cleanly closes the issue. Two small convention items are noted inline.
Issue Alignment ✅
All acceptance criteria from #886 are now satisfied:
lint:py— already present and now delegates to the refactored shared modulelint:py:fix— added by this PR ✅test:py— already presentlint:allincludeslint:py— already in place (line 27,package.json)- Both
fixExitCodeandformatExitCodeare surfaced per-skill ✅ - Skill discovery is
pyproject.toml-driven (no hardcoded paths) ✅
PR Template Compliance ⚠️
Two minor template items to be aware of:
-
Custom "Refactor" checkbox — The template's "Type of Change" section does not include a "Refactor" option. The description text adequately covers the shared-module extraction, so this checkbox is informational noise. Consider using "Other (please describe)" for items not in the template, or simply relying on the description.
-
"Linting configuration" checkbox — The PR adds linting runner scripts (
.ps1), not linting configuration files (e.g.,.markdownlint.json,PSScriptAnalyzer.psd1). The "Script/automation (.ps1, .sh, .py)" checkbox already accurately describes the changes; "Linting configuration" doesn't apply here.
Coding Standards ✅ (with notes)
Inline comments are raised for:
- Missing try-catch in main execution guard (
Invoke-PythonLintFix.ps1, line 137) — informational; same gap exists in the siblingInvoke-PythonLint.ps1. - Missing
#Requires -Version 7.0in module (PythonLintHelpers.psm1, line 9) — informational; consistent with the existing gap inLintingHelpers.psm1andFrontmatterValidation.psm1.
Code Quality ✅
Resolve-RuffCommandcorrectly short-circuits: Linux.venv/bin/ruff→ Windows.venv/Scripts/ruff.exe→ global fallback →$null. No issues.Write-PythonLintResultscorrectly guards the parent directory before writing and returns the resolved path. No issues.- The
ruff formatstep is unconditional afterruff check --fix, which is correct — formatting should run regardless of whether--fixmade changes. $global:LASTEXITCODEset in the ruff mock is the right pattern for Pester testing of external-process exit codes.
No security concerns, logic errors, or breaking changes found.
There was a problem hiding this comment.
Advisory review — this PR is from a maintainer. Findings are informational only.
Overview
This PR cleanly delivers the missing lint:py:fix entry that completes issue #886. The shared-module extraction into PythonLintHelpers.psm1 is the right architectural move, the dual ruff check --fix + ruff format chain is correct, and the 23 Pester tests provide solid coverage of the meaningful behavioral paths. Two inline comments flag new items not raised in prior rounds; the two open threads from the previous review remain unresolved.
Issue Alignment ✅
All seven acceptance criteria from #886 are now satisfied:
| Criterion | Status |
|---|---|
npm run lint:py |
✅ Pre-existing |
npm run lint:py:fix |
✅ Added by this PR |
npm run test:py |
✅ Pre-existing |
lint:all includes lint:py |
✅ Confirmed |
Both fixExitCode and formatExitCode surfaced per-skill |
✅ |
pyproject.toml-driven discovery (no hardcoded paths) |
✅ |
| Adding a new skill is auto-discovered | ✅ |
PR Template Compliance ⚠️
Two minor inaccuracies in the Type of Change section:
-
"Refactor" checkbox — this option does not exist in the template. The shared-module extraction is an implementation detail adequately described in the prose Description. Checking a non-existent option can confuse automation that parses checkbox state. "Other (please describe)" or the existing "Script/automation" checkbox would be more accurate.
-
"Linting configuration" checkbox — the template intends this for configuration files (
.markdownlint.json,PSScriptAnalyzer.psd1, etc.), not linting runner scripts. The "Script/automation (.ps1,.sh,.py)" checkbox already covers the new.ps1files accurately.
Several Required Automated Checks are unchecked without an N/A annotation (lint:md, spell-check, lint:frontmatter, validate:skills, lint:md-links, plugin:generate, docs:test). For a pure .ps1 + package.json change these are legitimately out of scope, but annotating them as (N/A — no markdown/frontmatter/skill/plugin changes) per the template guidance makes reviewer intent explicit. Of the unchecked items, spell-check is technically in-scope for English text in PowerShell comments and is worth a quick pass.
Open Threads from Prior Review
Two inline threads from the previous automated review (run 25033819647) remain unresolved in the current head commit:
- Missing
#Requires -Version 7.0inPythonLintHelpers.psm1— per PowerShell module conventions, the#Requiresstatement belongs after the purpose/author comment block (line 9 area). Open thread:scripts/linting/Modules/PythonLintHelpers.psm1 #discussion_r3151589208. - Missing try-catch in main execution guard (
Invoke-PythonLintFix.ps1, line 137) — ifInvoke-PythonLintFixthrows an unhandled terminating error the script exits with a raw exception trace rather than a cleanexit 1. Open thread:#discussion_r3151589203.
Both are low-risk for this PR's intended use case; bindsi's approval reflects that judgment. They are worth tracking for a follow-up that aligns all linting runners.
New Findings (inline comments)
-
PythonLintHelpers.psm1line 9 —CIHelpers.psm1imported but unused. None of the three exported functions callWrite-CIAnnotation,Set-CIOutput,Write-CIStepSummary, orSet-CIEnv. The import adds a hard relative-path dependency with no current benefit. If CI annotation support is planned for a future pass, a forward-intent comment clarifies the decision; otherwise the import can be removed. -
Invoke-PythonLintFix.ps1line 117 —errorsarray has mixed entry formats. The no-ruff path pushes a bare path string; the exception-catch path pushes a formatted string"$skillPath - error: $_". Callers iterating$result.errorsexpecting consistent bare paths will encounter a mixed-type array. Prefer a bare path inerrorsand surface the exception detail in theoutputfield or a dedicated key. The corresponding test assertion (Should -Not -BeNullOrEmpty) does not catch this regression; tightening it toShould -Contain $script:SkillDirwould.
Code Quality ✅
Resolve-RuffCommandshort-circuit order (Linux.venv→ Windows.venv→ global →$null) is correct and consistent withInvoke-PythonLint.ps1.Write-PythonLintResultscorrectly guards the parent directory and returns the resolved path.ruff formatis invoked unconditionally afterruff check --fix, which is correct — formatting should apply regardless of whether--fixmade changes.$global:LASTEXITCODEin the Pester ruff mock is the right pattern for testing external-process exit codes.- No security concerns, logic errors, or breaking changes identified.
…nLint - add -Fix switch and remove Invoke-PythonLintFix.ps1 plus its tests - extract shared logic into PythonLintHelpers.psm1 with #Requires 7.0 - wrap main execution in try-catch with Write-CIAnnotation - update package.json lint:py:fix to call main script with -Fix ♻️ - Generated by Copilot
There was a problem hiding this comment.
Advisory review, this PR is from a maintainer. Findings are informational only.
Review Summary
This PR cleanly extends the existing Invoke-PythonLint.ps1 runner with a -Fix switch, extracts three shared helpers into a new PythonLintHelpers.psm1 module, and ships a dedicated test file for those helpers. The implementation is well-structured and the refactor reduces duplication without changing the behavior of the existing lint:py path.
Three advisory observations are posted as inline comments; none are blocking.
Issue Alignment
✅ Closes #886. The key acceptance criteria are met:
npm run lint:py:fixappliesruff check --fix+ruff formatacross all Python skills.lint:allalready includeslint:py(confirmed in the currentpackage.json);lint:py:fixis intentionally excluded because it mutates source files — the PR description explains this clearly.- New Python skills with
pyproject.tomlare auto-discovered throughGet-PythonSkillwithout any script changes required.
PR Template Compliance
✅ Description, related issue, type of change, testing, and security sections are all populated.
🔍 Minor description inaccuracy: The Description section and "Review Comment Resolution" item 1 both refer to Invoke-PythonLintFix.ps1 as a separate file, but the actual implementation adds a -Fix switch to the existing Invoke-PythonLint.ps1 — no separate Invoke-PythonLintFix.ps1 was created. The package.json script confirms this (pwsh -NoProfile -File ./scripts/linting/Invoke-PythonLint.ps1 -Fix ...). The description could be updated to accurately reflect the implementation choice.
The unchecked automated checks (lint:md, lint:frontmatter, spell-check, etc.) are reasonable N/A for a PowerShell-only change with no markdown or frontmatter files modified.
Coding Standards
✅ Copyright headers, #Requires -Version 7.0, [CmdletBinding()], [OutputType()], comment-based help, Export-ModuleMember, and the if ($MyInvocation.InvocationName -ne '.') guard are all present and correct.
Three minor observations posted as inline comments:
Write-CIAnnotationimplicit transitive dependency (Invoke-PythonLint.ps1:195) —CIHelpersshould be imported directly in the script rather than relying on the nested import insidePythonLintHelpers.psm1.Out-FilevsSet-Content(PythonLintHelpers.psm1:135) —Set-Content -Encoding UTF8is the conventional pattern in this codebase.- Lost debug output (
PythonLintHelpers.psm1:78) — The original code printedUsing venv ruffwhen selecting the.venvbinary; that observability was dropped in the refactor.
Code Quality
✅ The dual-path if ($Fix) inside the foreach loop is verbose but readable. The alternative — a separate inner function per mode — would add indirection without significant benefit at this scale.
✅ The Excludes node_modules from discovery test in Invoke-PythonLint.Tests.ps1 is correctly updated to a pass-through mock (Mock Get-PythonSkill { @() }) with a comment explaining that the filter logic is now tested in PythonLintHelpers.Tests.ps1. This is the right approach after the extraction.
✅ PythonLintHelpers.Tests.ps1 uses real filesystem fixtures (New-Item, Set-Content) rather than mocks for Get-PythonSkill tests — appropriate since the function's value is precisely its file-system scanning behavior.
No security vulnerabilities or logic errors identified.
There was a problem hiding this comment.
Advisory review, this PR is from a maintainer. Findings are informational only.
Overview
This PR cleanly adds npm run lint:py:fix by extending Invoke-PythonLint.ps1 with a -Fix switch and extracting shared discovery/resolution/output logic into a new PythonLintHelpers.psm1 module. The implementation is well-structured, the Pester tests are comprehensive (both the new PythonLintHelpers.Tests.ps1 and the updated runner tests), and the PR description is thorough. No security issues or logic bugs were found.
1. Issue Alignment ✅
Closes #886 is accurate. lint:py and test:py were already present; lint:all already includes lint:py; and lint:py:fix is correctly excluded from lint:all since it mutates source files — all aligning with the issue's acceptance criteria.
2. PR Template Compliance ⚠️
The Required Automated Checks section lists eight commands that "must pass before merging." Only npm run lint:ps is checked. The following remain unchecked:
npm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run validate:skillsnpm run lint:md-linksnpm run plugin:generatenpm run docs:test
For a purely PowerShell/JSON change, most of these will be green without any findings, but the template requires them to be run and checked before merge. Please run the remaining commands and tick the boxes (or annotate with N/A for those that genuinely do not apply).
3. Coding Standards ✅
- Copyright headers,
#Requires -Version 7.0, shebang placement,[CmdletBinding()],[OutputType()], andExport-ModuleMemberare all correct in the new module. - Function naming (
Get-PythonSkill,Resolve-RuffCommand,Write-PythonLintResults) uses approved verbs and followsVerb-NounPascalCase. - Comment-based help is present on all three exported functions.
- The double-quote style in
PythonLintHelpers.psm1'sImport-Modulematches the pre-existingLintingHelpers.psm1sibling — consistent within the module tier.
One structural deviation was flagged inline (see comment on line 29): CIHelpers.psm1 is loaded transitively rather than explicitly, which diverges from the pattern every other linting runner follows.
4. Code Quality ✅
No bugs, logic errors, or security concerns. The -Fix mode correctly chains ruff check --fix followed by ruff format, captures both exit codes, and surfaces them in the result hashtable — matching the issue's acceptance criterion. The Invoke-PythonLint.Tests.ps1 now covers the fix-mode path with four dedicated It blocks. The separate PythonLintHelpers.Tests.ps1 exercises Get-PythonSkill (including the node_modules exclusion at the real implementation level), Resolve-RuffCommand for all four resolution paths, and Write-PythonLintResults for default and explicit output paths with parent-directory creation.
Action Items
- Required: Run and check the remaining automated check items in the PR checklist (or annotate
N/Awhere genuinely inapplicable). - Suggested: Add an explicit
Import-ModuleforCIHelpers.psm1alongside thePythonLintHelpers.psm1import inInvoke-PythonLint.ps1(see inline comment).
4 participants
Description
Adds an
npm run lint:py:fixscript that wraps a newscripts/linting/Invoke-PythonLintFix.ps1runner. The script discovers every Python skill (any directory containingpyproject.toml, excludingnode_modules) and runs bothruff check . --fixandruff format .against each one, falling back from the local.venvruff to a global ruff when needed. Both ruff exit codes are captured and the combined output is written tologs/python-lint-fix-results.json.The new script is intentionally not wired into
npm run lint:all, since it mutates source files. It is opt-in for contributors who want to apply ruff autofixes across all Python skills in one command.Related Issue(s)
Closes #886
Type of Change
Code & Documentation:
lint:py)Infrastructure & Configuration:
Other:
.ps1,.sh,.py)Testing
Validation performed locally on
feat/886-python-lint-fix:npm run test:ps -- -TestPath "scripts/tests/linting/"— all green, including newPythonLintHelpers.Tests.ps1(11 tests coveringGet-PythonSkill,Resolve-RuffCommand, andWrite-PythonLintResults).npm run lint:ps— clean for the new module and runners.npm run lint:py:fix— exit 0, valid JSON written tologs/python-lint-fix-results.jsonwith bothfixExitCodeandformatExitCodeper skill.Test coverage includes parameter validation, ruff tool availability and
.venvfallback behavior, skill discovery (pyproject.tomlinclusion +node_modulesexclusion),ruff formatchaining and exit-code propagation, parent-directory creation before writing results, and output JSON persistence.Review Comment Resolution
This revision addresses all five review threads on the PR:
[OutputType([hashtable])]on runners — added to bothInvoke-PythonLint.ps1andInvoke-PythonLintFix.ps1along with# Purpose:/# Author:headers.ruff formatafterruff check --fix—Invoke-PythonLintFix.ps1now invokes both subcommands per skill and surfacesfixExitCodeandformatExitCodein the result hashtable.Get-PythonSkill,Resolve-RuffCommand, andWrite-PythonLintResultsintoscripts/linting/Modules/PythonLintHelpers.psm1, consumed by both runners.Out-File—Write-PythonLintResultscreates the output's parent directory when missing.Checklist
Required Checks
Invoke-*.ps1runners; shared module underscripts/linting/Modules/; mirrored*.Tests.ps1underscripts/tests/linting/).lint:pybehavior unchanged).Required Automated Checks
npm run lint:psnpm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run validate:skillsnpm run lint:md-linksnpm run plugin:generatenpm run docs:testSecurity Considerations
ruffalready required by Python skills.pyproject.toml, invokesruffwith--fixandformat, and writes a single JSON log file underlogs/.Additional Notes
lint:py:fixis intentionally excluded fromlint:allbecause it modifies source files. It is meant to be invoked explicitly by contributors..venvruff and falls back to a globally availableruffonly when no.venvis present, matching the resolution logic used bylint:py.