Skip to content

docs(lefthook-powershell): correct the PSScriptAnalyzer failure mechanism - #628

Merged
kyle-sexton merged 3 commits into
mainfrom
fix/598-pssa-race-docs
Sep 28, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
fix/598-pssa-race-docs

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Closes #598

Summary

The lefthook-powershell runner, worker, both READMEs and the test comment said PSScriptAnalyzer 1.25.0 leaks engine state between targets and that process isolation fixes it. dotfiles#774 showed the failure on one file in a fresh process. The cause is the CommandInfo race between rules running in parallel (PowerShell/PSScriptAnalyzer#1867, #1708).

Fix

No behavior change. No retry was added, per the maintainer decision on #598.

Verification

  • pwsh -NoProfile -File components/lefthook-powershell/psscriptanalyzer-staged.test.ps1: "PSScriptAnalyzer staged adapter regressions passed."
  • markdownlint-cli2 on both READMEs: 0 issues. Pre-commit hooks green.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01LCEBhuXxZAT8K619tC6T5f

…nism

The runner header, worker header, READMEs and test comment described a
cross-target engine-state leak that process isolation fixes. The failure is
a CommandInfo race between rules running in parallel inside one
Invoke-ScriptAnalyzer call, and it reproduces on one file in a fresh
process. Name the real cause, state that the per-target worker only keeps
one failure from masking others, and document the triggers to remove until
PowerShell/PSScriptAnalyzer#2206 ships.

Closes #598

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCEBhuXxZAT8K619tC6T5f
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-27T22:33:44.152835Z e3d847b PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 32s —— View job


Security review complete

  • Gather context (PR diff, changed files)
  • Run /review:security-review criteria over the diff
  • Post findings (or clear result)

No security issues found.

