Skip to content

chore(git): add husky pre-push format hook#59

Merged
PatrickSys merged 4 commits intomasterfrom
feat/pre-push-format-hook
Mar 5, 2026
Merged

chore(git): add husky pre-push format hook#59
PatrickSys merged 4 commits intomasterfrom
feat/pre-push-format-hook

Conversation

@PatrickSys
Copy link
Owner

Summary

  • Add Husky devDependency and prepare script
  • Configure pre-push hook to run \pnpm format\
  • Harden grammar sync script against file lock errors
  • Increase timeout for search hints test to reduce flakiness

Test plan

  • pnpm test (runs via pre-commit hook)
  • git push on feature branch (verifies pre-push hook runs pnpm format)

- add husky devDependency and prepare script

- configure pre-push hook to run pnpm format

- harden sync-grammars and extend search hints test timeout to avoid flakiness
@greptile-apps
Copy link

greptile-apps bot commented Mar 5, 2026

Greptile Summary

This PR wires up Husky v9 to enforce code quality gates: pnpm test on pre-commit and pnpm format on pre-push. It also hardens sync-grammars.mjs against Windows file-lock errors and bumps the search-hints test timeout to reduce CI flakiness. The Husky wiring (package.json + lockfile) is correct, but the pre-push hook has a fundamental logic flaw that prevents it from achieving its goal.

Key changes:

  • package.json / pnpm-lock.yaml: Add husky@9.1.7 as a devDependency with a prepare install script — correct and clean.
  • .husky/pre-push: Runs pnpm format --write, but because the hook executes after commits are finalized, any files reformatted by Prettier are left as unstaged working-tree changes. The pushed commits remain unformatted. Using pnpm format:check instead would correctly reject a push when files need formatting.
  • .husky/pre-commit: Missing a shebang line, inconsistent with the pre-push hook style.
  • scripts/sync-grammars.mjs: The EPERM/EBUSY catch block is a good defensive addition for Windows, but copied++ runs even when a file is skipped, inflating the final log count.
  • tests/search-hints.test.ts: 60-second timeout added to the first integration test — reasonable fix for flakiness.

Confidence Score: 3/5

  • Safe to merge for the lockfile and test changes, but the pre-push hook's core purpose is broken and should be fixed before shipping.
  • The non-hook changes (lockfile, grammar sync hardening, test timeout) are low-risk. However, the pre-push hook runs pnpm format --write after commits are finalised, which means it modifies the working tree without affecting the pushed commits — the hook is effectively a no-op for enforcing formatting on remote code and leaves the developer's repo in an unexpected state after every push. This logic flaw is the primary reason for the reduced score.
  • .husky/pre-push requires the most attention — the choice of pnpm format vs pnpm format:check determines whether the hook achieves its goal.

Important Files Changed

Filename Overview
.husky/pre-push New pre-push hook that runs pnpm format --write; this modifies files in the working tree after commits are finalized, so the pushed commits remain unformatted — the hook does not achieve its stated goal.
.husky/pre-commit New pre-commit hook running pnpm test; missing a shebang line, inconsistent with the pre-push hook style.
package.json Adds husky as a devDependency and a prepare script to install hooks on pnpm install; straightforward and correct.
scripts/sync-grammars.mjs Adds EPERM/EBUSY error handling for Windows file locks; logic is sound but the copied counter is incremented even when a file is skipped, making the final log line misleading.
tests/search-hints.test.ts Adds a 60-second timeout to the first integration test to reduce flakiness on slower machines; no functional changes.
pnpm-lock.yaml Lockfile updated to add husky@9.1.7; auto-generated and consistent with package.json changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[git commit] --> B[pre-commit hook\npnpm test]
    B -->|tests pass| C[Commit created]
    B -->|tests fail| D[Commit blocked]

    E[git push] --> F[pre-push hook\npnpm format --write]
    F --> G{Files modified\nby formatter?}
    G -->|Yes| H[Working tree modified\nbut NOT committed]
    G -->|No| I[Push proceeds\nwith original commits]
    H --> I
    I --> J[Remote receives\noriginal unformatted commits]
    H --> K[Developer left with\nunstaged formatted changes]

    style H fill:#ffcccc,stroke:#cc0000
    style J fill:#ffcccc,stroke:#cc0000
    style K fill:#fff3cd,stroke:#ffa500
