Skip to content

fix: CRLF-safe typos directive, non-silent analyzer hook, accurate config pointers - #77

Merged
kyle-sexton merged 8 commits into
mainfrom
fix/config-hygiene-crlf-analyzer-pointers
Jul 8, 2026
Merged

fix: CRLF-safe typos directive, non-silent analyzer hook, accurate config pointers#77
kyle-sexton merged 8 commits into
mainfrom
fix/config-hygiene-crlf-analyzer-pointers

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Config-hygiene fixes surfaced by a downstream code-review pass (kyle-sexton/provisioning). All are in the Track-B source configs; consumers pick them up via the sync manifest (_typos.toml, .editorconfig, .shellcheckrc), except the lefthook runner which is manually adopted.

Substantive (silent-wrong-behavior bugs)

  • modules/typos/_typos.toml — the spellchecker:ignore-next-line directive regex used a bare \n, so on a CRLF file the \r was never matched and the pragma silently suppressed nothing (siblings are CRLF-aware via (?Rm)/(?s)). Added \r?. Verified with typos on a CRLF fixture: the next line is now suppressed; an unprotected line still flags.
  • modules/lefthook/psscriptanalyzer-staged.ps1ErrorAction = 'SilentlyContinue' swallowed every non-terminating analyzer error, not just the one benign PSUseCompatibleSyntax NRE its comment justified. A malformed PSScriptAnalyzerSettings.psd1 (or a rule that failed to load) would make the hook report clean while running unruled. Now: validate the settings parse (fail loud otherwise), capture errors via -ErrorVariable, drop only the benign Object reference not set NRE, and re-surface anything else. Logic verified locally (parse-guard, error triage, loop accumulation); PSScriptAnalyzer clean.

Cosmetic (pointer/header accuracy so copies read right downstream)

  • .editorconfig — header claimed only Markdown/shell/PowerShell+batch, but the file also carries JSON/YAML/TOML, JS/TS, git-config, and lockfile sections; broadened. Named Biome's config home as the standards repo's modules/typescript so a consumer's copy doesn't read it as a dangling local path.
  • .shellcheckrc (root + modules/shellcheck) — qualified "its canonical home is modules/shellcheck/" as the standards repo's path, same reason.

Verified: PSScriptAnalyzer 0, editorconfig-checker clean, _typos.toml valid TOML, lefthook pre-commit green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JLR9bgyuhEX6YUUkAhkhy5


Note

Medium Risk
Changes pre-commit PowerShell lint behavior (stricter failures on bad settings or analyzer errors); typos and comment edits are low impact but the hook affects every staged PS commit using Lefthook.

Overview
Fixes silent-wrong behavior in synced Track-B configs and the Lefthook PowerShell lane.

modules/typos/_typos.toml — the spellchecker:ignore-next-line ignore regex now uses \r?\n so CRLF files actually honor the pragma (a bare \n left the next line unprotected).

modules/lefthook/psscriptanalyzer-staged.ps1 — pre-commit no longer runs with SilentlyContinue on the whole analyzer pass. It parses PSScriptAnalyzerSettings.psd1 up front and exits on failure, excludes PSUseCompatibleSyntax in this fast lane (CI still enforces it), and fails on any captured analyzer/engine errors instead of reporting clean while unruled.

.editorconfig and .shellcheckrc (root + modules/shellcheck) — comment-only updates: broader file-class coverage in the header and explicit “standards repo” paths so synced copies don’t read like broken local references.

Reviewed by Cursor Bugbot for commit 36313ec. Bugbot is set up for automated code reviews on this repo. Configure here.

kyle-sexton and others added 2 commits July 8, 2026 12:37
…rors

Two adopted configs silently did the wrong thing:

- modules/typos/_typos.toml: the `spellchecker:ignore-next-line` directive regex
  used a bare `\n`, so on a CRLF-terminated file the `\r` before the newline was
  never matched and the pragma silently suppressed nothing (its siblings are
  CRLF-aware via (?Rm)/(?s)). Add `\r?` so it works on both line endings.
  Verified with typos against a CRLF fixture: the next line is now suppressed,
  an unprotected line still flags.
- modules/lefthook/psscriptanalyzer-staged.ps1: `ErrorAction = 'SilentlyContinue'`
  swallowed EVERY non-terminating analyzer error, not just the one benign
  PSUseCompatibleSyntax NRE its comment justified - so a malformed
  PSScriptAnalyzerSettings.psd1 (or a rule that failed to load) would make the
  hook report clean while running unruled. Now: validate the settings file parses
  (fail loud if not), capture errors via -ErrorVariable, drop only the benign
  "Object reference not set" NRE, and re-surface anything else as a failure.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JLR9bgyuhEX6YUUkAhkhy5
Cosmetic comment fixes so the adopted copies read correctly downstream:

- .editorconfig: the header claimed to cover only "Markdown, shell, PowerShell"
  plus batch, but the file also carries config/serialization (JSON/YAML/TOML),
  JS/TS, git config, and lockfile sections; broaden the header to match. Name
  the Biome config's home explicitly as the standards repo's modules/typescript
  so a consumer's copied file does not read it as a dangling local path.
