Skip to content

chore: make the local lint loop match CI, and gate shellcheck on the active shell#24

Merged
jinyeow merged 3 commits into
mainfrom
chore/lint-ci-hygiene
Jul 15, 2026
Merged

chore: make the local lint loop match CI, and gate shellcheck on the active shell#24
jinyeow merged 3 commits into
mainfrom
chore/lint-ci-hygiene

Conversation

@jinyeow

@jinyeow jinyeow commented Jul 15, 2026

Copy link
Copy Markdown
Owner

Two "the local loop does not match CI" gaps, found via the project brain STATUS and each verified against the real thing rather than asserted.

1. Lint hook read the wrong ruleset (fix(claude))

claude/lint-powershell.ps1 only looked for .vscode/PSScriptAnalyzerSettings.psd1 walking up. This repo keeps its ruleset at the root — the file CI passes (ci.yml: -Settings ./PSScriptAnalyzerSettings.psd1). So the hook silently linted with PSSA defaults and reported rules the repo deliberately excludes. Every finding was a false positive the agent would then "fix".

Not hypothetical — while writing this PR the hook flagged 3 issues on its own new test file (PSUseBOMForUnicodeEncodedFile, PSUseShouldProcessForStateChangingFunctions ×2), all excluded at PSScriptAnalyzerSettings.psd1:20,25, while the whole tree is clean under the repo ruleset.

Now checks both layouts at each level. .vscode/ still wins where present, so those repos are unaffected.

Verification. RED/GREEN proven on the same tree, same input, differing only by hook version: old hook emits a false positive with a root ruleset present, new hook stays silent. New tests/lint-powershell.Tests.ps1 (3 tests). The first asserts the no-ruleset case still reports — without it an inert hook would green the whole suite, and it did in fact catch a bug in my own harness ([string] $SettingsDir binds $null to '', silently turning the no-ruleset case into a root-ruleset case).

2. shellcheck covered one file (ci(shellcheck))

