Skip to content

fix(migrate): skip git hooks setup when already configured - #3118

Merged
fengmk2 merged 9 commits into
voidzero-dev:mainfrom
0froq:fix/migrate-existing-hooks-prompt
Oct 20, 2026
Merged

fengmk2 merged 9 commits into
voidzero-dev:mainfrom
0froq:fix/migrate-existing-hooks-prompt

Conversation

@0froq

@0froq 0froq commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

vp migrate previously prompted for hook setup when .vite-hooks/ already contained project hooks. It now skips complete setups and completes partial setups without prompting.

A complete setup requires project hooks and a prepare or postinstall script that runs vp config with hooks enabled. It also requires valid dispatcher files and core.hooksPath set to .vite-hooks/_ at the repository root. Partial setups use the existing safety checks and preserve project hook files. Explicit --hooks and --no-hooks options take precedence.

Lifecycle detection distinguishes actual commands from text such as echo vp config. When needed, setup removes --no-hooks from a recognized vp config command in prepare, or adds a vp config command. It preserves unrelated script text and avoids an extra prepare command when postinstall already enables hooks.

Hooks and Vitest migration share the existing parser for literal commands. Reuse of the native shell parser is tracked in #3136.

Closes #3096.

@fengmk2

fengmk2 commented Oct 19, 2026

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-19T15:22:53.670582Z 20f2aa0 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 6b89872cca

ℹ️ 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".

fengmk2 commented Oct 19, 2026

Copy link
Copy Markdown
Member

Could we remove the Keep/Skip prompt and handle existing hooks automatically?

  • If hooks are already fully configured, skip setup silently.
  • If hook files exist but setup is incomplete, preserve those files and complete setup through the existing checks.

The detection should distinguish these cases, since files in .vite-hooks/ alone do not mean the hooks are fully configured.

@0froq

0froq commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, that makes sense. I'll do that and update the tests to cover both cases and push to this PR.

@0froq 0froq changed the title fix(migrate): show existing git hooks and offer Keep/Skip fix(migrate): skip git hooks setup when already configured Oct 19, 2026
@fengmk2

fengmk2 commented Oct 19, 2026

Copy link
Copy Markdown
Member

@codex review

@0froq

0froq commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review!

Updated: I removed the Keep/Skip prompt. If hooks are fully configured (prepare runs vp config, core.hooksPath is .vite-hooks/_, and the dispatcher is installed), vp migrate now skips the hooks step and logs "Git hooks already configured, skipping". If .vite-hooks/ only has hook files and setup is incomplete, it keeps those files and finishes setup through the existing checks. Tests cover both cases plus the no-hooks and --hooks/--no-hooks paths. PTAL.

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

Copy link
Copy Markdown

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: 21d3b2140f

ℹ️ 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".

Comment thread packages/cli/src/migration/setup-plan.ts Outdated
Comment thread packages/cli/src/migration/migrator/git-hooks.ts Outdated
Comment thread packages/cli/src/migration/migrator/git-hooks.ts Outdated
@fengmk2

fengmk2 commented Oct 19, 2026

Copy link
Copy Markdown
Member

@codex review

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

Copy link
Copy Markdown

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: 20f2aa0bb5

ℹ️ 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".

Comment thread packages/cli/src/migration/migrator/git-hooks.ts Outdated
Comment thread packages/cli/src/migration/migrator/git-hooks.ts Outdated
@fengmk2 fengmk2 self-assigned this Oct 20, 2026

@fengmk2 fengmk2 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@0froq Thanks! I will make some simplify changes before I merge.

0froq and others added 7 commits October 20, 2026 13:15
Drop the Keep/Skip prompt for existing .vite-hooks/ hooks. When hooks are
fully configured (prepare runs `vp config`, core.hooksPath points at
.vite-hooks/_, and the dispatcher is installed), skip the hooks step without
asking. When hook files exist but setup is incomplete, keep them and finish
setup through the existing preflight checks.
- Print the skip message through utils/terminal.ts.
- Accept a postinstall script running `vp config` as well as prepare, and
  don't add a redundant prepare script when postinstall already runs it.
- On POSIX, treat a non-executable dispatcher or hook stub as incomplete.
- Compare .vite-hooks/_ dispatcher and stubs with what `vp config` writes,
  reusing hookScript and the exported HOOK_STUB_SCRIPT; stale content means
  setup is incomplete.
- Don't count `vp config --no-hooks` in prepare/postinstall as a dispatcher
  refresh, for both detection and the redundant-prepare check.
…epending

When prepare already runs `vp config --no-hooks`, drop the flag from that
invocation in place rather than adding a second `vp config` command.
@fengmk2
fengmk2 force-pushed the fix/migrate-existing-hooks-prompt branch from 7a79213 to d9f9d2f Compare October 20, 2026 05:16
@0froq

0froq commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review and for taking care of the cleanup! @fengmk2

@fengmk2
fengmk2 merged commit cffe33f into voidzero-dev:main Oct 20, 2026
62 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.

vp migrate --full asks to set up pre-commit hooks even when they're already configured

2 participants