Loading

Last reviewed commit: b9381d7

.husky/pre-push Outdated
. "$(dirname -- "$0")/_/husky.sh"

echo "Running pnpm format before push..."
pnpm format
Copy link

Choose a reason for hiding this comment

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

pnpm format modifies files but won't affect pushed commits

Running pnpm format (which uses --write) in a pre-push hook formats files in the working tree after the commits have already been made. The push then proceeds with the original, potentially unformatted commits — so the remote branch ends up with unformatted code. Additionally, the developer is left with uncommitted formatting changes in their working directory after every push, which is confusing.

To actually enforce formatting on pushed code, consider one of these alternatives:

  1. Fail fast with format:check — reject the push if any file is unformatted, prompting the developer to format-and-commit before pushing:
Suggested change
pnpm format
pnpm format:check
  1. Move formatting to the pre-commit hook — format files before staging so the commit itself contains formatted code (common pattern with lint-staged).

throw error;
}
}
copied++;
Copy link

Choose a reason for hiding this comment

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

copied counter inflated when file is skipped

When renameSync throws EPERM/EBUSY, the file is intentionally skipped (the tmp file is cleaned up and execution continues). However, copied++ is still incremented on line 64, so the final log line — "sync-grammars: X wasm files → destDir" — counts skipped files as if they were successfully updated. This makes it harder to tell at a glance that some grammars were left at their previous version.

Consider tracking skipped files separately:

  try {
    fs.renameSync(tmp, dest);
    copied++;
  } catch (error) {
    ...
    if (...lock error...) {
      skipped++;
      console.warn(...);
    } else {
      throw error;
    }
  }

And update the final log to reflect both counts when skipped > 0.

@@ -0,0 +1 @@
pnpm test
Copy link

Choose a reason for hiding this comment

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

Missing shebang (inconsistent with pre-push)

The pre-push hook declares #!/usr/bin/env sh on its first line, but pre-commit doesn't. Without a shebang the kernel picks a default interpreter, which varies by OS/distro. It's safer and more explicit to be consistent:

Suggested change
pnpm test
#!/usr/bin/env sh
pnpm test

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b9381d7215

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

.husky/pre-push Outdated
. "$(dirname -- "$0")/_/husky.sh"

echo "Running pnpm format before push..."
pnpm format

Choose a reason for hiding this comment

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

P2 Badge Fail pre-push when formatting changes are required

Running pnpm format here rewrites files but still exits successfully, so a push can proceed even when the commit being pushed is unformatted (the formatting only affects the local working tree after the fact). In the common case where formatting issues exist in staged commits, this hook gives a false pass and does not enforce formatting on remote history.

Useful? React with 👍 / 👎.

// Best-effort cleanup; ignore if removal fails
}

if (error && typeof error === 'object' && 'code' in error && (error.code === 'EPERM' || error.code === 'EBUSY')) {

Choose a reason for hiding this comment

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

P2 Badge Treat EPERM as sync failure instead of lock warning

This branch suppresses all EPERM rename failures as if they were transient file locks, but EPERM also occurs for real permission/configuration problems (for example read-only worktrees or ACL restrictions). In those environments the script now reports success and continues with stale grammar binaries, masking a hard failure in prebuild/pretest flows.

Useful? React with 👍 / 👎.

- switch the pre-push hook to pnpm format:check so pushes fail before unformatted commits are sent
- remove deprecated husky bootstrap lines from the hook file for v9-style hooks
- report skipped grammar sync files separately from successful updates
@PatrickSys
Copy link
Owner Author

Addressed the valid review points in �6d95fc:

  • switched .husky/pre-push to pnpm format:check so pushes fail before unformatted commits are sent
  • removed the deprecated Husky bootstrap lines and kept the hook in Husky v9's plain-file format
  • fixed scripts/sync-grammars.mjs so skipped locked files are counted separately from successful updates

Re-ran pnpm test and pnpm run format:check locally before pushing.

Keep commits lightweight while enforcing formatting and tests before code is pushed.
@PatrickSys PatrickSys merged commit 91c7edc into master Mar 5, 2026
3 checks passed
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.

1 participant