fix(#548): pre-push markdownlint lints tracked files only#567
Conversation
- Replace the raw **/*.md glob in the markdownlint pre-push command with a git ls-files '*.md'-derived file list, so untracked markdown (installed-skill bundled docs, .claude/session/*.md scratch files, symlinked dirs) is never linted by the gate — eliminating false push failures for adopters with untracked markdown in the working tree - Add a graceful skip when no tracked .md files exist (new repo or branch with no markdown changes) - Add two regression-guard test cases to test_pre_push_gate.sh: case8 proves a lint-dirty UNTRACKED file does NOT block the push; case9 proves a lint-dirty TRACKED file still blocks it Refs #548 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
atlas-apex
left a comment
There was a problem hiding this comment.
Code Review: PR #567
Commit: 146e30eb30579b9a8d9fccf0fec2207f4ac88fcc
Summary
Changes the markdownlint entry in .claude/project-config.json's pre_push.commands to drive its file list from git ls-files '*.md' (tracked files only) instead of the raw **/*.md glob, so untracked markdown (installed-skill bundled docs, .claude/session/*.md scratch) no longer false-fails the pre-push gate. Adds regression cases 8 (untracked bad md → no fail) and 9 (tracked bad md → fails) to test_pre_push_gate.sh.
Checklist Results
- Architecture & Design: Pass
- Code Quality: Pass
- Testing: Pass
- Security: Pass
- Performance: Pass (n/a)
- PR Description & Glossary: Pass
- Summary Bullet Narrative: Pass — all three bullets are verb + rationale, none label-only
- Technical Decisions (AgDR):N/A — bug fix to an existing mechanism;
git ls-filesis git plumbing already used across the hooks. No new dependency / framework / pattern / storage choice. - Adopter Handbooks: N/A — diff touches a bash test + JSON config only.
migration-safety.md(blocking) doesn't trigger (no migration-path files); layering / commit-msg advisory handbooks don't apply.
Verification performed
I checked out the PR HEAD into a worktree and ran the actual 9-case suite (not just the 7-case local copy):
PASS [untracked-bad-md-ignored] (case8)
PASS [tracked-bad-md-fails] (case9)
PASS: 9 FAIL: 0
npx was present on the runner, so case9 actually executed (not the graceful SKIP path) — confirming the fix does NOT weaken the gate: a lint-dirty tracked file still blocks the push with markdownlint: FAILED.
Shell-soundness of the production command verified in an isolated repo:
- Empty-list guard works —
md_files=$(git ls-files '*.md'); [ -z "$md_files" ] && { echo INFO; exit 0; }short-circuits when there's no tracked markdown, somarkdownlint-cli2is never invoked argument-less (which would otherwise default-scan the whole tree). Confirmed: empty list → clean exit, no lint run. - Filename safety — production uses
echo "$md_files" | tr '\n' '\0' | xargs -0, which is space-safe. Verifiedwith space.mdis passed as a single argument underxargs -0, whereas plain newline-splitxargswould have split it into two bogus paths. The production string is the more robust of the two shapes. - Tracked-only scope —
git ls-files '*.md'reports only index-tracked markdown; untracked working-tree files are invisible to it. This matches what a push delivers and what CI sees. - npx-missing skip preserved — the
command -v npx ... || { echo INFO; exit 0; }prefix is unchanged, so contributors without Node are still skip-with-note, not hard-blocked.
Glossary present and accurate. No secrets, no private-project leaks in the diff (2 files: the test and the config).
Issues Found
None blocking.
Suggestions
- nit (non-blocking): the production command uses
tr '\n' '\0' | xargs -0while the two new test cases use plain newline-splitxargs. The tests therefore validate the behaviour against a slightly simplified command string rather than the literal production string. The test comments explicitly explain why (jq rejects\0in the JSON test fixture; sandbox filenames are space-free), so this is a documented, reasonable trade-off — and the production string is the safer variant. No change required; flagging only so a future reader knows the two strings intentionally differ.
Verdict
APPROVED
🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 146e30eb30579b9a8d9fccf0fec2207f4ac88fcc
Summary
**/*.mdglob linted any.mdin the working tree, including installed-skill bundled docs,.claude/session/*.mdscratch files, and symlinked directories that never reach the repo. Replaced withgit ls-files '*.md'so the gate lints exactly what a push delivers — matching what CI sees and preventing brittle per-machine ignore-glob workarounds..mdfiles prints an informational note and exits 0, keeping behaviour consistent with the other guarded commands (npx-not-found path).case8proves that a lint-dirty UNTRACKED file does NOT trigger a gate failure;case9proves that a lint-dirty TRACKED file still blocks the push, so the fix does not weaken the gate for real content.Testing
.mdfile (e.g. in.claude/session/scratch.mdor.claude/skills/some-tool/DOCS.md).git push— gate must pass..mdfile (git add+git commit) and push again — gate must fail with the markdownlint error.bash .claude/hooks/tests/test_pre_push_gate.sh— all 9 cases must pass.Refs #548
Glossary
git ls-files); the only content a push actually delivers to the remote and that CI will see.pre-push-gate.sh) that mirrors CI checks beforegit pushto prevent wasting CI minutes on avoidable failures.git ls-files'*.md'for all tracked markdown).