This PR is documentation-only: it edits PowerShell comment-based help blocks, a test-file comment, and two README.md files to correct the description of a PSScriptAnalyzer failure mechanism (a CommandInfo race, per PowerShell/PSScriptAnalyzer#1867 and #1708) instead of the previously-claimed engine-state leak. No executable logic changed in any of the five files:

  • components/lefthook-powershell/README.md — prose only
  • components/lefthook-powershell/psscriptanalyzer-staged.ps1 — comment-based help only (lines inside the <# ... #> block)
  • components/lefthook-powershell/psscriptanalyzer-staged.test.ps1 — a # comment only, test assertions unchanged
  • components/lefthook-powershell/psscriptanalyzer-target.ps1 — comment-based help only
  • components/psscriptanalyzer/README.md — prose only

No injection surfaces, authorization logic, secrets/token handling, or GitHub Actions workflow changes are touched by this diff, so there's no security-relevant surface to review here.

@claude

claude Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 37s —— View job


Code review — PR #628

  • Gather context (PR diff, changed files)
  • Understand the request (/review:code-review lane)
  • Review changed files for correctness/maintainability
  • Post inline findings
  • Post summary

This is a docs/comment-only correction (no .ps1 logic changed), replacing the old "process isolation prevents the race" claim with the more accurate CommandInfo-race explanation across the runner header, worker header, both READMEs, and the test comment. The prose is internally consistent and the new footnote links ([3]-[5] in components/lefthook-powershell/README.md) are correctly numbered and non-conflicting.

One finding posted inline on components/lefthook-powershell/README.md (lines 24-26): the new guidance instructs removing Export-ModuleMember from a .psm1 that exports every function it defines, as a race trigger to eliminate until PowerShell/PSScriptAnalyzer#2206 ships. But components/lefthook-powershell/fixtures/get-command/CommandLookup.psm1 does exactly that, and it's part of the six-file set the PR's own cited regression (psscriptanalyzer-staged.test.ps1) repeatedly runs through real PSScriptAnalyzer 1.25.0 as verification. The doc's own prescription isn't followed by the fixture it ships alongside — worth resolving one way or the other (fix the fixture or scope the guidance).

Author-claimed verification (test run, markdownlint, pre-commit) was not independently re-run by this review; it's reported as-is from the PR description, not confirmed.

No security, architecture, or test-coverage concerns beyond the above — this is a low-risk, well-scoped documentation correction otherwise.

Branch: fix/598-pssa-race-docs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e3d847be9b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread components/lefthook-powershell/README.md Outdated
Comment thread components/lefthook-powershell/README.md Outdated
…rop it from the fixture

The README advice now covers aliases and says what a module with private
members does instead. The get-command regression fixture exported its one
function explicitly, which is the race trigger the README tells consumers to
remove; without the call the module exports the same function.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LCEBhuXxZAT8K619tC6T5f
@claude

claude Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 39s —— View job


Security review complete

  • Gather context (PR diff at HEAD da3d686, changed files)
  • Run /review:security-review criteria over the full diff
  • Post findings (or clear result)

No security issues found.

This PR is documentation/comment-only plus one test-fixture tweak:

  • components/lefthook-powershell/README.md, components/psscriptanalyzer/README.md — prose corrections describing the PSScriptAnalyzer CommandInfo race instead of the previously-claimed engine-state leak.
  • components/lefthook-powershell/psscriptanalyzer-staged.ps1, psscriptanalyzer-target.ps1 — comment-based help text only (inside <# ... #> blocks); no code paths changed.
  • components/lefthook-powershell/psscriptanalyzer-staged.test.ps1 — a # comment update only; test assertions unchanged.
  • components/lefthook-powershell/fixtures/get-command/CommandLookup.psm1 — drops the Export-ModuleMember -Function Get-ExternalToolPath call. This fixture is only ever dot-loaded by its own colocated Pester spec (CommandLookup.Tests.ps1) and by the psscriptanalyzer-staged.test.ps1 regression as an analyzer target — it is never imported into another module's session state or exposed as a public interface, so exporting all its members instead of one has no authorization or trust-boundary effect.

No injection surfaces, authorization logic, secrets/token handling, or GitHub Actions workflow changes are touched by this diff, so there's no security-relevant surface to review here.
· Branch: fix/598-pssa-race-docs

@claude

claude Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 21s —— View job


Code review — PR #628

  • Gather context (diff, changed files, CLAUDE.md)
  • Check prior review findings (fixture Export-ModuleMember issue) against latest commit
  • Review changed files for correctness/maintainability
  • Post inline findings (none — see below)
  • Post summary

Reviewed da3d686 against origin/main (/review:code-review criteria). This is a documentation/comment-only correction — no .ps1/.psm1 executable logic changed except removing one Export-ModuleMember call from a fixture.

Both previously-open review threads are resolved by the latest commit (da3d686, "scope the Export-ModuleMember advice and drop it from the fixture"):

  • The Codex P2 finding (alias-export filtering) is addressed: the guidance in components/lefthook-powershell/README.md#L24-28 now scopes eligibility to a module that "exports every function and alias it defines" and explicitly calls out that a module hiding private members keeps Export-ModuleMember or moves its export list into a manifest.
  • The earlier finding that fixtures/get-command/CommandLookup.psm1 contradicted its own doc's prescription is fixed: the Export-ModuleMember -Function Get-ExternalToolPath call is now removed (CommandLookup.psm1), and it defines only that one function with no aliases, so it's now a clean match for the "exports every function it defines" case. Confirmed CommandLookup.Tests.ps1 doesn't depend on the removed call (it just calls Get-ExternalToolPath after Import-Module, which still exports it by default).

Verification performed this pass:

  • git diff origin/main...HEAD reviewed in full across all 6 changed files.
  • Repo-wide grep for Export-ModuleMember confirms no other fixture/target file under components/lefthook-powershell/ or components/psscriptanalyzer/ still calls it in a way that contradicts the new guidance (the one remaining hit in psscriptanalyzer-staged.test.ps1:84 is inside a single-quoted heredoc string content for a synthetic fake module, not code PSScriptAnalyzer parses as a call in that file).
  • Cross-checked the PSUseCorrectCasing-disabled claim against PSScriptAnalyzerSettings.psd1 (the rule key is absent, matching both the README/worker-header text and the test's own assertion at psscriptanalyzer-staged.test.ps1:122-124).
  • Confirmed the six-file count assertion (historicalHookFiles.Count -eq 6) matches: settings file (1) + 3 fixture files + adapter (1) + test file itself (1).
  • Grepped for stale "engine state"/"cross-target" leak language repo-wide outside the changed files — none found; the correction is complete across the docs it touches.

Author-claimed verification (the pwsh regression run, markdownlint-cli2, pre-commit hooks) was not independently re-run in this pass — reported as-is from the PR description, not confirmed by this review.

No new correctness, maintainability, or documentation-accuracy issues found. Nothing to flag inline.
· Branch: fix/598-pssa-race-docs

kyle-sexton added a commit that referenced this pull request Sep 28, 2026
…ockstep-drift (#634)

Closes #633

## Summary

`pr-convention-lockstep` failed on every open PR with `drift: hook
validator: KEYWORD_ERE / NO_ISSUE_ERE declarations not found`.
melodic-software/claude-code-plugins#4636 rewrote the hook validator
around a per-line `scan_linkage` and renamed its closing pattern to
`CLOSING_ERE`, so `parseValidatorPatterns` could not find it. The
validator and the ci-workflows `pr-contract` composite still agree, so
this was a parse failure, not drift.

## Fix

- `parseValidatorPatterns` reads `CLOSING_ERE` (plus the unchanged
`NO_ISSUE_ERE`) and translates `[[:blank:]]` as well as `[[:space:]]`.
- The new EREs are lowercase with no `i` flag, matched against
`${line,,}`. The parser now asserts that lowercasing is present and the
probes lowercase their input, the same way the composite check already
asserts `lower = tolower(line)`.
- The test fixture takes the new validator shape; a new test fails when
the validator stops lowercasing.
- `policy.json` is unchanged. Every closing keyword and no-issue marker
it lists is still accepted by both the composite (`run.sh` `scan_line`
and the `no-issue` test) and the validator. The new `Refs:` / `Relates
to:` acceptance and negated-closer rejection are enforced by both, but
`policy.json` has no field for them and `pr-convention-policy.mjs` and
the pinned reusables do not implement them, so recording them would be a
separate change.

## Verification

- `npm run test:pr-convention-lockstep`: 40/40 pass.
- `npm run test:pr-convention-policy`: 11/11 pass.
- `npm run lint:pr-convention-lockstep` against live sources with a
token: `pr-convention lockstep: all copies and consumer pins match
policy.json`, exit 0.
- `biome check` on both touched files: clean.

## Related

- melodic-software/claude-code-plugins#4636 (the validator rewrite)
- melodic-software/ci-workflows#544 (the `pr-contract` negation and
`Refs:` change the validator mirrors)
- Unblocks #628, #630, #631, #632

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01LCEBhuXxZAT8K619tC6T5f

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 33s —— View job


Security review complete

  • Gather context (PR diff, origin/main...HEAD at HEAD fcac817)
  • Invoke /review:security-review and follow its instructions
  • Post findings (or clear result)

No security issues found.

fcac817 is a no-op merge of main into this branch — git diff origin/main...HEAD --stat shows the same six files as the prior review pass at da3d686, with no new content. This PR remains documentation/comment-only:

  • components/lefthook-powershell/README.md, components/psscriptanalyzer/README.md — prose corrections describing the PSScriptAnalyzer CommandInfo race instead of the previously-claimed engine-state leak.
  • components/lefthook-powershell/psscriptanalyzer-staged.ps1, psscriptanalyzer-target.ps1 — comment-based help text only (inside <# ... #> blocks); no executable code paths changed.
  • components/lefthook-powershell/psscriptanalyzer-staged.test.ps1 — a # comment update only; test assertions unchanged.
  • components/lefthook-powershell/fixtures/get-command/CommandLookup.psm1 — drops an Export-ModuleMember -Function Get-ExternalToolPath call. This fixture is only ever dot-loaded by its own colocated Pester spec and by the psscriptanalyzer-staged.test.ps1 regression as an analyzer target — never imported into another module's session state or exposed as a public interface — so broadening its export set has no authorization or trust-boundary effect.

No injection surfaces, authorization logic, secrets/token handling, or GitHub Actions workflow changes are touched by this diff, so there's no security-relevant surface to review here. This matches the two prior clean passes on this branch (e3d847b, da3d686); the skip-gate condition that a head with an already-applicable successful review needs no re-review holds, and this pass independently confirms the diff hasn't changed.

Branch: fix/598-pssa-race-docs

@kyle-sexton
kyle-sexton merged commit f129fe3 into main Sep 28, 2026
49 checks passed
@kyle-sexton
kyle-sexton deleted the fix/598-pssa-race-docs branch September 28, 2026 13:42
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.

lefthook-powershell: PSScriptAnalyzer NullReferenceException is an in-process rule race, not cross-target state

1 participant