- .shellcheckrc (root + modules/shellcheck): likewise qualify "its canonical
  home is modules/shellcheck/" as the standards repo's path, so the pointer
  doesn't dangle once copied into a consumer with no modules/ tree.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JLR9bgyuhEX6YUUkAhkhy5
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

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


PR Review

  • Read CLAUDE.md / REVIEW.md for repo guidelines
  • Analyze PR diff
  • Review modules/typos/_typos.toml — CRLF-safe ignore directive
  • Review modules/lefthook/psscriptanalyzer-staged.ps1 — silent error fix
  • Review .editorconfig — header accuracy
  • Review .shellcheckrc / modules/shellcheck/.shellcheckrc — pointer accuracy
  • Post findings

Overall this is a well-reasoned fix PR. The substantive changes address real bugs with correct logic; the cosmetic changes improve accuracy for downstream consumers. A couple of minor observations below.


modules/typos/_typos.toml — CRLF fix

Line 17Correct. Adding \r? is the right fix. The sibling disable-line directive uses (?Rm) where multiline $ matches before \r\n in PCRE2, so it was already CRLF-tolerant. The (?s) block directives match . across \r and \n, so also fine. Only the two-line ignore-next-line pattern was vulnerable — it had to cross the literal \n and a bare \n doesn't match \r\n. The explanation comment is accurate.


modules/lefthook/psscriptanalyzer-staged.ps1 — error handling

Settings parse guard (lines 35–43): Correct approach. Import-PowerShellDataFile -ErrorAction Stop gives you a hard failure on a malformed .psd1 rather than the previous silent miss. The try/catch + exit 1 path and the error message are clear.

Error accumulation pattern (lines 50–54): The foreach statement + -ErrorVariable err + immediate AddRange is correct. With the bare name form (-ErrorVariable err, no + prefix), PowerShell replaces $err on each cmdlet call rather than appending. The if ($err) { $saErrors.AddRange(@($err)) } inside the loop body correctly harvests each call's errors before the next call resets the variable.

