Repository navigation
Exclude node_modules From Markdown Lint and Docker Lint Discovery - #1474
Merged
Merged
Conversation
## What this changes `.markdownlint-cli2.jsonc` gains `ignores` and `globs`. `scripts/docker_lint.py` drops `node_modules` from target discovery, and `GOVERNANCE.md`, `OPERATIONS.md`, and `scripts/README.md` say so where each already describes that discovery. `.gitignore` gains `node_modules/`. ## Why Per #1466, `"gitignore": true` was the config's only ignore source and no fleet `.gitignore` listed `node_modules`, so a checkout that had ever run a Node tool locally linted third-party Markdown. Measured on a scratch repo against markdownlint-cli2 v0.23.2: 1711 errors under `node_modules`, 0 after the `ignores` entry, with the entry filtering explicitly named file arguments as well as globs. `**/node_modules/**` rather than `node_modules`, because the bare form matched the root copy only and left a nested workspace copy linted. `globs` is declared alongside it because an `ignores` entry with no positive glob to subtract from lints no files and exits 0, where the same invocation before this change exited 2 on a usage error. A command-line glob still unions with it and a command-line negation still wins, so a repo narrowing the Lint Markdown step's own `globs:` input is unaffected. ## Why the wrapper is in scope too `docker_lint.py` discovers targets with `git ls-files --others --exclude-standard`, so an untracked, unignored `node_modules/**/*.sh` became a shellcheck and shfmt target, which no linter config can reach because neither tool has one. editorconfig-checker and cspell are immune, measured: the first excludes `node_modules` by default, the second takes two literal root paths. ## Why not only `.gitignore` #1466 offered that as the alternative, on the premise that `.gitignore` is not a carried file. It is carried, at `presence` fidelity, so hub content does not propagate and a carrier would keep the defect. Raising the fidelity is not the fix either, since `intent` never judges content and `.gitignore` legitimately varies by language. It is added here for this repository's own tree, where it is what keeps a local install out of `git add`. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe lint configuration and Docker runner now exclude ChangesLint target exclusion
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Lint target discovery now skips node_modules at any depth while retaining repository targets, with matching configuration, documentation, and regression coverage. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (6 skipped: 6 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive and well-tested; only a minor wording fix in a new JSONC comment was found.
Pull request overview
Updates the Markdown and Docker lint target-discovery logic to avoid linting third-party content under node_modules/, and documents that behavior where discovery is described.
Changes:
- Add
node_modulesexclusion and explicitglobsto.markdownlint-cli2.jsoncto prevent linting third-party Markdown and avoid silent zero-target runs. - Exclude any
**/node_modules/**paths fromscripts/docker_lint.pytarget discovery (with unit tests covering nested and pattern-free cases). - Update runbooks/docs to state that Docker lint discovery excludes
node_modules, and ignorenode_modules/locally via.gitignore.
File summaries
| File | Description |
|---|---|
scripts/docker_lint.py |
Adds a git pathspec exclusion for **/node_modules/** during target discovery. |
scripts/tests/test_docker_lint.py |
Adds regression tests ensuring node_modules is excluded for markdownlint/shellcheck and doesn’t break pattern-free linters. |
.markdownlint-cli2.jsonc |
Adds globs and ignores to avoid linting node_modules content and prevent silent zero-target behavior. |
scripts/README.md |
Documents that the Docker lint runner excludes node_modules from discovery. |
GOVERNANCE.md |
Updates the wrapper description to note node_modules exclusion during discovery. |
OPERATIONS.md |
Updates the local verification runbook to note node_modules exclusion during discovery. |
.gitignore |
Ignores node_modules/ for this repository’s working tree. |
reports/canonical-review.json |
Records the canonical-review entry for the .markdownlint-cli2.jsonc unit. |
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot read "nothing positive to subtract from lints no files" as making "lints" a noun, which it does. The claim is unchanged: the ignores entry only subtracts, so a run with no positive glob lints nothing and exits 0. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The exclusion logic is correctly implemented, covered by targeted tests, and consistently documented across the referenced runbooks.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Owner
Author
|
@coderabbitai review |
|
This was referenced Sep 9, 2026
ptr727
added a commit
that referenced
this pull request
Sep 10, 2026
…g, and the Release-Trigger Retirement (#1489) Promotes thirteen merged pull requests from `develop` to `main`. ## What this promotes - **#1486** State the `host-tools.json` carry rule and stop the empty-overlay warning. `spec/files.json` now requires the declaration unconditionally, and `host_gate.py` no longer warns on an empty one. - **#1484** Fan the validator's pytest leg out over a `python-versions` input, so a caller with more than one interpreter no longer silently loses its other legs to a pinned 3.13, with an entry guard that refuses a value `fromJson` would otherwise admit. - **#1476** Retire the publish-on-merge release trigger, which `spec/files.json` applied to an interface contract whose task has no publish-on-merge path. - **#1479** Ship the release caller snippet and retire the not-yet-released claim. - **#1474** Exclude `node_modules` from markdownlint and Docker lint discovery at any depth. - **#1468** Move the carried-surface cluster off decision, recording the measurements and what stays unmeasured on the issues that hold them. - **#1498** Correct the release-caller snippet's publish-on-merge claim, raised by CodeRabbit against this promotion and fixed on its own branch, since a promotion PR's head is `develop`. - **#1494** Settle the markdown lint exclusion seam as a nested config. - **#1492** Check declared deployment environments and fix the carried-payload rule. - **#1501** Move the session handoff to a chain of labeled issues. Two of its units are verbatim fidelity, `AGENTS.md` "Session Scope" and `GOVERNANCE.md` "Communicating with the User", so every fleet repo picks them up on its next resync. - **#1505** Refuse a bare handoff link rather than reading it as no successor, answering a finding raised against this promotion. - **#1504** Reject a deployment environment name or branch carrying surrounding whitespace, answering two more. - **#1506** Encode an environment name before it becomes a path segment, delete a branch that cannot fire, and dedupe a registry name by case, answering two more and declining one on a reproduction. ## Closing references A `develop`-targeted pull request registers no closing reference, so the issues those pull requests settled close with this promotion. #1470 is the one that needs a word. Two of its seven acceptance items are not met as literally written, and the maintainer's decision to close it here anyway, with the basis for both, is recorded on the issue itself. Closes #1483 Closes #1473 Closes #1487 Closes #1482 Closes #1475 Closes #1470 ## Left open deliberately - **#1499**, **#1500** follow-ups #1501 filed rather than fixed. - **#1503** the validator-looser-than-its-consumer class, filed from findings on this promotion and deliberately not closed by it. Five instances recorded, and the fix is a design decision rather than a check. - **#1497** the Docker delta list never says to remove the `push:` block. - **#1472** the fleet-level pin call, deliberately not pre-empted by #1479. - **#1321**, **#1464** carry #1468's decision record and remain in their own scope. ## Verification `origin/main..origin/develop` merges clean, no end-of-line conflict. Each promoted pull request ran the fleet gate set on its own branch before merging; this promotion adds no content of its own. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01817rckKywjfoaEV5WF9TKs
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.
Addresses #1466.
What this changes
.markdownlint-cli2.jsoncgains"ignores": ["**/node_modules/**"]and"globs": ["**/*.md"].scripts/docker_lint.pydrops any path undernode_modulesfrom target discovery, with three tests.GOVERNANCE.md,OPERATIONS.md, andscripts/README.mdsay so where each already describes that discovery..gitignoregainsnode_modules/, for this repository's own tree.The measurements
Run against
markdownlint-cli2v0.23.2 on a scratch git repository holding a realnode_modules, a nestedsub/node_modules, a gitignoreddist/, and two authored files.**/*.mdnode_modulesignores: ["**/node_modules/**"],.gitignorewithoutnode_modulesdist/still excluded bygitignore: trueignores: ["node_modules"]or["node_modules/"]sub/node_modulesstill lintedignoresapplied to explicitly named file argumentsLinting: 1 fileof 3 namedignoreswith noglobs, no command-line globLinting: 0 files, exit 0globs, no command-line globglobsplus command-line**/*.mdglobsplus command-line!docs/**The fifth and sixth rows are why
globsis declared alongsideignoresrather than after it: anignoresentry with no positive glob to subtract from turns a loud usage error into a silent success. The last two rows are why a repository narrowing the Lint Markdown step's ownglobs:input is unaffected.Why the wrapper is in scope
docker_lint.pydiscovers targets withgit ls-files --others --exclude-standard, so an untracked, unignorednode_modules/**/*.shbecame a shellcheck and shfmt target. No linter config reaches that, because neither tool has one. Measured on the same scratch repository:git ls-files --others --exclude-standard -- '*.sh'returnednode_modules/somepkg/bin/run.sh, and returns nothing once the exclude pathspec is appended.editorconfig-checkerandcspellare immune, also measured rather than assumed.editorconfig-checker --dry-runon that tree listed neithernode_modulescopy, because it excludes them by default.cspell's two patterns are literal root paths that no nested file matches.:(glob,exclude)rather than:(exclude), because withoutglobmagic the leading**/does not match zero directories and the root copy leaked. Each of the three tests fails with the change reverted.Why not
.gitignorealone#1466 offered that as the alternative, on the premise that
.gitignoreis not a carried file. It is carried, atpresencefidelity, so hub content does not propagate and a carrier would keep the defect. Raising the fidelity is not the fix either:intentnever judges content, and a.gitignorelegitimately varies by language, which is why it sits atpresence. It is added here for this repository's own tree, where it is what keeps a local install out of agit add.The Financial-Modeling outlier
#1466 asked whether that repository's
ignoresblock is the fix to promote. It is not: its entries aredata/**andnotes/forms-decisions.md, a committed document archive and the owner's working notes, neither of themnode_modules. It is genuinelymodifiedagainst this canonical and needs its own disposition. Its comment did already identify the silent-zero-files hazard above, which is where theglobshalf of this change comes from.Verification
ruff format --check,ruff check,mypy,prose_lint.py --diff HEAD,repo_gate.py --check eol,spec/validate.py, all 15scripts/testssuites, anddocker_lint.py --linter markdownlint --linter editorconfig-checkerover this tree, which reported 182 Markdown files and 0 issues with anode_modulespresent.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
node_modulesdirectories at any depth, preventing locally installed packages from being treated as project files.Documentation
node_modulesexclusions.node_modulespaths.Chores
node_modules/to ignored files so local dependencies are not included in version control.