v2: Add left-hook pre-push#1003
Conversation
… npm install on checkout and merge
|
What happens if the test(s) fail? I once had a colleague tell me they don't like putting things like tests in a pre-commit script because you should never stop a developer from committing their code. To be fair, this was in the context of employees working on a commercial software stack, not an open source SDK. Still, it has stuck with me and I think there is something to it. Example might be if a developer is going out of town for a week and wants to commit their work in progress on a feature. If it is failing a test and can't be committed, then no one else can pick it up while they are out. |
They can still do |
|
Appreciate the PR! Maybe we should discuss this in an in issue/discussion to figure out what we want to agree on? fwiw my view is that if you want to run things locally you always can, and if not CI will be there to catch this. I've personally found pre-commit/other hooks burn more of my time than they save, or are frustrating when trying to commit a WIP or draft piece of work (appreciate can use |
|
could we do pre-push by default, and make pre-commit optional? |
…cript-sdk into feature/ts-sdk-hooks
… into feature/ts-sdk-hooks
This is a great idea and alleviates a lot of the concerns. Done and pushed. cc @pcarleton |
|
Following up on this. We could observe a lot of PRs having a "prettier fix" commit quite often - this on top of a "test fix" commit. Having some hooks would resolve that. |
|
KKonstantinov
changed the title
KKonstantinov
changed the title
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
…cript-sdk into feature/ts-sdk-hooks
KKonstantinov
added
the
v2
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
Pre-push LGTM and seems useful to encourage better submissions.
The post-checkout/post-merge pnpm install feels a bit aggro though - I wouldn't want every branch switch to fire pnpm install tbh.
Could we stick to the pre-push hook only? We can still keep the others optional.
…yml; fix typo in .gitignore
|
@felixweinberger done, addressed the comment |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude review |
| - name: 'Build' | ||
| run: pnpm run build:all | ||
| fail_text: | | ||
| 💡 To catch build issues earlier, enable the pre-commit hook: | ||
| cp lefthook-local.example.yml lefthook-local.yml |
There was a problem hiding this comment.
🟡 The Build job fail_text (lines 35-37) suggests enabling the pre-commit hook to catch build issues earlier, but lefthook-local.example.yml only defines Typecheck and Lint Fix & Format pre-commit jobs -- there is no build step. A contributor following this advice would still only get build feedback at pre-push time.
Extended reasoning...
What the bug is
The Build job in lefthook.yml (lines 33-37) has a fail_text that reads:
To catch build issues earlier, enable the pre-commit hook:
cp lefthook-local.example.yml lefthook-local.yml
This suggests that enabling the optional pre-commit hooks will catch build failures earlier. However, lefthook-local.example.yml only defines two pre-commit jobs: Typecheck (pnpm typecheck:all) and Lint Fix & Format (pnpm lint:fix:all). There is no build step in the pre-commit configuration.
Step-by-step proof
- A contributor pushes code and the pre-push Build job (pnpm run build:all) fails.
- They see the fail_text suggesting they copy lefthook-local.example.yml to lefthook-local.yml.
- They follow the advice: cp lefthook-local.example.yml lefthook-local.yml.
- Now on their next commit, the pre-commit hook runs -- but it only executes Typecheck and Lint Fix & Format.
- No build step runs at pre-commit time.
- The contributor pushes again and the build still fails at pre-push time, exactly as before.
- The advice was misleading -- enabling the pre-commit hook did nothing to catch build issues earlier.
Why existing code does not prevent this
The Typecheck and Lint jobs have similar fail_text messages, which are at least partially justified -- the pre-commit config does include pnpm typecheck:all and pnpm lint:fix:all. It appears the Build job fail_text was copied from one of the other jobs without adjusting for the fact that no corresponding build step exists in the pre-commit example.
Impact
This is a misleading hint that could confuse contributors. Someone who follows the advice expecting their build issues to be caught at commit time will be frustrated when they still only see build failures at push time. It erodes trust in the developer tooling suggestions.
How to fix
Either:
- Remove the fail_text hint from the Build job (or replace it with a message that does not reference pre-commit hooks), or
- Add a build step (pnpm build:all) to the pre-commit jobs in lefthook-local.example.yml.
Option 1 is likely preferable since running a full build on every commit would be slow and counterproductive for the DX goals of this PR.
The nightly update-spec-types workflow is failing because lefthook's pre-push hook (added in #1003) runs `pnpm run lint:all`, which rejects the freshly-fetched spec.types.ts for not matching prettier style. Format the output via prettier's Node API using the project config before writing it to disk. This also reduces nightly PR diffs to only semantic changes, not formatting noise.
5 participants
This is a follow-up to #976 with a goal of improving DX and reducing contributor friction by ensuring linting, formatting, and builds are validated before pushing.
It introduces the following git hooks via Lefthook:
Default Hooks (
lefthook.yml)Pre-push (runs in parallel):
pnpm typecheck:all)pnpm lint:all)pnpm build:all)Post-checkout & Post-merge:
pnpm installto keep dependencies in syncOptional Pre-commit Hooks (
lefthook-local.example.yml)For contributors who prefer earlier feedback, an optional pre-commit configuration is provided:
This enables:
pnpm typecheck:all)pnpm lint:fix:all) withstage_fixed: trueMotivation and Context
PRs could get slowed down by failing CI when there is an accidental omission by the contributor to run lint/typecheck/build before pushing.
Using pre-push as the default (rather than pre-commit) balances catching issues early while not slowing down the commit workflow. Contributors who want faster feedback can opt into the pre-commit hooks.
How Has This Been Tested?
npx lefthook run pre-pushlefthook-local.example.ymltolefthook-local.ymlBreaking Changes
No breaking changes, a pure DX improvement.
Types of changes
Checklist