Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
PR Review Summary
2 Key Findings | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
- 🟠 Major:
.husky/pre-push:6Pre-push hook modifies working tree instead of validating (uses--writeflag)
🟠 1) agents-docs/content/community/contributing/overview.mdx Contributing documentation now describes outdated behavior
Issue: The contributing documentation explicitly states that the pre-push hook runs pnpm check:prepush (lint, typecheck, and tests). This is now stale and will mislead contributors.
Why: Contributors following this documentation will incorrectly believe that lint, typecheck, and tests run on pre-push. When they push code that passes format checks but fails lint/typecheck/tests, they will be surprised when CI fails. This undermines trust in the documentation.
Fix: Update the documentation to reflect the new behavior:
Line 131-133:
#### Pre-push Hook
The pre-push hook runs `pnpm format` to verify code formatting. Full validation (lint, typecheck, tests) runs in CI.Line 257:
These checks must pass before a PR can be merged. Pre-commit uses lint-staged, pre-push runs format checks, and CI is the source of truth.Refs:
💭 Consider (1) 💭
💭 1) turbo.json:168-172 Orphaned check:prepush task
Issue: The check:prepush turbo task and npm script are no longer invoked by the pre-push hook.
Why: While not broken, these are now orphaned from their intended use case. Creates maintenance overhead and potential confusion.
Fix: If the intent is to permanently reduce pre-push validation, consider removing the orphaned task or documenting its alternate purpose.
Refs: turbo.json:168
💡 APPROVE WITH SUGGESTIONS
Summary: The intent to reduce pre-push wait times is reasonable — deferring full validation to CI is a valid workflow choice. However, two issues should be addressed: (1) the pnpm format command uses --write which modifies files during push (should use pnpm format:check instead), and (2) the contributing documentation needs updating to reflect the new behavior.
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
turbo.json:163-167 |
check:husky task may also be orphaned |
Low confidence — unclear if used elsewhere; not directly related to this PR's changes |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
3 | 1 | 1 | 0 | 1 | 0 | 0 |
pr-review-docs |
2 | 1 | 0 | 0 | 0 | 0 | 1 |
| Total | 5 | 2 | 1 | 0 | 1 | 0 | 1 |
Note: Docs findings #1 and #2 were merged into a single Main finding (same documentation file, same root cause).
| # Full validation runs in CI | ||
|
|
||
| pnpm check:prepush | ||
| pnpm format |
There was a problem hiding this comment.
🟠 MAJOR: Pre-push hook modifies working tree instead of validating
Issue: The pnpm format command expands to biome check --linter-enabled=false --write. The --write flag causes Biome to modify files in place during a push operation.
Why: This is problematic because:
- Files are modified mid-push, leaving the working tree in a state where pushed commits don't match local files
- After pushing,
git statuswill show uncommitted changes that weren't part of the push - Developers may not realize their local state has diverged from what was pushed
If the intent is to enforce formatting before push, the hook should validate rather than auto-fix. Auto-fixing belongs in pre-commit (which already does this via lint-staged).
Fix:
| pnpm format | |
| pnpm format:check |
Refs:
No description provided.