Minor comment accuracy nit (line 49): The comment says "a foreach statement (not ForEach-Object) keeps them in this scope so they accumulate." The accumulation is really from the manual AddRange call; foreach matters here because it keeps $err in the current scope (so it's readable immediately after each Invoke-ScriptAnalyzer call), not because of any automatic accumulation. This is a subtle distinction but the comment could mislead a future reader into thinking the foreach itself is the accumulation mechanism.

Error triage (line 59):

$realErrors = @($saErrors | Where-Object { $_.Exception.Message -notmatch 'Object reference not set' })

The filter works correctly for the stated intent, but it's slightly broader than needed. Any NullReferenceException from any analyzer rule (not just PSUseCompatibleSyntax) would be silently dropped. In practice this is very low risk — NREs from other rules would indicate a broken rule that should surface — but the filter is wider than the justification. A more targeted guard, if this ever matters, would also check the origin (e.g., the rule name or call stack), though that requires knowing what PSScriptAnalyzer surfaces in OriginInfo. Flagging for awareness rather than blocking merge.

View this line →

Write-Output for error messages (lines 39, 61): Using Write-Output rather than Write-Error is intentional and correct here — Lefthook captures stdout from the hook command and shows it to the user, so writing to stdout is the right channel. No concern.


.editorconfig — header accuracy

Lines 1–11 — The expanded header now accurately enumerates all sections in the file (JSON/YAML/TOML, JS/TS, git config, lockfiles were present but unmentioned before). The Biome path clarification (modules/typescriptthe standards repo under modules/typescript) is the right fix; without it a consumer's copy reads as a dangling local path.


.shellcheckrc / modules/shellcheck/.shellcheckrc — pointer accuracy

Lines 1–5 — The same "standards repo's" qualification applied here for the same reason. Both files are identical (same SHA), which is appropriate since they're supposed to be synced copies.


Summary

Area Finding Severity
_typos.toml CRLF fix Correct; comment accurate
.psd1 parse guard Correct; improves failure visibility
Error accumulation Correct; minor comment imprecision Nit
NRE filter breadth Slightly wider than justified; low risk Minor
.editorconfig header Accurate; Biome path clarified
.shellcheckrc pointers Accurate; same-SHA sync correct

No blocking issues. The two minor points (comment imprecision on accumulation, NRE filter breadth) are worth considering but don't change the correctness of the behavior.

Comment thread modules/lefthook/psscriptanalyzer-staged.ps1
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

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


PR Review

  • Read CLAUDE.md / REVIEW.md — none found
  • Read the changed PS1 file and verify Cursor Bugbot's finding
  • Fix identified — see patch below (push access denied, applying manually)
  • Post findings

Bug confirmed: false hook failure on clean scan

Cursor Bugbot's High Severity finding is correct.

Root causemodules/lefthook/psscriptanalyzer-staged.ps1 line 55:

$findings = foreach ($file in $targets) {
    Invoke-ScriptAnalyzer ...   # produces no output when the file is clean
}
$findings = @($findings)        # @($null) → Count = 1 ← bug

When all staged files pass, foreach produces no pipeline output and $findings is $null. In PowerShell, @($null) constructs a one-element array containing $null, so $findings.Count is 1. The if ($findings.Count) guard is therefore $true on a clean scan, the hook tries to format $null properties, prints ::: for each (empty), and exits 1 — a false failure on every clean commit.

Fix — filter nulls before wrapping:

-$findings = @($findings)
+$findings = @($findings | Where-Object { $_ })

Where-Object { $_ } drops $null values, so an all-clean run produces an empty array (Count = 0) and the hook correctly exits 0.

Fix this →


Remaining observations from the prior review

The previous review's two minor notes stand — neither blocks merge, but worth considering alongside the bug fix:

Finding File Line Severity
@($null).Count = 1 false failure on clean scan psscriptanalyzer-staged.ps1 55 Bug — fix above
NRE filter broader than justified — any rule's NRE is silently dropped, not just PSUseCompatibleSyntax psscriptanalyzer-staged.ps1 59 Minor / awareness
foreach comment says it's the accumulation mechanism; accumulation is actually from AddRange psscriptanalyzer-staged.ps1 49 Nit
_typos.toml CRLF \r? fix _typos.toml 17 ✅ Correct
.editorconfig header / Biome path .editorconfig 1–11 ✅ Correct
.shellcheckrc canonical-path qualification .shellcheckrc 1–5 ✅ Correct

The $null bug should be resolved before merge — it would make the hook fail on every clean staged-file run, which defeats the purpose of the hook. The one-line fix above is the right approach.

@cursor cursor 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.

Stale comment

Risk: medium. Cursor Bugbot completed with a skipped check and reported one unresolved high-severity finding in the staged analyzer hook. Human review is needed before merge; no reviewers were assigned because the only maintainer is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@cursor cursor 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.

Stale comment

Risk: medium. Not approving: Cursor Bugbot reported 1 unresolved high-severity finding in psscriptanalyzer-staged.ps1, and the Bugbot check completed as skipped. Human review is needed before merge; no reviewers were assigned because the only repo contributor is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

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


PR Review

  • Read CLAUDE.md for repo guidelines — none found
  • Read current state of changed files
  • Verify null-findings fix (@($null) bug) in psscriptanalyzer-staged.ps1
  • Review full diff from origin/main
  • Post comprehensive findings

@($null) false-failure fix — VERIFIED ✅

The Cursor Bugbot High Severity finding (and prior review) identified that $findings = @($findings) would produce @($null).Count = 1 when all staged files pass, making the hook fail on every clean commit. The latest commit (89d1ed1 — "fix: ignore null analyzer findings") resolves this correctly:

# Line 55 — was: $findings = @($findings)
$findings = @($findings | Where-Object { $_ })

Where-Object { $_ } drops $null before wrapping, so an all-clean run yields an empty array (Count = 0) and the hook exits 0. Correct fix, correctly placed (between the accumulation loop and the error-triage block).


Remaining observations (unchanged from prior review)

Minor — NRE filter breadth (line 59)

$realErrors = @($saErrors | Where-Object { $_.Exception.Message -notmatch 'Object reference not set' })

The filter silences any NullReferenceException from any rule, not just PSUseCompatibleSyntax. A broken rule that happens to throw an NRE would be dropped rather than surfaced. In practice extremely low risk — NREs from non-PSUseCompatibleSyntax rules are rare and the filter is still a huge improvement over the original SilentlyContinue blanket. Flagging for awareness; not a blocker.

Nit — foreach accumulation comment (line 49)

"a foreach statement (not ForEach-Object) keeps them in this scope so they accumulate"

The accumulation is actually from the manual $saErrors.AddRange(@($err)) call, not from foreach itself. foreach matters because it keeps $err readable in the enclosing scope (vs ForEach-Object where $err is scoped to the scriptblock), but that's about scope visibility, not accumulation. Minor wording imprecision; doesn't affect correctness.


All files — summary

File Finding Status
psscriptanalyzer-staged.ps1:55 @($null) false-failure fix via Where-Object { $_ } ✅ Fixed in 89d1ed1
psscriptanalyzer-staged.ps1:35–43 .psd1 parse guard — Import-PowerShellDataFile -ErrorAction Stop ✅ Correct
psscriptanalyzer-staged.ps1:51–54 Error accumulation via AddRange in foreach loop ✅ Correct
psscriptanalyzer-staged.ps1:59 NRE filter slightly broader than PSUseCompatibleSyntax only Minor / awareness
psscriptanalyzer-staged.ps1:49 Comment says foreach causes accumulation; it's actually AddRange Nit
modules/typos/_typos.toml:14 \r?\n CRLF-safe fix; comment is accurate ✅ Correct
.editorconfig:1–11 Broadened header; Biome path qualified to standards repo ✅ Correct
.shellcheckrc / modules/shellcheck/.shellcheckrc "standards repo's" qualification; same SHA (correct sync) ✅ Correct

No blocking issues. The critical false-failure bug is resolved. The two remaining minor points don't change correctness and are fine to carry forward.

cursor[bot]
cursor Bot approved these changes Jul 8, 2026
…yntax only

The staged-file analyzer hook tolerated a benign NullReferenceException from
PSUseCompatibleSyntax by filtering on the substring "Object reference not set".
That message match was too broad: it dropped EVERY analyzer error carrying that
message, including an NRE thrown by a consumer's custom/broken rule - silently
turning a broken ruleset into a green scan and defeating the hook's purpose.

The analyzer's ErrorRecord does not embed the rule name for built-in-rule
failures, so the NRE cannot be attributed to PSUseCompatibleSyntax by field
alone. Narrow the ignore as tightly as the data allows: tolerate the NRE only
when all hold - the message is the NRE signature, PSUseCompatibleSyntax is
actually effective for the scan (it ships enabled and is only skipped when the
consumer's settings exclude it), and the error did not originate from an
external/custom rule (those run via a distinct engine path - GetExternalRecord,
RuntimeException, error id 80131501, targeting the analyzer engine object).
Every other analyzer error, including any custom-rule NRE, now surfaces.

Co-Authored-By: Claude <noreply@anthropic.com>
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 56s —— View job


PR Review

  • Read CLAUDE.md for repo guidelines
  • Read current state of psscriptanalyzer-staged.ps1 — verify NRE narrowing commit
  • Review all changed files at HEAD
  • Post findings

Latest commit — af81420: Narrow NRE ignore to PSUseCompatibleSyntax only

This commit addresses the "minor" finding from the prior two review passes: the NRE error filter was previously broader than its justification — it silenced any NullReferenceException from any rule, not just PSUseCompatibleSyntax. The latest commit replaces that with a precisely targeted, three-condition guard.

$compatSyntaxEffective tracking (lines 50–57) — Correct. Sets to false if the consumer's .psd1 explicitly excludes PSUseCompatibleSyntax via ExcludeRules, or if an explicit IncludeRules list omits it. The @(... | Where-Object { $_ }) null-guards handle a .psd1 that doesn't define either key (produces an empty array, not an error). The no-settings-file path ($settingsData is $null → block skipped → $compatSyntaxEffective = $true) is also correct: PSUseCompatibleSyntax ships in the default ruleset, so it's effective when there are no consumer overrides.

External-rule detection (lines 83–87):

$fromExternalRule =
    ($_.FullyQualifiedErrorId -match '^80131501') -or
    ($_.Exception.StackTrace -match 'GetExternalRecord') -or
    ($null -ne $_.TargetObject -and $_.TargetObject.GetType().FullName -eq 'Microsoft.Windows.PowerShell.ScriptAnalyzer.ScriptAnalyzer')

Three independent heuristics OR'd together — any one matching classifies the error as external. This is the right approach: each heuristic could fail independently (PSScriptAnalyzer version change, stripped stack traces, TargetObject being $null), and combining them makes detection robust. The error-ID heuristic (^80131501) is the most version-sensitive, but the StackTrace and TargetObject fallbacks cover it.

Benign filter logic (line 87):

-not ($isNullRefError -and $compatSyntaxEffective -and -not $fromExternalRule)

Where-Object on $saErrors passes through (surfaces) any error for which this expression is $true — i.e., anything that is NOT (NRE + compat-syntax-effective + built-in-rule). The three-way AND means an NRE only gets silenced when all conditions are satisfied simultaneously: the error IS a null-ref, PSUseCompatibleSyntax IS active, AND the error is NOT from an external rule. This is exactly the stated intent. ✅

Previously fixed — null-findings guard (line 69):

$findings = @($findings | Where-Object { $_ })

Present and correct. @($null).Count = 1 false-failure on clean scan is resolved. ✅


Remaining nit (non-blocking, carried from prior reviews)

Comment inaccuracy (line 63)

"a foreach statement (not ForEach-Object) keeps them in this scope so they accumulate"

The foreach keyword matters here because it scopes $err to the enclosing scope (not to a scriptblock's inner scope as ForEach-Object would), making $err readable immediately after each call. But the accumulation happens via the explicit $saErrors.AddRange(@($err)) call, not from foreach itself. The comment conflates scope-visibility with accumulation. A reader unfamiliar with PowerShell scoping could conclude that foreach performs automatic accumulation. Fix this →


All files — final summary

File Finding Status
psscriptanalyzer-staged.ps1:50–57 $compatSyntaxEffective tracking — ExcludeRules, IncludeRules, and no-settings-file all handled correctly
psscriptanalyzer-staged.ps1:81–88 NRE filter narrowed to PSUseCompatibleSyntax only via 3-condition guard
psscriptanalyzer-staged.ps1:69 @($null) false-failure fixed via Where-Object { $_ }
psscriptanalyzer-staged.ps1:35–43 .psd1 parse guard — hard fail on malformed settings
psscriptanalyzer-staged.ps1:63 Comment says foreach causes accumulation; it's actually AddRange Nit
modules/typos/_typos.toml:17 \r?\n CRLF-safe fix; comment accurate
.editorconfig:1–11 Broadened header; Biome path qualified to standards repo
.shellcheckrc / modules/shellcheck/.shellcheckrc "standards repo's" qualification; same SHA (correct sync)

No blocking issues. The NRE narrowing is well-implemented and resolves the last substantive concern from the prior review passes. The one remaining nit (comment imprecision on foreach vs AddRange) doesn't affect correctness.

Comment thread modules/lefthook/psscriptanalyzer-staged.ps1 Outdated

@cursor cursor 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.

Stale comment

Risk: medium. Not approving: Cursor Bugbot completed as skipped and reported 1 unresolved finding in psscriptanalyzer-staged.ps1 on the latest commit. Human review is needed; no reviewers assigned because the only contributor is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@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: af814209a1

ℹ️ 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 modules/lefthook/psscriptanalyzer-staged.ps1 Outdated
… hook

The repo's CI PSScriptAnalyzer settings (modules/powershell/PSScriptAnalyzerSettings.psd1)
enable PSAvoidLongLines (120) and PSUseConsistentIndentation, which the prior
commit's module-default local run did not exercise. Hoist the analyzer engine
type name and the error's TargetObject into locals so no line exceeds 120 chars,
and align the multi-line boolean continuation to the block's indentation. Logic
is unchanged; verified 0 findings under the CI settings.

Co-Authored-By: Claude <noreply@anthropic.com>
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

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


PR Review

  • Read CLAUDE.md for repo guidelines
  • Read current state of psscriptanalyzer-staged.ps1
  • Assess Cursor Bugbot finding: Rules.PSUseCompatibleSyntax.Enable = $false not checked
  • Assess Codex finding: wildcard IncludeRules patterns not handled
  • Review latest style commit (c6e0a70)
  • Post findings

Latest commit (c6e0a70) — style only ✅

Wraps a long comment line and realigns the multi-line boolean continuation to pass PSAvoidLongLines (120) and PSUseConsistentIndentation — the same CI settings in modules/powershell/PSScriptAnalyzerSettings.psd1. The commit message confirms 0 PSScriptAnalyzer findings under those settings. No logic changes.


Bugbot Medium: Rules.PSUseCompatibleSyntax.Enable = $false not checked

Valid finding. The $compatSyntaxEffective block (lines 52–56) only checks ExcludeRules and IncludeRules membership. PSScriptAnalyzer's .psd1 format supports a third way to disable a rule:

Rules = @{
    PSUseCompatibleSyntax = @{
        Enable = $false
    }
}

If a consumer uses this pattern, $compatSyntaxEffective stays $true, and an NRE from any rule — not just the benign one — would be silently dropped. This is more likely than it might appear: the repo's own modules/powershell/PSScriptAnalyzerSettings.psd1 uses the Rules.X.Enable pattern extensively (e.g. PSUseCompatibleSyntax = @{ Enable = $true; TargetVersions = @(...) }), so consumers adopting that template could naturally reach for the same form to disable a rule.

Fix — add one check inside the if ($settingsData) block, after the existing IncludeRules line:

if ($settingsData.Rules.$compatSyntaxRule.Enable -eq $false) { $compatSyntaxEffective = $false }

In PowerShell $null -eq $false evaluates to $false, so this safely no-ops when Rules or Rules.PSUseCompatibleSyntax is absent or unset. Fix this →


Codex P2: wildcard IncludeRules/ExcludeRules patterns not handled

Valid but lower-risk. PSScriptAnalyzer accepts wildcards in IncludeRules and ExcludeRules. The current -contains / -notcontains checks do exact matching only, so:

  • ExcludeRules = @('PSUseCompatible*')$compatSyntaxEffective stays $true (wrong — rule is excluded)
  • IncludeRules = @('PSUseCompatible*')$compatSyntaxEffective is set to $false (wrong — rule IS active via the wildcard match)

The repo's own settings use exact names, so this gap doesn't affect the immediate use case. But the fix is mechanical:

# replace:
if ($excludeRules -contains $compatSyntaxRule) { $compatSyntaxEffective = $false }
if ($includeRules.Count -and ($includeRules -notcontains $compatSyntaxRule)) { $compatSyntaxEffective = $false }
# with:
if ($excludeRules | Where-Object { $compatSyntaxRule -like $_ }) { $compatSyntaxEffective = $false }
if ($includeRules.Count -and -not ($includeRules | Where-Object { $compatSyntaxRule -like $_ })) { $compatSyntaxEffective = $false }

Fix this →


Previously noted nit — still present

Comment imprecision (line 63): "a foreach statement (not ForEach-Object) keeps them in this scope so they accumulate" — the accumulation is from AddRange; foreach matters for scope visibility of $err, not automatic accumulation. Non-blocking wording nit from prior review passes.


Summary

File Finding Status
psscriptanalyzer-staged.ps1 Latest commit wraps long lines + fixes indentation only; 0 logic changes ✅ Style fix
psscriptanalyzer-staged.ps1:52–56 Rules.X.Enable = $false not checked — NRE from non-compat-syntax rule could be silently dropped Medium — fix above
psscriptanalyzer-staged.ps1:55–56 -contains misses wildcard IncludeRules/ExcludeRules patterns Low-Medium — fix above
psscriptanalyzer-staged.ps1:63 Comment conflates foreach scope with accumulation (from AddRange) Nit
psscriptanalyzer-staged.ps1:69 @($null) false-failure fixed
psscriptanalyzer-staged.ps1:35–43 .psd1 parse guard — hard fail on malformed settings
psscriptanalyzer-staged.ps1:82–89 NRE narrowed to PSUseCompatibleSyntax via 3-condition guard
modules/typos/_typos.toml:17 \r?\n CRLF-safe fix
.editorconfig:1–11 Broadened header; Biome path qualified
.shellcheckrc / modules/shellcheck/.shellcheckrc Standards-repo path qualification

The Rules.X.Enable = $false gap is the most actionable of the two new findings given that the repo's own settings template uses this form. Both are worth addressing; neither changes the fundamental correctness of the PR's core fixes.

@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: c6e0a702ad

ℹ️ 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 modules/lefthook/psscriptanalyzer-staged.ps1 Outdated

@cursor cursor 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.

Stale comment

Risk: medium. Not approving: Cursor Bugbot completed as skipped and still has an unresolved medium-severity finding in psscriptanalyzer-staged.ps1 on the latest commit. Human review is needed; no reviewers assigned because the only repo contributor is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

…e and wildcard-aware

$compatSyntaxEffective decided whether the benign PSUseCompatibleSyntax NRE is
tolerable, but it only inspected ExcludeRules/IncludeRules by exact match. Two
gaps let it misjudge whether the rule actually runs:

- Rules.PSUseCompatibleSyntax.Enable = $false disables the rule, yet the check
  ignored the Rules table and still treated an NRE from another rule as benign.
- IncludeRules/ExcludeRules accept wildcards, so IncludeRules = 'PSUseCompatible*'
  runs the rule but exact match set effective=false, treating its benign NRE as
  real and blocking every staged PowerShell commit in such repos.

Compute effectiveness across all three signals - Rules.<name>.Enable=$false,
wildcard-aware ExcludeRules, wildcard-aware IncludeRules - defaulting to
effective (the rule ships enabled) only when nothing disables or omits it.

Co-Authored-By: Claude <noreply@anthropic.com>
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

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


PR Review

  • Read CLAUDE.md for repo guidelines — none found
  • Read current state of psscriptanalyzer-staged.ps1 at HEAD (decebb1)
  • Verify Rules.PSUseCompatibleSyntax.Enable = $false check added
  • Verify wildcard IncludeRules/ExcludeRules handling added
  • Review all changed files at HEAD
  • Post findings

Latest commit (decebb1) — Bugbot Medium + Codex P2 both resolved ✅

This commit makes the $compatSyntaxEffective tracking complete by adding the two remaining gaps identified in the previous review passes.

Rules.PSUseCompatibleSyntax.Enable = $false check (line 56–58) — Correct. The expression $settingsData.Rules.$compatSyntaxRule.Enable -eq $false safely handles all null-chain cases: if Rules, Rules.PSUseCompatibleSyntax, or Enable is absent, PowerShell evaluates the chain to $null, and $null -eq $false is $false, so the guard no-ops. Only an explicit Enable = $false triggers the $compatSyntaxEffective = $false assignment. The Bugbot Medium finding is fully addressed. ✅

Wildcard-aware ExcludeRules/IncludeRules (lines 61, 64) — Correct. The prior -contains/-notcontains exact matching is replaced with -like via Where-Object:

if ($excludeRules | Where-Object { $compatSyntaxRule -like $_ }) { ... }
if ($includeRules.Count -and -not ($includeRules | Where-Object { $compatSyntaxRule -like $_ })) { ... }

The operand order ($compatSyntaxRule -like $pattern) is the right way round — the literal name is tested against each pattern. The Codex P2 finding is fully addressed. ✅

Three-condition OR structure — All three disable conditions (Enable = $false, ExcludeRules match, IncludeRules omission) are additive: any one can set $compatSyntaxEffective = $false, and there is no path back to $true once set. Order of evaluation is irrelevant for correctness. The comment at lines 49–52 now accurately enumerates all three conditions and explicitly documents the wildcard behavior. ✅

Previously fixed items — all still correct:

Check Location Status
@($null) false-failure via Where-Object { $_ } line 79
.psd1 parse guard — hard fail on malformed settings lines 37–43
Error accumulation via AddRange in foreach loop lines 75–78
NRE narrowed to PSUseCompatibleSyntax via 3-condition guard lines 92–99

Remaining nit (carried across all review passes — still present)

Comment inaccuracy (line 73)

"a foreach statement (not ForEach-Object) keeps them in this scope so they accumulate"

The accumulation is from the explicit $saErrors.AddRange(@($err)) call (line 77), not from foreach itself. foreach matters because it runs in the enclosing scope rather than a child scriptblock scope (as ForEach-Object would), making $err readable immediately after each Invoke-ScriptAnalyzer call — but foreach does not accumulate anything automatically. A future reader might conclude the keyword is doing the accumulation work. Fix this →

Not a blocker — doesn't affect correctness.


All files — final summary

File Finding Status
psscriptanalyzer-staged.ps1:56–58 Rules.X.Enable = $false check — Bugbot Medium resolved
psscriptanalyzer-staged.ps1:61, 64 Wildcard -like matching — Codex P2 resolved
psscriptanalyzer-staged.ps1:79 @($null) false-failure fixed
psscriptanalyzer-staged.ps1:37–43 .psd1 parse guard
psscriptanalyzer-staged.ps1:92–99 NRE triage — 3-condition guard
psscriptanalyzer-staged.ps1:73 Comment conflates foreach scope with accumulation (from AddRange) Nit
modules/typos/_typos.toml:17 \r?\n CRLF-safe fix
.editorconfig:1–11 Broadened header; Biome path qualified to standards repo
.shellcheckrc / modules/shellcheck/.shellcheckrc "standards repo's" qualification; same-SHA sync

No blocking issues. All substantive findings from prior review passes are resolved. The PR is in good shape to merge.

@cursor cursor 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.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit decebb1. Configure here.

Comment thread modules/lefthook/psscriptanalyzer-staged.ps1 Outdated

@cursor cursor 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.

Stale comment

Risk: medium. Not approving: Cursor Bugbot completed as skipped and reported 1 unresolved finding in psscriptanalyzer-staged.ps1 on the latest commit. Human review is needed; no reviewers assigned because the only repo contributor is the PR author.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

…urface all analyzer errors

The staged-file hook tolerated a benign PSUseCompatibleSyntax NullReferenceException
by inferring whether that rule was "effective" for the scan. That inference drew a
stream of valid edge-case findings (default Enable=$false, scalar vs array settings,
wildcard include/exclude) - accidental complexity in service of suppressing one
benign error.

Remove the whole surface instead: exclude PSUseCompatibleSyntax from this fast lane
via -ExcludeRule so the rule never runs and its benign NRE never arises, then surface
EVERY analyzer/engine error with no message-based suppression. This is strictly
non-silent - a rule that fails to load or a broken ruleset now always fails the hook,
the PR's stated intent. The rule remains enforced authoritatively in CI (the gate).

Tradeoff: a consumer that explicitly enabled PSUseCompatibleSyntax loses it in the
fast pre-commit lane but still gets it in CI.

Co-Authored-By: Claude <noreply@anthropic.com>
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 44s —— View job


PR Review

  • Read current state of psscriptanalyzer-staged.ps1 at HEAD (36313ec)
  • Assess Bugbot Low: scalar IncludeRules/ExcludeRules string splits characters
  • Review latest refactor commit
  • Post findings

Latest commit (36313ec) — architectural refactor ✅

This commit abandons the $compatSyntaxEffective inference approach entirely and solves the problem at the source: exclude PSUseCompatibleSyntax from this staged lane via -ExcludeRule so the rule never runs and its benign NRE never arises. This is the correct call.

The $compatSyntaxEffective block was accreting edge-case complexity from legitimate review findings: Rules.X.Enable = $false, wildcard IncludeRules/ExcludeRules, scalar-vs-array settings values. Each fix closed one gap but opened surface for the next. Excluding the rule eliminates the problem class rather than patching each instance.


Bugbot Low: scalar rules list splits characters — OBSOLETE

The entire $excludeRules/$includeRules parsing and wildcard-matching block was deleted. There is no code that pipes scalar strings through Where-Object for rule-name matching anymore. This finding does not apply to the current implementation.


Implementation review

-ExcludeRule alongside -Settings (line 50) — Correct. PSScriptAnalyzer's -ExcludeRule parameter adds to any ExcludeRules in the settings file rather than replacing them, so a consumer's existing exclusions are preserved. The comment on lines 47–49 accurately documents this.

Error collection (lines 55–59) — Correct. -ErrorAction SilentlyContinue suppresses console spew; -ErrorVariable err captures errors for inspection; AddRange accumulates across files. With PSUseCompatibleSyntax excluded, any NRE that does appear is from a different rule or broken ruleset and surfaces as a failure (lines 65–68). Strictly non-silent. ✅

Null-findings guard (line 60)@($findings | Where-Object { $_ }) present and correct. The @($null).Count = 1 false-failure fix from 89d1ed1 is preserved. ✅

Error surfacing (lines 62–68) — All analyzer errors now surface; no message-based suppression. This is the stated intent of the original PR and is now implemented with far less complexity. ✅

Trade-off documented — Commit message and inline comment both acknowledge: a consumer who explicitly enabled PSUseCompatibleSyntax loses it in the fast pre-commit lane but retains CI as the authoritative gate. This is an acceptable trade.

Persistent nit (line 53) — "a foreach statement (not ForEach-Object) keeps them in this scope so they accumulate across files" — accumulation is from AddRange (line 58); foreach matters for scope visibility of $err between calls. This wording imprecision has been noted across every prior review pass; not a blocker, and I won't flag it again.


All files — final summary

File Finding Status
psscriptanalyzer-staged.ps1 Refactor: exclude PSUseCompatibleSyntax rather than infer its state — eliminates problem class
psscriptanalyzer-staged.ps1:50 -ExcludeRule alongside -Settings — correct, exclusions additive
psscriptanalyzer-staged.ps1:55–59 Error accumulation via AddRange in foreach — correct
psscriptanalyzer-staged.ps1:60 @($null) false-failure guard preserved
psscriptanalyzer-staged.ps1:62–68 All errors surfaced, no suppression
Bugbot Low: scalar rules string enumeration Obsolete — relevant code deleted
modules/typos/_typos.toml:17 \r?\n CRLF-safe fix
.editorconfig:1–11 Broadened header; Biome path qualified to standards repo
.shellcheckrc / modules/shellcheck/.shellcheckrc "standards repo's" qualification; same-SHA sync

No blocking issues. The refactor is architecturally sounder than the approach it replaced — the complexity budget was being spent on inference logic that is simply unnecessary when the rule can be excluded outright. All substantive findings from prior review passes are resolved. Ready to merge.

@kyle-sexton
kyle-sexton merged commit 0877f28 into main Jul 8, 2026
37 checks passed
@kyle-sexton
kyle-sexton deleted the fix/config-hygiene-crlf-analyzer-pointers branch July 8, 2026 18:28

@cursor cursor 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.

Risk: medium. Approved: Cursor Bugbot passed on the latest commit with no unresolved findings, and all prior automated review threads are resolved. No reviewers assigned.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@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: 36313ecc7a

ℹ️ 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 modules/lefthook/psscriptanalyzer-staged.ps1
kyle-sexton added a commit that referenced this pull request Jul 11, 2026
## Summary

- add a centralized GitHub Actions runner policy that enforces approved
selector routing, public/hosted boundaries, explicit read-only
permissions, cancellation-safe literal hosted fallback, and
machine-readable hosted exceptions
- derive the required literal fallback from the governed policy default,
so changing an approved hosted image is a policy/configuration change
rather than parser-code surgery
- route Standards' 28 eligible private Linux workloads through the
governed selector while retaining exact hosted exceptions for policy and
control-plane boundaries
- recursively validate repository-local reusable workflows and
caller/callee permission narrowing without allowing arbitrary secrets,
tokens, inputs, labels, or runner expressions
- distribute the locked policy runtime and Node version to six enrolled
private consumers from one deterministic manifest
- harden staged .NET formatting and PSScriptAnalyzer adapters with
deterministic cross-platform path semantics and per-target no-profile
PowerShell isolation
- keep the complete .NET-format named job managed by Standards while
each consumer owns only strict data in `.lefthook/dotnet-format.json`
- preserve executable source and consumer index modes through
distribution
- pin every production selector/reusable and Actionlint parity reference
to merged `ci-workflows/main` commit
`99ac2f8c5b09dbb785d4eaf18465cbd96c30290c`

## Dependencies

The final routing contract is the immutable squash merge from
melodic-software/ci-workflows#74 (including stacked #76/#77):

- `99ac2f8c5b09dbb785d4eaf18465cbd96c30290c`

## Reviewed head

`0795d22c89cb8fae11642ede9757e7b43fd5d546`

## Validation

Independent author, reviewer, recheck, and integration-review gates all
PASS with no findings.

- runner-policy adversarial suite: 83/83, including alternate configured
hosted-default proof
- Standards private self-audit: PASS
- .NET/Lefthook adapter: 12/12
- pinned Lefthook 2.1.9 validate, dump, and actual job execution: PASS
- independent argv probe: spaces, semicolons, and `$()` remain inert
data with `shell:false`
- production distribution suite: 114/114 under checksum-pinned yq 4.53.3
in author native Linux and hosted Linux
- independent reviewer inspected the exact-head hosted log and confirmed
assertions 1 through 114
- exact executable-bit gate: PASS; both source CLIs are index mode
`100755`
- routing graph: 28 selectors, 28 workloads, 31 actual `ci-status`
gates, zero selector gates
- final pin proof: exactly 46 merged-main references, zero stale
full/short feature-stack references, and 25 preserved transitional
compatibility references
- all eight changed files reconstruct byte-for-byte from only the two
intended SHA/comment substitutions
- Actionlint 1.7.12 plus hosted checksum-verified ShellCheck 0.11.0:
PASS
- all 10 uniquely referenced workflow/action paths exist at the
immutable ci-workflows commit
- six workflow schemas, Zizmor medium/high, Biome, Markdown, ShellCheck,
Gitleaks, full Lefthook, and diff checks: PASS
- signed final pin commit: `0795d22c89cb8fae11642ede9757e7b43fd5d546`

All 63 hosted checks pass on this exact head.

## Authoritative basis

- GitHub Actions workflow syntax and runner routing:
https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
- Node cross-platform path semantics:
https://nodejs.org/api/path.html#pathwin32
- Lefthook v2.1.9 named-job merge contract:
https://github.com/evilmartians/lefthook/blob/v2.1.9/docs/configuration/jobs.md
- Lefthook v2.1.9 job templates:
https://github.com/evilmartians/lefthook/blob/v2.1.9/docs/configuration/templates.md

## Rollout safety

This PR does not change GitHub variables, secrets, runners, repository
settings, or live infrastructure. Production routing remains hosted
until the IaC and physical canary gates are applied later.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **High Risk**
> Large CI workflow refactor with secrets/vars for runner selection and
a new security gate; misconfiguration could break merges or route jobs
incorrectly until fleet is live.
> 
> **Overview**
> Introduces a **YAML-aware runner policy** (`components/runner-policy`,
`.github/runner-policy.json`, `policy.json`) and a hosted **Runner
policy** CI lane that tests and enforces it against workflow inventory
and repository visibility.
> 
> **CI routing** shifts eligible lint/contract jobs from fixed
`ubuntu-latest` to paired `select-runner` + workload jobs using
`needs.select-*.outputs.runner || 'ubuntu-24.04'`, `if: ${{ !cancelled()
}}`, and `merge_group` support. Control-plane jobs (runner-policy gate,
ci-status, zizmor, osv-scanner) stay on explicit hosted runners with
documented exceptions. `ci-status` now requires `runner-policy`, treats
only `success` as pass (not `skipped`), and pins several workflows to
`ci-workflows@99ac2f8`.
> 
> **Local hooks:** Lefthook .NET formatting moves to a consumer-owned
`.lefthook/dotnet-format.json` and `dotnet-format-staged.mjs`
(shell-less `dotnet format whitespace`). PSScriptAnalyzer staged checks
run **one target per fresh `pwsh` worker**; `PSUseCorrectCasing` is
removed from settings. Dependabot gains an npm root for
`components/runner-policy`.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
0795d22. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
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