Skip to content

chore: add lint pre-commit hook#6697

Open
pkey wants to merge 2 commits intomainfrom
feat/add-pre-commit-hook
Open

chore: add lint pre-commit hook#6697
pkey wants to merge 2 commits intomainfrom
feat/add-pre-commit-hook

Conversation

@pkey
Copy link
Copy Markdown
Contributor

@pkey pkey commented Apr 1, 2026

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Commit messages
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)
  • Links to automated tests covering new functionality
  • Includes manual testing instructions (if necessary)
  • Updates relevant GitBook documentation (PR link: ___)
  • Includes product update to be announced in the next stable release notes

What does this PR do?

Adds a lint pre-commit hook that runs npm run lint (eslint + prettier) locally before each commit. This matches the linting step in the code-analysis CI job.

Also updates scripts/install-dev-dependencies.sh to run pre-commit install so the hooks are automatically activated for new and existing developers.

Why?

Linting issues that only surface in the code-analysis CI job increase the feedback loop — developers push, wait for CI, discover a formatting error, fix, push again. Running the same checks locally as a pre-commit hook catches these issues immediately, before the code even leaves the machine.

Where should the reviewer start?

  • .pre-commit-config.yaml — the new lint hook
  • scripts/install-dev-dependencies.sh — added pre-commit install

How should this be manually tested?

  1. Run pre-commit install (or ./scripts/install-dev-dependencies.sh)
  2. Introduce a formatting issue (e.g. bad indentation in a .ts file)
  3. git commit — should fail with prettier error
  4. Fix the formatting, commit again — should pass

What's the product update that needs to be communicated to CLI users?

N/A — internal developer tooling only.

@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Apr 1, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@pkey pkey force-pushed the feat/add-pre-commit-hook branch from fcbbc3c to b2116a2 Compare April 1, 2026 08:15
@pkey pkey marked this pull request as ready for review April 1, 2026 08:16
@pkey pkey requested review from a team as code owners April 1, 2026 08:16
@snyk-pr-review-bot

This comment has been minimized.

Run eslint and prettier only on staged files instead of the full
project to keep commits fast.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@pkey pkey force-pushed the feat/add-pre-commit-hook branch from b2116a2 to c3eb007 Compare April 1, 2026 08:23
@snyk-pr-review-bot

This comment has been minimized.

rev: v8.17.0
hooks:
- id: gitleaks
- repo: local
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Nice idea! did you intentionally not use npm run format?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only worry I have is performance/latency it adds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeterSchafer yeah I originally had npm run lint which I believe covers formatting and is also what we run in the code-analysis job. The issue with that is that it will run the linter/formatter on all the files every time.

This approach limits it only to what has changed before committing. Performance wise, we can't go any further I believe.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally on my 32GB M1 the whole npm run lint runs in 7s - so that's the absolute worst case scenario.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the latency is not exactly linear (1 file still takes 2.5s) but still worth it IMO given that this can easily be 20+ minutes before you find out you have a linter error in the CI.

image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think running the same commands as in the code-analysis step should suffice. i.e. npm run lint & make lint.

I agree that the ~15seconds it takes to run the pre-commit linting is better than waiting for the CI job to fail

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay let's run an experiment. when it gets too long, we can still change it!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@j-luong so would you prefer the full npm run lint command or this version? This version I believe covers everything though if you would change what npm run lint does it would fall through the cracks.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

Warnings
⚠️ There are multiple commits on your branch, please squash them locally before merging!

Generated by 🚫 dangerJS against 198147a

@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Dependency 🟠 [major]

The script now executes pre-commit install, but the pre-commit package is not added to the Brewfile (the primary dependency manifest for this script) within this PR. Since the script uses set -e, it will fail and exit for any user who does not already have pre-commit installed, preventing them from finishing the setup of other development dependencies.

pre-commit install
Strict CGO Dependency 🟠 [major]

The golangci-lint hook is configured with CGO_ENABLED=1. This forces the linter to require a local C toolchain (compiler and headers). Developers who do not have a C compiler installed will be unable to commit changes to the cliv2/ directory, even if their specific changes do not require CGO. This creates an unnecessary hurdle for contributors with standard Go installations.

entry: bash -c 'cd cliv2 && CGO_ENABLED=1 make lint'
Inefficient Hook Scope 🟡 [minor]

Setting pass_filenames: false for the golangci-lint hook means that the entire make lint command for the cliv2 project runs every time a file within that path is modified. This negates the performance benefits of pre-commit which usually only checks changed files. On larger projects, this will significantly slow down the git commit process for developers.

pass_filenames: false
📚 Repository Context Analyzed

This review considered 6 relevant code sections from 5 files (average relevance: 0.66)

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.

3 participants