The job ran shellcheck setup.sh and nothing else, so the sh hooks — including git/work-hooks/policy and policy.sh, whose fail-open semantics are load-bearing — were unlinted. They were unlinted locally too: shellcheck was installed on neither Windows nor WSL, so koalaman.shellcheck is added to winget/packages.json (installed and used to verify everything below).

  • Widened to the 8 active first-party shell files; the job's exact backslash-continued command verified clean locally.
  • Dropped continue-on-error — an advisory job nobody reads is the same as absent.
  • Excluded, deliberately and with reasons in the job: legacy (scripts/, config/, nvim/install.sh), sourced fragments (bash/*, no shebang), vendored skills, and the upstream git-ctags hooks — whose SC1007 is a false positive (CDPATH= cd -- is a valid env-var prefix), so gating would mean suppressing, not fixing. Measured: 33 findings across legacy, 10 across the ctags hooks.
  • One real finding, fixed: claude/statusline-command.sh now quotes "@{u}...HEAD" (SC1083 — the braces are literal git upstream shorthand). Verified quoted and unquoted resolve identically against a real repo.

Still needed for #2 to actually gate

The repo ruleset requires status checks by name and does not list shellcheck, so this PR alone gives a red ✗ that still will not block a merge. Adding shellcheck (active shell) to required_status_checks is a GitHub setting, not a repo change — best done after merge so the renamed job has run once on main.

Verify

Invoke-Pester -Path tests          # 27 passed, 1 skipped (pre-existing)
Invoke-ScriptAnalyzer -Path . -Recurse -Settings ./PSScriptAnalyzerSettings.psd1   # clean

jinyeow added 3 commits July 16, 2026 03:35
The PostToolUse lint hook only ever looked for `.vscode/PSScriptAnalyzerSettings.psd1`
walking up from the edited file. This repo keeps its ruleset at the ROOT - which is the
file CI passes (`-Settings ./PSScriptAnalyzerSettings.psd1`) - so the hook silently fell
back to PSSA defaults and reported rules the repo deliberately excludes. Every finding
was a false positive the agent would then "fix": Write-Host on git/work-hooks/policy.ps1
was the recurring one, and the hook flagged BOM + ShouldProcess on this commit's own test
file while the whole tree is clean under the repo ruleset.

Check both layouts at each level instead. `.vscode/` still wins where it exists, so repos
using that layout are unaffected.

Pinned by tests/lint-powershell.Tests.ps1. Its first test asserts the no-ruleset case
still REPORTS, so the suite cannot green on a hook that has stopped running - the failure
mode the other two tests would otherwise hide.
The job ran `shellcheck setup.sh` and nothing else, so the sh hooks - including
git/work-hooks/policy and policy.sh, whose fail-open semantics are load-bearing - were
unlinted in CI. They were unlinted locally too: shellcheck was installed on neither
Windows nor WSL, so add koalaman.shellcheck to winget/packages.json.

Widened to the eight active first-party shell files, verified clean locally by running
the job's exact command. Legacy (scripts/, config/, nvim/install.sh), sourced fragments
(bash/*), vendored skills, and the upstream git-ctags hooks stay excluded - each carries
pre-existing findings in code that works, mirroring the PSScriptAnalyzer job's carve-out.
An explicit list beats a glob here because the exclusions would outnumber the inclusions.

Dropped continue-on-error: the job was advisory, which is the same as absent. The one
finding in the set is fixed here - claude/statusline-command.sh quotes "@{u}...HEAD",
whose braces are literal git upstream shorthand (SC1083). Verified quoted and unquoted
resolve identically against a real repo.

NOTE: the repo ruleset requires status checks BY NAME and does not list shellcheck, so
this alone still will not block a merge. Adding it to the ruleset is a GitHub setting,
not a repo change.
From Codex's review of #24, verified before acting: the walk-up had no boundary, so a
stray PSScriptAnalyzerSettings.psd1 above a project silently governed it. Reproduced -
a git project with no ruleset of its own went SILENT because the hook used a ruleset one
level above it. Worst case is one file in $HOME quietly setting the ruleset for every
repo on the machine.

The hazard predates this branch, but checking the bare filename widened it materially:
a stray `PSScriptAnalyzerSettings.psd1` in a home dir is far likelier than a `.vscode/`
one. It also made the new tests environment-dependent - they assumed no stray ruleset
above $env:TEMP.

Stop at `.git`/`.jj`, checked AFTER the ruleset so one sitting at the root still wins.
`.jj` because a non-colocated Jujutsu repo has no `.git` - the same reason
nvim/lua/config/autocmds.lua roots on both. No `-PathType`: in this repo's own
bare-worktree layout `.git` is a hidden FILE, not a directory, and a Container check
would miss it and let the walk escape. Both forms are pinned by tests; every test now
sets a boundary, so they assert behaviour rather than the machine's directory layout.

Also generalises claude/AGENTS.md's linting rule, which hardcoded
`-Settings .vscode/PSScriptAnalyzerSettings.psd1` as though it were the universal path.
That standing instruction is the upstream cause of this whole class of bug: the hook was
only ever downstream of it.
@jinyeow

jinyeow commented Jul 15, 2026

Copy link
Copy Markdown
Owner Author

Codex cross-model review — applied

Codex (read-only MCP reviewer) found one real bug I missed. Verified before acting rather than taken on trust.

Applied — ruleset walk was unbounded (92753c6). Reproduced it: a git project with no ruleset of its own, plus a stray PSScriptAnalyzerSettings.psd1 one level above it → the hook went silent, linting with settings from outside the project. Worst case is a single file in $HOME quietly setting the ruleset for every repo on the machine.

The hazard predates this branch, but checking the bare filename widened it materially — a stray PSScriptAnalyzerSettings.psd1 in a home dir is far likelier than a .vscode/ one. It also made the new tests environment-dependent (they assumed nothing stray above $env:TEMP); every test now sets its own boundary, so they assert behaviour rather than the machine's directory layout.

Fix: stop at .git/.jj, checked after the ruleset so one sitting at the root still wins.

  • .jj because a non-colocated Jujutsu repo has no .git — the same reason nvim/lua/config/autocmds.lua roots on both.
  • No -PathType: in this repo's own bare-worktree layout .git is a hidden file, not a directory (gitdir: …/.bare/worktrees/…). A Container check would miss it and let the walk escape. Both forms are now pinned by tests.

Applied — claude/AGENTS.md:91 (92753c6). It hardcoded -Settings .vscode/PSScriptAnalyzerSettings.psd1 as though universal. Codex's argument holds: that standing instruction is the upstream cause of this whole class of bug — the hook was only ever downstream of it. Now says locate the ruleset and pass what CI passes.

Not applied — derive the shellcheck list from git ls-files (LOW). Measured the exclusions first: 33 findings across legacy, 10 across the ctags hooks — they'd outnumber the inclusions. And a wrong glob fails open, silently linting nothing, which is the worse failure for a gate. Keeping the explicit list; the job comment says to add new files there.

CI green: 28 passed / 2 skipped (pre-existing psmux skips — not installed on runners), PSScriptAnalyzer clean, shellcheck clean.

@jinyeow
jinyeow merged commit 1ce75d0 into main Jul 15, 2026
4 checks passed
@jinyeow
jinyeow deleted the chore/lint-ci-hygiene branch July 15, 2026 22:53
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