ci(hooks): Lint the staged Markdown, not the working copy - #170
Merged
Merged
Conversation
The pre-commit hook passed working-tree paths to markdownlint, so with a partly staged file the working copy could pass while the staged version, the one actually committed, failed and only CI caught it. The hook now writes the staged content of each file, and the staged lint config, to a temporary tree and lints that. Paths are read NUL-separated so spaces survive. A new hook test covers staged-only and working-copy-only violations, and CI runs it next to the pre-push tests. Refs #144 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Renamed Markdown files can bypass linting, and the config fallback can still use unstaged working-tree content.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Updates the pre-commit hook to lint staged Markdown content instead of working-tree content.
Changes:
- Creates an index-based temporary lint tree.
- Adds seven hook regression tests.
- Runs the new tests in CI.
| File | Description |
|---|---|
.github/hooks/pre-commit |
Lints staged Markdown snapshots. |
.github/hooks/tests/pre-commit.test.sh |
Tests staged-content behavior and paths with spaces. |
.github/workflows/ci.yml |
Adds pre-commit tests to CI. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #170 +/- ##
=======================================
Coverage 85.40% 85.40%
=======================================
Files 77 77
Lines 1596 1596
Branches 150 150
=======================================
Hits 1363 1363
Misses 189 189
Partials 44 44 |
The hook's --diff-filter=ACM left out renames, so a renamed Markdown file, even with edits, wasn't linted; R is now included. When the lint config wasn't staged, the hook fell back to the working copy's, which might not be in the commit; it now uses only the staged config. Adds a test case for each, and the CI comment names the pre-commit tests. Addresses Copilot review on #170. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The stub linter only checked that a config existed, so the suite would still pass if the hook linted under the working or HEAD config, or lost the paths the config's ignores rely on. The stub now reads a forbidden word and an ignored prefix from the config it finds, and new cases cover a staged config that differs from the working one and a staged file under an ignored path. Swapping the hook to the working config fails two of them. Addresses Copilot review on #170. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mpaulosky
added a commit
that referenced
this pull request
Sep 30, 2026
This was referenced Sep 30, 2026
mpaulosky
added a commit
to mpaulosky/IssueManager
that referenced
this pull request
Sep 30, 2026
## Summary Part 1 of #227, porting mpaulosky/IssueTracker#170 and #180. `.github/hooks/pre-commit` linted the working-tree files, so a partly staged Markdown file could pass locally and fail in CI. It now lints what the commit will contain. - **Staged snapshot:** each staged Markdown file's index content is written into a temporary tree with the same layout (`git checkout-index --prefix`), and markdownlint-cli2 runs from there. Paths are read NUL-separated, so spaces survive, and renamed files (`R`) are linted under their new name. - **Staged configs only, both of them:** `.markdownlint-cli2.jsonc` and `.markdownlint.json` are copied into the snapshot when they're staged. markdownlint-cli2 reads both, and this repo's `.markdownlint.json` (`"default": false`) turns off every rule the jsonc file doesn't set, so the hook reaches the same result as `scripts/gate.sh`. A config that exists only in the working copy isn't used. - **Hook tests:** `.github/hooks/tests/pre-commit.test.sh` (14 cases) and `pre-push.test.sh` (35 cases) run each hook in a throwaway repo with stub tools. A new `Hook tests` job in `ci.yml` runs both on every PR. It's not a required check. ## Testing - `pre-commit.test.sh`: 14 passed. The two `.markdownlint.json` cases were written first; the staged one failed until the hook snapshotted that file. - `pre-push.test.sh`: 35 passed, against this repo's `scripts/gate.sh`. - `shellcheck`, `actionlint`, `zizmor` and `yamllint` are clean, and this commit went through the new hook and the pre-push gate. Refs #227 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Item 2 of #144.
.github/hooks/pre-commitran markdownlint on the working-tree paths, so with a partly staged Markdown file the working copy could pass while the staged version (the one committed) failed, and only CI caught it..mdfile's staged content, plus the staged.markdownlint-cli2.jsonc, into a temporary tree with the same layout (git checkout-index --prefix), and lints that tree. The config's ignores (e.g.docs/blogs/**) still apply.R) are linted under their new name..github/hooks/tests/pre-commit.test.sh(same style as the pre-push tests, with a stub linter), run by the CI Hook tests job.Testing
pre-commit.test.sh: 12 cases pass, including "violation staged but fixed only in the working copy → refused", "violation only in the unstaged working copy → allowed", a renamed file, and a staged config that differs from the working copy's. The stub linter applies the config it finds (a forbidden word and an ignored path), so the suite fails if the hook lints under the working-copy config.markdownlint-cli2in a scratch repo using this repo's config: a staged violation is caught even when the working copy is fixed, anddocs/blogs/stays ignored.shellcheck,yamllint,actionlintclean;scripts/gate.shpassed.Refs #144 (items 1 and 3 follow separately; atelier-store later).
🤖 Generated with Claude Code