From 00c7c5c94a9d0c68fb54c0673d3d0af9a441d5e0 Mon Sep 17 00:00:00 2001 From: 0froq Date: Sun, 18 Oct 2026 22:35:47 +0800 Subject: [PATCH 1/8] fix(migrate): show existing git hooks and offer Keep/Skip Closes #3096 --- .../__tests__/setup-plan-git-hooks.spec.ts | 90 +++++++++++++++++++ .../cli/src/migration/migrator/git-hooks.ts | 10 +++ packages/cli/src/migration/setup-plan.ts | 9 +- packages/cli/src/utils/prompts.ts | 30 +++++++ 4 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts diff --git a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts new file mode 100644 index 0000000000..4b330369e7 --- /dev/null +++ b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts @@ -0,0 +1,90 @@ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +import * as prompts from '@voidzero-dev/vite-plus-prompts'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { PackageManager } from '../../types/index.js'; + +vi.mock('@voidzero-dev/vite-plus-prompts', async (importOriginal) => { + const mod = await importOriginal(); + return { + ...mod, + select: vi.fn(), + confirm: vi.fn(), + log: { ...mod.log, info: vi.fn(), warn: vi.fn() }, + }; +}); + +const { collectMigrationSetupPlan } = await import('../setup-plan.js'); + +const mockSelect = vi.mocked(prompts.select); +const mockConfirm = vi.mocked(prompts.confirm); + +describe('collectMigrationSetupPlan existing Vite+ hooks', () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'vp-test-setup-plan-hooks-')); + fs.writeFileSync(path.join(tmpDir, 'package.json'), '{}'); + fs.mkdirSync(path.join(tmpDir, '.vite-hooks')); + fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'commit-msg'), 'echo msg\n'); + fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'pre-commit'), 'vp staged\n'); + fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'README.md'), 'not a hook\n'); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + vi.clearAllMocks(); + }); + + function collect(options: { interactive: boolean; hooks?: boolean }) { + return collectMigrationSetupPlan( + tmpDir, + PackageManager.pnpm, + { ...options, agent: false, editor: false }, + [], + false, + ); + } + + it('lists detected hooks and keeps them by default', async () => { + mockSelect.mockResolvedValueOnce('keep'); + + const plan = await collect({ interactive: true }); + + expect(plan.shouldSetupHooks).toBe(true); + expect(mockConfirm).not.toHaveBeenCalled(); + expect(mockSelect).toHaveBeenCalledTimes(1); + const [selectOptions] = mockSelect.mock.calls[0]; + expect(selectOptions.message).toBe( + 'Git hooks already configured in .vite-hooks/ (pre-commit, commit-msg)', + ); + expect(selectOptions.initialValue).toBe('keep'); + expect(selectOptions.options.map((option) => option.value)).toEqual(['keep', 'skip']); + }); + + it('skips git hooks setup when Skip is selected', async () => { + mockSelect.mockResolvedValueOnce('skip'); + + const plan = await collect({ interactive: true }); + + expect(plan.shouldSetupHooks).toBe(false); + }); + + it('keeps existing hooks without prompting in non-interactive mode', async () => { + const plan = await collect({ interactive: false }); + + expect(plan.shouldSetupHooks).toBe(true); + expect(mockSelect).not.toHaveBeenCalled(); + }); + + it.each([true, false])('bypasses the prompt when --hooks=%s is passed', async (hooks) => { + const plan = await collect({ interactive: true, hooks }); + + expect(plan.shouldSetupHooks).toBe(hooks); + expect(mockSelect).not.toHaveBeenCalled(); + expect(mockConfirm).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/migration/migrator/git-hooks.ts b/packages/cli/src/migration/migrator/git-hooks.ts index b465aed44f..fd4261dedc 100644 --- a/packages/cli/src/migration/migrator/git-hooks.ts +++ b/packages/cli/src/migration/migrator/git-hooks.ts @@ -86,6 +86,16 @@ export function hasExistingViteHooksPolicy(projectPath: string): boolean { .some((entry) => entry.isFile() && SUPPORTED_GIT_HOOK_NAME_SET.has(entry.name)); } +/** + * List the supported hook files present in `.vite-hooks/`, in canonical order. + */ +export function listExistingViteHookNames(projectPath: string): string[] { + const hooksDir = path.join(projectPath, '.vite-hooks'); + return SUPPORTED_GIT_HOOK_NAMES.filter((hookName) => + fs.lstatSync(path.join(hooksDir, hookName), { throwIfNoEntry: false })?.isFile(), + ); +} + function findNonRegularViteHookPath(projectPath: string): string | null { for (const hookName of SUPPORTED_GIT_HOOK_NAMES) { const hookPath = path.join(projectPath, '.vite-hooks', hookName); diff --git a/packages/cli/src/migration/setup-plan.ts b/packages/cli/src/migration/setup-plan.ts index c7abeeb1eb..cd9750c768 100644 --- a/packages/cli/src/migration/setup-plan.ts +++ b/packages/cli/src/migration/setup-plan.ts @@ -14,13 +14,15 @@ import { isJsonLikeFile, selectEditor, } from '../utils/editor.ts'; -import { cancelAndExit, promptGitHooks } from '../utils/prompts.ts'; +import { cancelAndExit, promptExistingGitHooks, promptGitHooks } from '../utils/prompts.ts'; import { confirmEslintMigration, confirmTsupMigration, detectEslintProject, detectIncompatibleEslintIntegration, detectTsupProject, + hasExistingViteHooksPolicy, + listExistingViteHookNames, preflightGitHooksSetup, warnIncompatibleEslintIntegration, warnLegacyEslintConfig, @@ -45,7 +47,10 @@ async function collectGitHooksDecision( options: MigrationOptions, packages: readonly WorkspacePackage[] = [], ): Promise { - let shouldSetupHooks = await promptGitHooks(options); + let shouldSetupHooks = + options.hooks === undefined && options.interactive && hasExistingViteHooksPolicy(rootDir) + ? await promptExistingGitHooks(listExistingViteHookNames(rootDir)) + : await promptGitHooks(options); if (shouldSetupHooks) { const reason = preflightGitHooksSetup(rootDir, packageManager, packages); if (reason) { diff --git a/packages/cli/src/utils/prompts.ts b/packages/cli/src/utils/prompts.ts index 2350798a36..e1a7f13e35 100644 --- a/packages/cli/src/utils/prompts.ts +++ b/packages/cli/src/utils/prompts.ts @@ -244,6 +244,36 @@ export async function upgradeYarn(cwd: string, interactive?: boolean, silent = f } } +/** + * Ask what to do with project-owned Vite+ hooks that already exist. + * Returns true to keep them (hook setup leaves existing hooks unchanged), + * false to skip git hooks setup entirely. + */ +export async function promptExistingGitHooks(hookNames: readonly string[]): Promise { + const hookList = hookNames.length > 0 ? ` (${hookNames.join(', ')})` : ''; + const action = await prompts.select({ + message: `Git hooks already configured in .vite-hooks/${hookList}`, + options: [ + { + label: 'Keep', + value: 'keep', + hint: 'Leave existing hooks unchanged', + }, + { + label: 'Skip', + value: 'skip', + hint: "Don't touch git hooks setup", + }, + ], + initialValue: 'keep', + }); + if (prompts.isCancel(action)) { + cancelAndExit(); + return false; + } + return action === 'keep'; +} + export async function promptGitHooks(options: { hooks?: boolean; interactive: boolean; From 02ad95f85f82067d00ed984e538a5170cea2dc72 Mon Sep 17 00:00:00 2001 From: 0froq Date: Mon, 19 Oct 2026 22:46:33 +0800 Subject: [PATCH 2/8] fix(migrate): skip git hooks setup when already configured 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. --- .../__tests__/setup-plan-git-hooks.spec.ts | 239 +++++++++++++++--- .../cli/src/migration/migrator/git-hooks.ts | 35 +++ packages/cli/src/migration/setup-plan.ts | 22 +- packages/cli/src/utils/prompts.ts | 30 --- 4 files changed, 260 insertions(+), 66 deletions(-) diff --git a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts index 4b330369e7..25b7eeaf7e 100644 --- a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts +++ b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts @@ -1,3 +1,4 @@ +import { spawnSync } from 'node:child_process'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -18,20 +19,54 @@ vi.mock('@voidzero-dev/vite-plus-prompts', async (importOriginal) => { }); const { collectMigrationSetupPlan } = await import('../setup-plan.js'); +const { installGitHooks, isViteHooksSetupComplete } = await import('../migrator.js'); const mockSelect = vi.mocked(prompts.select); const mockConfirm = vi.mocked(prompts.confirm); +const mockInfo = vi.mocked(prompts.log.info); + +const SKIP_MESSAGE = 'Git hooks already configured, skipping'; + +function git(cwd: string, ...args: string[]) { + const result = spawnSync('git', args, { cwd, stdio: 'pipe' }); + if (result.status !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${result.stderr.toString()}`); + } +} + +function writePackageJson(dir: string, pkg: Record) { + fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify(pkg)); +} + +function readPackageJson(dir: string): { scripts?: Record } { + return JSON.parse(fs.readFileSync(path.join(dir, 'package.json'), 'utf8')); +} + +/** Simulate what `vp config` leaves behind: dispatcher files and core.hooksPath. */ +function installDispatcher(dir: string, hookNames: string[]) { + const dispatcherDir = path.join(dir, '.vite-hooks', '_'); + fs.mkdirSync(dispatcherDir, { recursive: true }); + fs.writeFileSync(path.join(dispatcherDir, 'h'), '#!/usr/bin/env sh\n'); + for (const hookName of hookNames) { + fs.writeFileSync( + path.join(dispatcherDir, hookName), + '#!/usr/bin/env sh\n. "$(dirname "$0")/h"', + ); + } + git(dir, 'config', 'core.hooksPath', '.vite-hooks/_'); +} + +function expectNoHooksPrompt() { + expect(mockConfirm).not.toHaveBeenCalled(); + expect(mockSelect).not.toHaveBeenCalled(); +} describe('collectMigrationSetupPlan existing Vite+ hooks', () => { let tmpDir: string; beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'vp-test-setup-plan-hooks-')); - fs.writeFileSync(path.join(tmpDir, 'package.json'), '{}'); - fs.mkdirSync(path.join(tmpDir, '.vite-hooks')); - fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'commit-msg'), 'echo msg\n'); - fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'pre-commit'), 'vp staged\n'); - fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'README.md'), 'not a hook\n'); + writePackageJson(tmpDir, {}); }); afterEach(() => { @@ -39,6 +74,19 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { vi.clearAllMocks(); }); + function writeProjectHooks() { + fs.mkdirSync(path.join(tmpDir, '.vite-hooks'), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'commit-msg'), 'echo msg\n'); + fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'pre-commit'), 'npm test\n'); + } + + function setUpFullyConfiguredHooks() { + git(tmpDir, 'init', '-q'); + writePackageJson(tmpDir, { scripts: { prepare: 'vp config' } }); + writeProjectHooks(); + installDispatcher(tmpDir, ['commit-msg', 'pre-commit']); + } + function collect(options: { interactive: boolean; hooks?: boolean }) { return collectMigrationSetupPlan( tmpDir, @@ -49,42 +97,173 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { ); } - it('lists detected hooks and keeps them by default', async () => { - mockSelect.mockResolvedValueOnce('keep'); + describe('fully configured hooks', () => { + beforeEach(setUpFullyConfiguredHooks); - const plan = await collect({ interactive: true }); + it('is detected as complete', () => { + expect(isViteHooksSetupComplete(tmpDir)).toBe(true); + }); - expect(plan.shouldSetupHooks).toBe(true); - expect(mockConfirm).not.toHaveBeenCalled(); - expect(mockSelect).toHaveBeenCalledTimes(1); - const [selectOptions] = mockSelect.mock.calls[0]; - expect(selectOptions.message).toBe( - 'Git hooks already configured in .vite-hooks/ (pre-commit, commit-msg)', + it.each([true, false])( + 'skips the hooks step without prompting (interactive=%s)', + async (interactive) => { + const plan = await collect({ interactive }); + + expect(plan.shouldSetupHooks).toBe(false); + expectNoHooksPrompt(); + expect(mockInfo).toHaveBeenCalledWith(SKIP_MESSAGE); + }, ); - expect(selectOptions.initialValue).toBe('keep'); - expect(selectOptions.options.map((option) => option.value)).toEqual(['keep', 'skip']); - }); - it('skips git hooks setup when Skip is selected', async () => { - mockSelect.mockResolvedValueOnce('skip'); + it('still honors --no-hooks', async () => { + const plan = await collect({ interactive: true, hooks: false }); - const plan = await collect({ interactive: true }); + expect(plan.shouldSetupHooks).toBe(false); + expectNoHooksPrompt(); + expect(mockInfo).not.toHaveBeenCalledWith(SKIP_MESSAGE); + }); - expect(plan.shouldSetupHooks).toBe(false); + it('still runs setup when --hooks is passed', async () => { + const plan = await collect({ interactive: true, hooks: true }); + + expect(plan.shouldSetupHooks).toBe(true); + expectNoHooksPrompt(); + expect(mockInfo).not.toHaveBeenCalledWith(SKIP_MESSAGE); + }); }); - it('keeps existing hooks without prompting in non-interactive mode', async () => { - const plan = await collect({ interactive: false }); + describe('partially configured hooks', () => { + it.each([ + [ + 'no prepare script', + () => { + git(tmpDir, 'init', '-q'); + writeProjectHooks(); + installDispatcher(tmpDir, ['commit-msg', 'pre-commit']); + }, + ], + [ + 'core.hooksPath unset', + () => { + git(tmpDir, 'init', '-q'); + writePackageJson(tmpDir, { scripts: { prepare: 'vp config' } }); + writeProjectHooks(); + }, + ], + [ + 'dispatcher missing', + () => { + setUpFullyConfiguredHooks(); + fs.rmSync(path.join(tmpDir, '.vite-hooks', '_'), { recursive: true }); + }, + ], + [ + 'dispatcher stub missing for a project hook', + () => { + setUpFullyConfiguredHooks(); + fs.rmSync(path.join(tmpDir, '.vite-hooks', '_', 'commit-msg')); + }, + ], + [ + 'no git repository', + () => { + writePackageJson(tmpDir, { scripts: { prepare: 'vp config' } }); + writeProjectHooks(); + }, + ], + ])('completes setup without prompting when %s', async (_case, setUp) => { + setUp(); + expect(isViteHooksSetupComplete(tmpDir)).toBe(false); + + for (const interactive of [true, false]) { + const plan = await collect({ interactive }); + expect(plan.shouldSetupHooks).toBe(true); + } + expectNoHooksPrompt(); + expect(mockInfo).not.toHaveBeenCalledWith(SKIP_MESSAGE); + }); - expect(plan.shouldSetupHooks).toBe(true); - expect(mockSelect).not.toHaveBeenCalled(); + it('does not treat hook files alone as fully configured', () => { + writeProjectHooks(); + + expect(isViteHooksSetupComplete(tmpDir)).toBe(false); + }); + + it('preserves existing hook files while completing setup', async () => { + writeProjectHooks(); + + const plan = await collect({ interactive: true }); + expect(plan.shouldSetupHooks).toBe(true); + expect(installGitHooks(tmpDir, true)).toBe(true); + + expect(fs.readFileSync(path.join(tmpDir, '.vite-hooks', 'pre-commit'), 'utf8')).toBe( + 'npm test\n', + ); + expect(fs.readFileSync(path.join(tmpDir, '.vite-hooks', 'commit-msg'), 'utf8')).toBe( + 'echo msg\n', + ); + expect(readPackageJson(tmpDir).scripts?.prepare).toBe('vp config'); + expect(fs.existsSync(path.join(tmpDir, 'vite.config.ts'))).toBe(false); + }); + + it('still runs the existing preflight checks', async () => { + git(tmpDir, 'init', '-q'); + writeProjectHooks(); + git(tmpDir, 'config', 'core.hooksPath', '.githooks'); + + const plan = await collect({ interactive: true }); + + expect(plan.shouldSetupHooks).toBe(false); + expectNoHooksPrompt(); + expect(prompts.log.warn).toHaveBeenCalledWith( + expect.stringContaining('core.hooksPath is already set to ".githooks"'), + ); + }); + + it.each([true, false])('respects --hooks=%s', async (hooks) => { + writeProjectHooks(); + + const plan = await collect({ interactive: true, hooks }); + + expect(plan.shouldSetupHooks).toBe(hooks); + expectNoHooksPrompt(); + }); }); - it.each([true, false])('bypasses the prompt when --hooks=%s is passed', async (hooks) => { - const plan = await collect({ interactive: true, hooks }); + describe('no existing hooks', () => { + it('does not treat non-hook files in .vite-hooks/ as existing hooks', async () => { + fs.mkdirSync(path.join(tmpDir, '.vite-hooks')); + fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'README.md'), 'not a hook\n'); + mockConfirm.mockResolvedValueOnce(true); + + const plan = await collect({ interactive: true }); + + expect(plan.shouldSetupHooks).toBe(true); + expect(mockConfirm).toHaveBeenCalledTimes(1); + }); + + it.each([true, false])('asks the original confirm prompt (answer=%s)', async (answer) => { + mockConfirm.mockResolvedValueOnce(answer); + + const plan = await collect({ interactive: true }); + + expect(plan.shouldSetupHooks).toBe(answer); + expect(mockConfirm).toHaveBeenCalledTimes(1); + expect(mockSelect).not.toHaveBeenCalled(); + }); + + it('sets up hooks by default in non-interactive mode', async () => { + const plan = await collect({ interactive: false }); + + expect(plan.shouldSetupHooks).toBe(true); + expectNoHooksPrompt(); + }); + + it.each([true, false])('respects --hooks=%s without prompting', async (hooks) => { + const plan = await collect({ interactive: true, hooks }); - expect(plan.shouldSetupHooks).toBe(hooks); - expect(mockSelect).not.toHaveBeenCalled(); - expect(mockConfirm).not.toHaveBeenCalled(); + expect(plan.shouldSetupHooks).toBe(hooks); + expectNoHooksPrompt(); + }); }); }); diff --git a/packages/cli/src/migration/migrator/git-hooks.ts b/packages/cli/src/migration/migrator/git-hooks.ts index fd4261dedc..cd244d06bc 100644 --- a/packages/cli/src/migration/migrator/git-hooks.ts +++ b/packages/cli/src/migration/migrator/git-hooks.ts @@ -96,6 +96,41 @@ export function listExistingViteHookNames(projectPath: string): string[] { ); } +/** + * Whether Vite+ hooks are already fully set up for this project, i.e. running + * the hooks step again would have nothing left to do. Hook files in + * `.vite-hooks/` alone are not enough; full setup also requires: + * + * - a `prepare` script that runs `vp config` (refreshes the dispatcher), and + * - at the git repository root, `core.hooksPath` pointing at `.vite-hooks/_` + * with the generated dispatcher (`_/h`) and a stub for every project hook. + */ +export function isViteHooksSetupComplete(projectPath: string): boolean { + if (!hasExistingViteHooksPolicy(projectPath)) { + return false; + } + const packageJsonPath = path.join(projectPath, 'package.json'); + if (!fs.existsSync(packageJsonPath)) { + return false; + } + const pkg = readJsonFile(packageJsonPath) as { scripts?: Record }; + if (!pkg.scripts?.prepare?.includes('vp config')) { + return false; + } + const gitRoot = findGitRoot(projectPath); + if (!gitRoot || path.resolve(projectPath) !== path.resolve(gitRoot)) { + return false; + } + if (normalizeGitHooksPath(getConfiguredHooksPath(projectPath)) !== '.vite-hooks/_') { + return false; + } + const dispatcherDir = path.join(projectPath, '.vite-hooks', '_'); + return ['h', ...listExistingViteHookNames(projectPath)].every( + (name) => + fs.lstatSync(path.join(dispatcherDir, name), { throwIfNoEntry: false })?.isFile() ?? false, + ); +} + function findNonRegularViteHookPath(projectPath: string): string | null { for (const hookName of SUPPORTED_GIT_HOOK_NAMES) { const hookPath = path.join(projectPath, '.vite-hooks', hookName); diff --git a/packages/cli/src/migration/setup-plan.ts b/packages/cli/src/migration/setup-plan.ts index cd9750c768..5ce54a5c25 100644 --- a/packages/cli/src/migration/setup-plan.ts +++ b/packages/cli/src/migration/setup-plan.ts @@ -14,7 +14,7 @@ import { isJsonLikeFile, selectEditor, } from '../utils/editor.ts'; -import { cancelAndExit, promptExistingGitHooks, promptGitHooks } from '../utils/prompts.ts'; +import { cancelAndExit, promptGitHooks } from '../utils/prompts.ts'; import { confirmEslintMigration, confirmTsupMigration, @@ -22,7 +22,7 @@ import { detectIncompatibleEslintIntegration, detectTsupProject, hasExistingViteHooksPolicy, - listExistingViteHookNames, + isViteHooksSetupComplete, preflightGitHooksSetup, warnIncompatibleEslintIntegration, warnLegacyEslintConfig, @@ -47,10 +47,20 @@ async function collectGitHooksDecision( options: MigrationOptions, packages: readonly WorkspacePackage[] = [], ): Promise { - let shouldSetupHooks = - options.hooks === undefined && options.interactive && hasExistingViteHooksPolicy(rootDir) - ? await promptExistingGitHooks(listExistingViteHookNames(rootDir)) - : await promptGitHooks(options); + let shouldSetupHooks: boolean; + if (options.hooks === undefined && hasExistingViteHooksPolicy(rootDir)) { + // Project-owned hooks in .vite-hooks/ already express the intent to use + // Vite+ hooks, so don't ask again. + if (isViteHooksSetupComplete(rootDir)) { + prompts.log.info('Git hooks already configured, skipping'); + return false; + } + // Hook files exist but setup is incomplete: finish it through the usual + // checks. setupGitHooks keeps existing hook files unchanged. + shouldSetupHooks = true; + } else { + shouldSetupHooks = await promptGitHooks(options); + } if (shouldSetupHooks) { const reason = preflightGitHooksSetup(rootDir, packageManager, packages); if (reason) { diff --git a/packages/cli/src/utils/prompts.ts b/packages/cli/src/utils/prompts.ts index e1a7f13e35..2350798a36 100644 --- a/packages/cli/src/utils/prompts.ts +++ b/packages/cli/src/utils/prompts.ts @@ -244,36 +244,6 @@ export async function upgradeYarn(cwd: string, interactive?: boolean, silent = f } } -/** - * Ask what to do with project-owned Vite+ hooks that already exist. - * Returns true to keep them (hook setup leaves existing hooks unchanged), - * false to skip git hooks setup entirely. - */ -export async function promptExistingGitHooks(hookNames: readonly string[]): Promise { - const hookList = hookNames.length > 0 ? ` (${hookNames.join(', ')})` : ''; - const action = await prompts.select({ - message: `Git hooks already configured in .vite-hooks/${hookList}`, - options: [ - { - label: 'Keep', - value: 'keep', - hint: 'Leave existing hooks unchanged', - }, - { - label: 'Skip', - value: 'skip', - hint: "Don't touch git hooks setup", - }, - ], - initialValue: 'keep', - }); - if (prompts.isCancel(action)) { - cancelAndExit(); - return false; - } - return action === 'keep'; -} - export async function promptGitHooks(options: { hooks?: boolean; interactive: boolean; From 35782cb097f0086fe13e69874792439bfe1762c2 Mon Sep 17 00:00:00 2001 From: 0froq Date: Mon, 19 Oct 2026 23:08:00 +0800 Subject: [PATCH 3/8] fix(migrate): address review on hooks detection - 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. --- .../__tests__/setup-plan-git-hooks.spec.ts | 80 ++++++++++++++++--- .../cli/src/migration/migrator/git-hooks.ts | 39 ++++++--- packages/cli/src/migration/setup-plan.ts | 3 +- 3 files changed, 100 insertions(+), 22 deletions(-) diff --git a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts index 25b7eeaf7e..093afbff0b 100644 --- a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts +++ b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts @@ -18,14 +18,21 @@ vi.mock('@voidzero-dev/vite-plus-prompts', async (importOriginal) => { }; }); +vi.mock('../../utils/terminal.ts', async (importOriginal) => { + const mod = await importOriginal(); + return { ...mod, log: vi.fn() }; +}); + +const terminal = await import('../../utils/terminal.ts'); const { collectMigrationSetupPlan } = await import('../setup-plan.js'); const { installGitHooks, isViteHooksSetupComplete } = await import('../migrator.js'); const mockSelect = vi.mocked(prompts.select); const mockConfirm = vi.mocked(prompts.confirm); -const mockInfo = vi.mocked(prompts.log.info); +const mockLog = vi.mocked(terminal.log); const SKIP_MESSAGE = 'Git hooks already configured, skipping'; +const skipMessage = expect.stringContaining(SKIP_MESSAGE); function git(cwd: string, ...args: string[]) { const result = spawnSync('git', args, { cwd, stdio: 'pipe' }); @@ -46,12 +53,12 @@ function readPackageJson(dir: string): { scripts?: Record } { function installDispatcher(dir: string, hookNames: string[]) { const dispatcherDir = path.join(dir, '.vite-hooks', '_'); fs.mkdirSync(dispatcherDir, { recursive: true }); - fs.writeFileSync(path.join(dispatcherDir, 'h'), '#!/usr/bin/env sh\n'); + fs.writeFileSync(path.join(dispatcherDir, 'h'), '#!/usr/bin/env sh\n', { mode: 0o755 }); + fs.chmodSync(path.join(dispatcherDir, 'h'), 0o755); for (const hookName of hookNames) { - fs.writeFileSync( - path.join(dispatcherDir, hookName), - '#!/usr/bin/env sh\n. "$(dirname "$0")/h"', - ); + const stubPath = path.join(dispatcherDir, hookName); + fs.writeFileSync(stubPath, '#!/usr/bin/env sh\n. "$(dirname "$0")/h"', { mode: 0o755 }); + fs.chmodSync(stubPath, 0o755); } git(dir, 'config', 'core.hooksPath', '.vite-hooks/_'); } @@ -111,16 +118,26 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { expect(plan.shouldSetupHooks).toBe(false); expectNoHooksPrompt(); - expect(mockInfo).toHaveBeenCalledWith(SKIP_MESSAGE); + expect(mockLog).toHaveBeenCalledWith(skipMessage); }, ); + it('accepts a postinstall script that runs vp config', async () => { + writePackageJson(tmpDir, { scripts: { postinstall: 'vp config' } }); + + expect(isViteHooksSetupComplete(tmpDir)).toBe(true); + const plan = await collect({ interactive: true }); + expect(plan.shouldSetupHooks).toBe(false); + expectNoHooksPrompt(); + expect(mockLog).toHaveBeenCalledWith(skipMessage); + }); + it('still honors --no-hooks', async () => { const plan = await collect({ interactive: true, hooks: false }); expect(plan.shouldSetupHooks).toBe(false); expectNoHooksPrompt(); - expect(mockInfo).not.toHaveBeenCalledWith(SKIP_MESSAGE); + expect(mockLog).not.toHaveBeenCalledWith(skipMessage); }); it('still runs setup when --hooks is passed', async () => { @@ -128,7 +145,7 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { expect(plan.shouldSetupHooks).toBe(true); expectNoHooksPrompt(); - expect(mockInfo).not.toHaveBeenCalledWith(SKIP_MESSAGE); + expect(mockLog).not.toHaveBeenCalledWith(skipMessage); }); }); @@ -164,6 +181,20 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { fs.rmSync(path.join(tmpDir, '.vite-hooks', '_', 'commit-msg')); }, ], + [ + 'the dispatcher is not executable', + () => { + setUpFullyConfiguredHooks(); + fs.chmodSync(path.join(tmpDir, '.vite-hooks', '_', 'h'), 0o644); + }, + ], + [ + 'a dispatcher stub is not executable', + () => { + setUpFullyConfiguredHooks(); + fs.chmodSync(path.join(tmpDir, '.vite-hooks', '_', 'pre-commit'), 0o644); + }, + ], [ 'no git repository', () => { @@ -171,7 +202,11 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { writeProjectHooks(); }, ], - ])('completes setup without prompting when %s', async (_case, setUp) => { + ])('completes setup without prompting when %s', async (testCase, setUp) => { + if (process.platform === 'win32' && testCase.includes('not executable')) { + // Windows has no POSIX execute bits, so these cases do not apply. + return; + } setUp(); expect(isViteHooksSetupComplete(tmpDir)).toBe(false); @@ -180,7 +215,7 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { expect(plan.shouldSetupHooks).toBe(true); } expectNoHooksPrompt(); - expect(mockInfo).not.toHaveBeenCalledWith(SKIP_MESSAGE); + expect(mockLog).not.toHaveBeenCalledWith(skipMessage); }); it('does not treat hook files alone as fully configured', () => { @@ -206,6 +241,29 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { expect(fs.existsSync(path.join(tmpDir, 'vite.config.ts'))).toBe(false); }); + it('does not add a redundant prepare script when postinstall runs vp config', async () => { + writePackageJson(tmpDir, { scripts: { postinstall: 'vp config', prepare: 'tsc -b' } }); + writeProjectHooks(); + + const plan = await collect({ interactive: true }); + expect(plan.shouldSetupHooks).toBe(true); + expect(installGitHooks(tmpDir, true)).toBe(true); + + expect(readPackageJson(tmpDir).scripts).toEqual({ + postinstall: 'vp config', + prepare: 'tsc -b', + }); + }); + + it('prepends vp config to an existing prepare script', () => { + writePackageJson(tmpDir, { scripts: { prepare: 'tsc -b' } }); + writeProjectHooks(); + + expect(installGitHooks(tmpDir, true)).toBe(true); + + expect(readPackageJson(tmpDir).scripts?.prepare).toBe('vp config && tsc -b'); + }); + it('still runs the existing preflight checks', async () => { git(tmpDir, 'init', '-q'); writeProjectHooks(); diff --git a/packages/cli/src/migration/migrator/git-hooks.ts b/packages/cli/src/migration/migrator/git-hooks.ts index cd244d06bc..06ea66a2af 100644 --- a/packages/cli/src/migration/migrator/git-hooks.ts +++ b/packages/cli/src/migration/migrator/git-hooks.ts @@ -101,9 +101,11 @@ export function listExistingViteHookNames(projectPath: string): string[] { * the hooks step again would have nothing left to do. Hook files in * `.vite-hooks/` alone are not enough; full setup also requires: * - * - a `prepare` script that runs `vp config` (refreshes the dispatcher), and + * - a `prepare` or `postinstall` script that runs `vp config` (refreshes the + * dispatcher), and * - at the git repository root, `core.hooksPath` pointing at `.vite-hooks/_` - * with the generated dispatcher (`_/h`) and a stub for every project hook. + * with the generated dispatcher (`_/h`) and a stub for every project hook, + * all executable on POSIX (git ignores non-executable hooks). */ export function isViteHooksSetupComplete(projectPath: string): boolean { if (!hasExistingViteHooksPolicy(projectPath)) { @@ -114,7 +116,7 @@ export function isViteHooksSetupComplete(projectPath: string): boolean { return false; } const pkg = readJsonFile(packageJsonPath) as { scripts?: Record }; - if (!pkg.scripts?.prepare?.includes('vp config')) { + if (!hasVpConfigLifecycleScript(pkg.scripts)) { return false; } const gitRoot = findGitRoot(projectPath); @@ -125,9 +127,25 @@ export function isViteHooksSetupComplete(projectPath: string): boolean { return false; } const dispatcherDir = path.join(projectPath, '.vite-hooks', '_'); - return ['h', ...listExistingViteHookNames(projectPath)].every( - (name) => - fs.lstatSync(path.join(dispatcherDir, name), { throwIfNoEntry: false })?.isFile() ?? false, + return ['h', ...listExistingViteHookNames(projectPath)].every((name) => { + const stats = fs.lstatSync(path.join(dispatcherDir, name), { throwIfNoEntry: false }); + if (!stats?.isFile()) { + return false; + } + // `vp config` chmods the dispatcher files to 0755; git ignores hooks that + // are not executable. Windows has no POSIX execute bits to check. + return process.platform === 'win32' || (stats.mode & 0o111) !== 0; + }); +} + +/** + * Whether a lifecycle script that `vp config` supports (`prepare` or + * `postinstall`) already runs it to refresh the hook dispatcher. + */ +function hasVpConfigLifecycleScript(scripts: Record | undefined): boolean { + return ( + (scripts?.prepare?.includes('vp config') ?? false) || + (scripts?.postinstall?.includes('vp config') ?? false) ); } @@ -327,10 +345,11 @@ export function setupGitHooks( if (!pkg.scripts) { pkg.scripts = {}; } - if (!pkg.scripts.prepare) { - pkg.scripts.prepare = 'vp config'; - } else if (!pkg.scripts.prepare.includes('vp config')) { - pkg.scripts.prepare = `vp config && ${pkg.scripts.prepare}`; + // Leave scripts alone when `prepare` or `postinstall` already runs it. + if (!hasVpConfigLifecycleScript(pkg.scripts)) { + pkg.scripts.prepare = pkg.scripts.prepare + ? `vp config && ${pkg.scripts.prepare}` + : 'vp config'; } return pkg; diff --git a/packages/cli/src/migration/setup-plan.ts b/packages/cli/src/migration/setup-plan.ts index 5ce54a5c25..a205a83773 100644 --- a/packages/cli/src/migration/setup-plan.ts +++ b/packages/cli/src/migration/setup-plan.ts @@ -15,6 +15,7 @@ import { selectEditor, } from '../utils/editor.ts'; import { cancelAndExit, promptGitHooks } from '../utils/prompts.ts'; +import { log, muted } from '../utils/terminal.ts'; import { confirmEslintMigration, confirmTsupMigration, @@ -52,7 +53,7 @@ async function collectGitHooksDecision( // Project-owned hooks in .vite-hooks/ already express the intent to use // Vite+ hooks, so don't ask again. if (isViteHooksSetupComplete(rootDir)) { - prompts.log.info('Git hooks already configured, skipping'); + log(`${muted('•')} Git hooks already configured, skipping`); return false; } // Hook files exist but setup is incomplete: finish it through the usual From c5f4a9dbab2d8335d3cc43a02a48679d763257d1 Mon Sep 17 00:00:00 2001 From: 0froq Date: Mon, 19 Oct 2026 23:25:15 +0800 Subject: [PATCH 4/8] fix(migrate): validate hook dispatcher contents and vp config flags - 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. --- packages/cli/src/config/hooks.ts | 5 +- .../__tests__/setup-plan-git-hooks.spec.ts | 56 ++++++++++++++++++- .../cli/src/migration/migrator/git-hooks.ts | 53 +++++++++++++++--- 3 files changed, 102 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/config/hooks.ts b/packages/cli/src/config/hooks.ts index 3c475abdad..b67603c4d6 100644 --- a/packages/cli/src/config/hooks.ts +++ b/packages/cli/src/config/hooks.ts @@ -83,6 +83,9 @@ c=$? exit $c`; } +/** The per-hook stub written to `/_/`; it sources the `h` dispatcher. */ +export const HOOK_STUB_SCRIPT = '#!/usr/bin/env sh\n. "$(dirname "$0")/h"'; + export interface InstallResult { message: string; isError: boolean; @@ -569,7 +572,7 @@ export function install(dir?: string, options: InstallOptions = {}): InstallResu writeFileSync(internal('h'), hookScript(location.dir), { mode: 0o755 }); chmodSync(internal('h'), 0o755); for (const hook of SUPPORTED_GIT_HOOK_NAMES) { - writeFileSync(internal(hook), `#!/usr/bin/env sh\n. "$(dirname "$0")/h"`, { mode: 0o755 }); + writeFileSync(internal(hook), HOOK_STUB_SCRIPT, { mode: 0o755 }); chmodSync(internal(hook), 0o755); } if (!gitConfigLocalHas('core.hooksPath', location.target)) { diff --git a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts index 093afbff0b..3913b23300 100644 --- a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts +++ b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts @@ -6,6 +6,7 @@ import path from 'node:path'; import * as prompts from '@voidzero-dev/vite-plus-prompts'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { HOOK_STUB_SCRIPT, hookScript } from '../../config/hooks.ts'; import { PackageManager } from '../../types/index.js'; vi.mock('@voidzero-dev/vite-plus-prompts', async (importOriginal) => { @@ -53,11 +54,11 @@ function readPackageJson(dir: string): { scripts?: Record } { function installDispatcher(dir: string, hookNames: string[]) { const dispatcherDir = path.join(dir, '.vite-hooks', '_'); fs.mkdirSync(dispatcherDir, { recursive: true }); - fs.writeFileSync(path.join(dispatcherDir, 'h'), '#!/usr/bin/env sh\n', { mode: 0o755 }); + fs.writeFileSync(path.join(dispatcherDir, 'h'), hookScript('.vite-hooks'), { mode: 0o755 }); fs.chmodSync(path.join(dispatcherDir, 'h'), 0o755); for (const hookName of hookNames) { const stubPath = path.join(dispatcherDir, hookName); - fs.writeFileSync(stubPath, '#!/usr/bin/env sh\n. "$(dirname "$0")/h"', { mode: 0o755 }); + fs.writeFileSync(stubPath, HOOK_STUB_SCRIPT, { mode: 0o755 }); fs.chmodSync(stubPath, 0o755); } git(dir, 'config', 'core.hooksPath', '.vite-hooks/_'); @@ -132,6 +133,15 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { expect(mockLog).toHaveBeenCalledWith(skipMessage); }); + it.each(['tsc -b && vp config', 'vp config --no-hooks --hooks', 'pnpm exec vp config'])( + 'accepts a lifecycle script running %s', + (prepare) => { + writePackageJson(tmpDir, { scripts: { prepare } }); + + expect(isViteHooksSetupComplete(tmpDir)).toBe(true); + }, + ); + it('still honors --no-hooks', async () => { const plan = await collect({ interactive: true, hooks: false }); @@ -195,6 +205,36 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { fs.chmodSync(path.join(tmpDir, '.vite-hooks', '_', 'pre-commit'), 0o644); }, ], + [ + 'the dispatcher content is stale', + () => { + setUpFullyConfiguredHooks(); + fs.writeFileSync(path.join(tmpDir, '.vite-hooks', '_', 'h'), '#!/usr/bin/env sh\n'); + }, + ], + [ + 'a dispatcher stub content is stale', + () => { + setUpFullyConfiguredHooks(); + fs.writeFileSync(path.join(tmpDir, '.vite-hooks', '_', 'commit-msg'), 'exit 0\n'); + }, + ], + [ + 'prepare runs vp config --no-hooks', + () => { + setUpFullyConfiguredHooks(); + writePackageJson(tmpDir, { scripts: { prepare: 'vp config --no-hooks' } }); + }, + ], + [ + 'postinstall runs vp config --hooks --no-hooks', + () => { + setUpFullyConfiguredHooks(); + writePackageJson(tmpDir, { + scripts: { postinstall: 'tsc -b && vp config --hooks --no-hooks' }, + }); + }, + ], [ 'no git repository', () => { @@ -255,6 +295,18 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { }); }); + it('adds a refreshing prepare script when postinstall runs vp config --no-hooks', () => { + writePackageJson(tmpDir, { scripts: { postinstall: 'vp config --no-hooks' } }); + writeProjectHooks(); + + expect(installGitHooks(tmpDir, true)).toBe(true); + + expect(readPackageJson(tmpDir).scripts).toEqual({ + postinstall: 'vp config --no-hooks', + prepare: 'vp config', + }); + }); + it('prepends vp config to an existing prepare script', () => { writePackageJson(tmpDir, { scripts: { prepare: 'tsc -b' } }); writeProjectHooks(); diff --git a/packages/cli/src/migration/migrator/git-hooks.ts b/packages/cli/src/migration/migrator/git-hooks.ts index 06ea66a2af..210c862434 100644 --- a/packages/cli/src/migration/migrator/git-hooks.ts +++ b/packages/cli/src/migration/migrator/git-hooks.ts @@ -5,7 +5,12 @@ import * as prompts from '@voidzero-dev/vite-plus-prompts'; import spawn from 'cross-spawn'; import { rewriteScripts } from '../../../binding/index.js'; -import { findUnsafeHookInstallPath, SUPPORTED_GIT_HOOK_NAMES } from '../../config/hooks.ts'; +import { + findUnsafeHookInstallPath, + HOOK_STUB_SCRIPT, + hookScript, + SUPPORTED_GIT_HOOK_NAMES, +} from '../../config/hooks.ts'; import type { PackageManager, WorkspacePackage } from '../../types/index.ts'; import { documentationUrl } from '../../utils/documentation.ts'; import { findGitRoot } from '../../utils/git.ts'; @@ -105,7 +110,8 @@ export function listExistingViteHookNames(projectPath: string): string[] { * dispatcher), and * - at the git repository root, `core.hooksPath` pointing at `.vite-hooks/_` * with the generated dispatcher (`_/h`) and a stub for every project hook, - * all executable on POSIX (git ignores non-executable hooks). + * matching what `vp config` writes and executable on POSIX (git ignores + * non-executable hooks). */ export function isViteHooksSetupComplete(projectPath: string): boolean { if (!hasExistingViteHooksPolicy(projectPath)) { @@ -127,14 +133,26 @@ export function isViteHooksSetupComplete(projectPath: string): boolean { return false; } const dispatcherDir = path.join(projectPath, '.vite-hooks', '_'); - return ['h', ...listExistingViteHookNames(projectPath)].every((name) => { - const stats = fs.lstatSync(path.join(dispatcherDir, name), { throwIfNoEntry: false }); + const expectedFiles: [string, string][] = [ + ['h', hookScript('.vite-hooks')], + ...listExistingViteHookNames(projectPath).map((name): [string, string] => [ + name, + HOOK_STUB_SCRIPT, + ]), + ]; + return expectedFiles.every(([name, expectedContent]) => { + const filePath = path.join(dispatcherDir, name); + const stats = fs.lstatSync(filePath, { throwIfNoEntry: false }); if (!stats?.isFile()) { return false; } // `vp config` chmods the dispatcher files to 0755; git ignores hooks that // are not executable. Windows has no POSIX execute bits to check. - return process.platform === 'win32' || (stats.mode & 0o111) !== 0; + if (process.platform !== 'win32' && (stats.mode & 0o111) === 0) { + return false; + } + // Stale or modified dispatcher files are refreshed by running setup again. + return fs.readFileSync(filePath, 'utf8') === expectedContent; }); } @@ -143,10 +161,27 @@ export function isViteHooksSetupComplete(projectPath: string): boolean { * `postinstall`) already runs it to refresh the hook dispatcher. */ function hasVpConfigLifecycleScript(scripts: Record | undefined): boolean { - return ( - (scripts?.prepare?.includes('vp config') ?? false) || - (scripts?.postinstall?.includes('vp config') ?? false) - ); + return runsVpConfigHooks(scripts?.prepare) || runsVpConfigHooks(scripts?.postinstall); +} + +/** + * Whether a script runs `vp config` with hooks enabled. `--no-hooks` skips the + * dispatcher install, so it does not count; the last of `--hooks` / + * `--no-hooks` wins, as in the `vp config` argument parser. + */ +function runsVpConfigHooks(script: string | undefined): boolean { + if (!script) { + return false; + } + return script.split(/&&|\|\||;|\|/).some((command) => { + const tokens = command.trim().split(/\s+/); + const start = tokens.findIndex((token, i) => token === 'vp' && tokens[i + 1] === 'config'); + if (start === -1) { + return false; + } + const hookFlags = tokens.slice(start + 2).filter((t) => t === '--hooks' || t === '--no-hooks'); + return hookFlags.at(-1) !== '--no-hooks'; + }); } function findNonRegularViteHookPath(projectPath: string): string | null { From 2e7417218a57d911181814dab748f39e8ec63a95 Mon Sep 17 00:00:00 2001 From: 0froq Date: Mon, 19 Oct 2026 23:30:50 +0800 Subject: [PATCH 5/8] fix(migrate): re-enable vp config --no-hooks in prepare instead of prepending When prepare already runs `vp config --no-hooks`, drop the flag from that invocation in place rather than adding a second `vp config` command. --- .../__tests__/setup-plan-git-hooks.spec.ts | 19 ++++++++++ .../cli/src/migration/migrator/git-hooks.ts | 37 ++++++++++++++++--- 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts index 3913b23300..f18e4d60f4 100644 --- a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts +++ b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts @@ -307,6 +307,25 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { }); }); + it.each([ + ['vp config --no-hooks', 'vp config'], + ['tsc -b && vp config --no-hooks', 'tsc -b && vp config'], + [ + 'vp config --no-hooks --hooks-dir .vite-hooks --no-agent && tsc -b', + 'vp config --hooks-dir .vite-hooks --no-agent && tsc -b', + ], + ])('rewrites prepare %j in place to re-enable hooks', (prepare, expected) => { + writePackageJson(tmpDir, { scripts: { prepare, postinstall: 'node setup.js' } }); + writeProjectHooks(); + + expect(installGitHooks(tmpDir, true)).toBe(true); + + expect(readPackageJson(tmpDir).scripts).toEqual({ + prepare: expected, + postinstall: 'node setup.js', + }); + }); + it('prepends vp config to an existing prepare script', () => { writePackageJson(tmpDir, { scripts: { prepare: 'tsc -b' } }); writeProjectHooks(); diff --git a/packages/cli/src/migration/migrator/git-hooks.ts b/packages/cli/src/migration/migrator/git-hooks.ts index 210c862434..1a608e0630 100644 --- a/packages/cli/src/migration/migrator/git-hooks.ts +++ b/packages/cli/src/migration/migrator/git-hooks.ts @@ -173,9 +173,9 @@ function runsVpConfigHooks(script: string | undefined): boolean { if (!script) { return false; } - return script.split(/&&|\|\||;|\|/).some((command) => { + return script.split(SCRIPT_COMMAND_SEPARATOR).some((command) => { const tokens = command.trim().split(/\s+/); - const start = tokens.findIndex((token, i) => token === 'vp' && tokens[i + 1] === 'config'); + const start = findVpConfigToken(tokens); if (start === -1) { return false; } @@ -184,6 +184,30 @@ function runsVpConfigHooks(script: string | undefined): boolean { }); } +const SCRIPT_COMMAND_SEPARATOR = /(&&|\|\||;|\|)/; + +function findVpConfigToken(tokens: string[]): number { + return tokens.findIndex((token, i) => token === 'vp' && tokens[i + 1] === 'config'); +} + +/** + * Drop `--no-hooks` from every `vp config` invocation in a script, keeping + * other commands and flags intact. Returns null when the script has no + * `vp config` invocation to rewrite. + */ +function enableVpConfigHooks(script: string): string | null { + let rewritten = false; + const parts = script.split(SCRIPT_COMMAND_SEPARATOR).map((part, index) => { + // Odd indexes are the captured separators. + if (index % 2 === 1 || findVpConfigToken(part.trim().split(/\s+/)) === -1) { + return part; + } + rewritten = true; + return part.replace(/\s+--no-hooks(?=\s|$)/g, ''); + }); + return rewritten ? parts.join('') : null; +} + function findNonRegularViteHookPath(projectPath: string): string | null { for (const hookName of SUPPORTED_GIT_HOOK_NAMES) { const hookPath = path.join(projectPath, '.vite-hooks', hookName); @@ -382,9 +406,12 @@ export function setupGitHooks( } // Leave scripts alone when `prepare` or `postinstall` already runs it. if (!hasVpConfigLifecycleScript(pkg.scripts)) { - pkg.scripts.prepare = pkg.scripts.prepare - ? `vp config && ${pkg.scripts.prepare}` - : 'vp config'; + // Re-enable an existing `vp config --no-hooks` in prepare rather than + // adding a second `vp config` invocation. + const enabledPrepare = pkg.scripts.prepare && enableVpConfigHooks(pkg.scripts.prepare); + pkg.scripts.prepare = + enabledPrepare || + (pkg.scripts.prepare ? `vp config && ${pkg.scripts.prepare}` : 'vp config'); } return pkg; From 4bcab50e873e0cee78f653b580b5939b36929d5a Mon Sep 17 00:00:00 2001 From: MK Date: Tue, 20 Oct 2026 12:03:36 +0800 Subject: [PATCH 6/8] fix(migrate): safely detect existing hook setup --- .../.vite-hooks/commit-msg | 1 + .../.vite-hooks/pre-commit | 1 + .../package.json | 10 ++ .../snapshots.toml | 13 +++ .../migration_existing_vite_hooks.md | 79 +++++++++++++ .../__tests__/setup-plan-git-hooks.spec.ts | 72 ++++++++++-- .../cli/src/migration/migrator/git-hooks.ts | 100 +++++++++++----- .../cli/src/migration/vitest-v5/commands.ts | 108 ++---------------- packages/cli/src/utils/shell-command.ts | 107 +++++++++++++++++ 9 files changed, 358 insertions(+), 133 deletions(-) create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/.vite-hooks/commit-msg create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/.vite-hooks/pre-commit create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/package.json create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/snapshots.toml create mode 100644 crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/snapshots/migration_existing_vite_hooks.md create mode 100644 packages/cli/src/utils/shell-command.ts diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/.vite-hooks/commit-msg b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/.vite-hooks/commit-msg new file mode 100644 index 0000000000..90defd2f5d --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/.vite-hooks/commit-msg @@ -0,0 +1 @@ +echo project-owned commit-msg diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/.vite-hooks/pre-commit b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/.vite-hooks/pre-commit new file mode 100644 index 0000000000..3080a51a98 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/.vite-hooks/pre-commit @@ -0,0 +1 @@ +echo project-owned pre-commit diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/package.json b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/package.json new file mode 100644 index 0000000000..d16354968d --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/package.json @@ -0,0 +1,10 @@ +{ + "name": "migration-existing-vite-hooks", + "packageManager": "pnpm@12.11.2", + "scripts": { + "prepare": "echo vp config --no-hooks" + }, + "devDependencies": { + "vite-plus": "latest" + } +} diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/snapshots.toml b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/snapshots.toml new file mode 100644 index 0000000000..5e771f9e6c --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/snapshots.toml @@ -0,0 +1,13 @@ +[[case]] +name = "migration_existing_vite_hooks" +vp = "global" +steps = [ + { argv = ["git", "init"], snapshot = false }, + { argv = ["pnpm", "--version"], snapshot = false }, + { argv = ["vp", "migrate", "--full", "--no-agent", "--no-editor"], comment = "Complete the existing hook setup without prompting, replacing project hooks, or changing echo arguments." }, + { argv = ["vpt", "print-file", "package.json", ".vite-hooks/pre-commit", ".vite-hooks/commit-msg"] }, + { argv = ["git", "config", "--local", "core.hooksPath"] }, + { argv = ["vpt", "stat-file", ".vite-hooks/_/h", "--assert", "file"] }, + { argv = ["vp", "migrate", "--full", "--no-agent", "--no-editor"], comment = "A complete hook setup is detected and skipped without prompting on the next migration." }, + { argv = ["vpt", "print-file", "package.json", ".vite-hooks/pre-commit", ".vite-hooks/commit-msg"] }, +] diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/snapshots/migration_existing_vite_hooks.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/snapshots/migration_existing_vite_hooks.md new file mode 100644 index 0000000000..5b12ffce10 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_existing_vite_hooks/snapshots/migration_existing_vite_hooks.md @@ -0,0 +1,79 @@ +# migration_existing_vite_hooks + +## `git init` + + +## `pnpm --version` + + +## `vp migrate --full --no-agent --no-editor` + +Complete the existing hook setup without prompting, replacing project hooks, or changing echo arguments. + +``` +VITE+ - The Unified Toolchain for the Web + +◇ Updated . to Vite+ +• Node pnpm +• Dependencies: + vite-plus latest → + vite → +• Git hooks configured +• Package manager settings configured +``` + +## `vpt print-file package.json .vite-hooks/pre-commit .vite-hooks/commit-msg` + +``` +{ + "name": "migration-existing-vite-hooks", + "packageManager": "pnpm@12.11.2", + "scripts": { + "prepare": "vp config && echo vp config --no-hooks" + }, + "devDependencies": { + "vite-plus": "catalog:" + } +} +echo project-owned pre-commit +echo project-owned commit-msg +``` + +## `git config --local core.hooksPath` + +``` +.vite-hooks/_ +``` + +## `vpt stat-file .vite-hooks/_/h --assert file` + +``` +.vite-hooks/_/h: file +``` + +## `vp migrate --full --no-agent --no-editor` + +A complete hook setup is detected and skipped without prompting on the next migration. + +``` +VITE+ - The Unified Toolchain for the Web + +This project is already using Vite+! Happy coding! +``` + +## `vpt print-file package.json .vite-hooks/pre-commit .vite-hooks/commit-msg` + +``` +{ + "name": "migration-existing-vite-hooks", + "packageManager": "pnpm@12.11.2", + "scripts": { + "prepare": "vp config && echo vp config --no-hooks" + }, + "devDependencies": { + "vite-plus": "catalog:" + } +} +echo project-owned pre-commit +echo project-owned commit-msg +``` diff --git a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts index f18e4d60f4..d57c22e566 100644 --- a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts +++ b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts @@ -133,14 +133,34 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { expect(mockLog).toHaveBeenCalledWith(skipMessage); }); - it.each(['tsc -b && vp config', 'vp config --no-hooks --hooks', 'pnpm exec vp config'])( - 'accepts a lifecycle script running %s', - (prepare) => { - writePackageJson(tmpDir, { scripts: { prepare } }); + it.each([ + 'tsc -b && vp config', + 'vp config --no-hooks --hooks', + 'pnpm exec vp config', + 'vp "config" --hooks-dir="./.vite-hooks"', + 'echo "build; done" && vp config', + ])('accepts a lifecycle script running %s', (prepare) => { + writePackageJson(tmpDir, { scripts: { prepare } }); - expect(isViteHooksSetupComplete(tmpDir)).toBe(true); - }, - ); + expect(isViteHooksSetupComplete(tmpDir)).toBe(true); + }); + + it.each([ + 'echo vp config', + 'node setup.js vp config', + 'echo "build && vp config && done"', + 'vp config "--no-hooks"', + 'vp config --help', + 'vp config --hooks-dir .custom-hooks', + 'VP_GIT_HOOKS=0 vp config', + 'cd packages/app && vp config', + ])('does not count %j as a hook refresh', async (prepare) => { + writePackageJson(tmpDir, { scripts: { prepare } }); + + expect(isViteHooksSetupComplete(tmpDir)).toBe(false); + expect((await collect({ interactive: true })).shouldSetupHooks).toBe(true); + expectNoHooksPrompt(); + }); it('still honors --no-hooks', async () => { const plan = await collect({ interactive: true, hooks: false }); @@ -310,6 +330,10 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { it.each([ ['vp config --no-hooks', 'vp config'], ['tsc -b && vp config --no-hooks', 'tsc -b && vp config'], + [ + 'echo "build && --no-hooks" && vp "config" "--no-hooks" && echo --no-hooks', + 'echo "build && --no-hooks" && vp "config" && echo --no-hooks', + ], [ 'vp config --no-hooks --hooks-dir .vite-hooks --no-agent && tsc -b', 'vp config --hooks-dir .vite-hooks --no-agent && tsc -b', @@ -349,6 +373,40 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { ); }); + it('runs preflight when the dispatcher directory is a regular file', async () => { + setUpFullyConfiguredHooks(); + const dispatcherDir = path.join(tmpDir, '.vite-hooks', '_'); + fs.rmSync(dispatcherDir, { recursive: true }); + fs.writeFileSync(dispatcherDir, 'preserve this file'); + + expect(isViteHooksSetupComplete(tmpDir)).toBe(false); + const plan = await collect({ interactive: true }); + + expect(plan.shouldSetupHooks).toBe(false); + expectNoHooksPrompt(); + expect(prompts.log.warn).toHaveBeenCalledWith( + expect.stringContaining('Git hook dispatcher path ".vite-hooks/_" is unsafe'), + ); + expect(fs.readFileSync(dispatcherDir, 'utf8')).toBe('preserve this file'); + }); + + it.each([ + 'echo vp config --no-hooks', + 'node setup.js vp config --no-hooks', + 'echo "build && vp config --no-hooks && done"', + 'cd tools && vp config', + 'VP_GIT_HOOKS=0 vp config', + 'echo "$BUILD_STATUS"', + ])('preserves arguments in %j when adding a refresh command', (prepare) => { + writePackageJson(tmpDir, { scripts: { prepare } }); + writeProjectHooks(); + + expect(installGitHooks(tmpDir, true)).toBe(true); + expect(readPackageJson(tmpDir).scripts?.prepare).toBe(`vp config && ${prepare}`); + expect(installGitHooks(tmpDir, true)).toBe(true); + expect(readPackageJson(tmpDir).scripts?.prepare).toBe(`vp config && ${prepare}`); + }); + it.each([true, false])('respects --hooks=%s', async (hooks) => { writeProjectHooks(); diff --git a/packages/cli/src/migration/migrator/git-hooks.ts b/packages/cli/src/migration/migrator/git-hooks.ts index 1a608e0630..72b5fa5b1e 100644 --- a/packages/cli/src/migration/migrator/git-hooks.ts +++ b/packages/cli/src/migration/migrator/git-hooks.ts @@ -4,7 +4,7 @@ import path from 'node:path'; import * as prompts from '@voidzero-dev/vite-plus-prompts'; import spawn from 'cross-spawn'; -import { rewriteScripts } from '../../../binding/index.js'; +import { parseConfigArgs, rewriteScripts } from '../../../binding/index.js'; import { findUnsafeHookInstallPath, HOOK_STUB_SCRIPT, @@ -15,6 +15,7 @@ import type { PackageManager, WorkspacePackage } from '../../types/index.ts'; import { documentationUrl } from '../../utils/documentation.ts'; import { findGitRoot } from '../../utils/git.ts'; import { editJsonFile, isJsonFile, readJsonFile } from '../../utils/json.ts'; +import { type CommandToken, literalArgv, literalCommands } from '../../utils/shell-command.ts'; import { hasStagedConfigInViteConfig, mergeStagedConfigToViteConfig, @@ -132,6 +133,9 @@ export function isViteHooksSetupComplete(projectPath: string): boolean { if (normalizeGitHooksPath(getConfiguredHooksPath(projectPath)) !== '.vite-hooks/_') { return false; } + if (findUnsafeHookInstallPath(projectPath, '.vite-hooks')) { + return false; + } const dispatcherDir = path.join(projectPath, '.vite-hooks', '_'); const expectedFiles: [string, string][] = [ ['h', hookScript('.vite-hooks')], @@ -170,24 +174,66 @@ function hasVpConfigLifecycleScript(scripts: Record | undefined) * `--no-hooks` wins, as in the `vp config` argument parser. */ function runsVpConfigHooks(script: string | undefined): boolean { - if (!script) { - return false; - } - return script.split(SCRIPT_COMMAND_SEPARATOR).some((command) => { - const tokens = command.trim().split(/\s+/); - const start = findVpConfigToken(tokens); - if (start === -1) { - return false; - } - const hookFlags = tokens.slice(start + 2).filter((t) => t === '--hooks' || t === '--no-hooks'); - return hookFlags.at(-1) !== '--no-hooks'; - }); + return vpConfigHookCommands(script).some( + (args) => + args.findLast(({ value }) => value === '--hooks' || value === '--no-hooks')?.value !== + '--no-hooks', + ); } -const SCRIPT_COMMAND_SEPARATOR = /(&&|\|\||;|\|)/; - -function findVpConfigToken(tokens: string[]): number { - return tokens.findIndex((token, i) => token === 'vp' && tokens[i + 1] === 'config'); +/** Only recognize literal invocations in the project directory. Text passed + * to echo, node, or another command must not count as a dispatcher refresh. */ +function vpConfigHookCommands(script: string | undefined): CommandToken[][] { + let commands = script ? literalCommands(script) : undefined; + // Preserve a leading refresh even when later commands need shell evaluation. + // setupGitHooks prepends this form when it cannot rewrite an existing script. + if (!commands && script?.includes('&&')) { + const first = literalArgv(script.slice(0, script.indexOf('&&'))); + commands = first?.length ? [first] : undefined; + } + if (!commands) { + return []; + } + const result: CommandToken[][] = []; + for (const tokens of commands) { + const values = tokens.map(({ value }) => value); + let start = 0; + if (/^[A-Za-z_]\w*=/.test(values[0] ?? '')) { + // Environment overrides can disable hooks or redirect Git operations. + return result; + } + if ( + ['cd', 'pushd', 'popd', '.', 'source', 'eval', 'export', 'set', 'unset'].includes(values[0]) + ) { + return result; + } + if (['pnpm', 'npm', 'yarn', 'bun', 'npx', 'bunx'].includes(values[0])) { + start++; + if (values[start] === 'exec' || values[start] === 'x') { + start++; + } + if (values[start] === '--') { + start++; + } + } + if (values[start] !== 'vp' || values[start + 1] !== 'config') { + continue; + } + const args = tokens.slice(start + 2); + // The native parser prints help; do not invoke it for help-only scripts. + if (args.some(({ value }) => value === '--help' || value === '-h')) { + continue; + } + const parsed = parseConfigArgs(args.map(({ value }) => value)); + if ( + parsed.status === 'ok' && + (parsed.value.hooksDir === undefined || + normalizeGitHooksPath(parsed.value.hooksDir) === '.vite-hooks') + ) { + result.push(args); + } + } + return result; } /** @@ -196,16 +242,16 @@ function findVpConfigToken(tokens: string[]): number { * `vp config` invocation to rewrite. */ function enableVpConfigHooks(script: string): string | null { - let rewritten = false; - const parts = script.split(SCRIPT_COMMAND_SEPARATOR).map((part, index) => { - // Odd indexes are the captured separators. - if (index % 2 === 1 || findVpConfigToken(part.trim().split(/\s+/)) === -1) { - return part; - } - rewritten = true; - return part.replace(/\s+--no-hooks(?=\s|$)/g, ''); - }); - return rewritten ? parts.join('') : null; + const flags = vpConfigHookCommands(script) + .flat() + .filter(({ value }) => value === '--no-hooks'); + if (!flags.length) { + return null; + } + for (const flag of flags.toReversed()) { + script = script.slice(0, flag.start).trimEnd() + script.slice(flag.end); + } + return script; } function findNonRegularViteHookPath(projectPath: string): string | null { diff --git a/packages/cli/src/migration/vitest-v5/commands.ts b/packages/cli/src/migration/vitest-v5/commands.ts index 20e17d8eca..a3d213f620 100644 --- a/packages/cli/src/migration/vitest-v5/commands.ts +++ b/packages/cli/src/migration/vitest-v5/commands.ts @@ -1,76 +1,14 @@ +import { literalArgv, literalCommands } from '../../utils/shell-command.ts'; import type { RewriteResult, VitestV5Finding } from './ast.ts'; -interface CommandToken { - value: string; - start: number; - end: number; -} - -/** Parse only a single literal argv command. Shell expansion, pipelines, and - * compound commands need review; treating them as argv could change behavior. */ -export function literalArgv(command: string): CommandToken[] | undefined { - const tokens: CommandToken[] = []; - let value = ''; - let start = 0; - let quote = ''; - let active = false; - for (let i = 0; i < command.length; i++) { - const character = command[i]; - if (quote) { - if (character === quote) { - quote = ''; - } else if ( - character === '\\' || - (quote === '"' && (character === '$' || character === '`')) - ) { - return undefined; - } else { - value += character; - } - } else if (character === "'" || character === '"') { - if (!active) { - start = i; - } - quote = character; - active = true; - } else if (character === '\n' || character === '\r') { - return undefined; - } else if (/\s/.test(character)) { - if (active) { - tokens.push({ value, start, end: i }); - } - active = false; - value = ''; - } else if (/[|&;<>($`\\)]/.test(character)) { - return undefined; - } else { - if (!active) { - start = i; - } - value += character; - active = true; - } - } - if (quote) { +/** Read test commands without evaluating shell state or directory changes. */ +export function literalTestCommands(command: string): string[][] | undefined { + const commands = literalCommands(command); + if (!commands) { return undefined; } - if (active) { - tokens.push({ value, start, end: command.length }); - } - return tokens; -} - -/** Read command lists without executing a shell. Directory-changing builtins, - * expansions, pipelines, and shell programs remain unresolved. */ -export function literalTestCommands(command: string): string[][] | undefined { - const commands: string[][] = []; - let quote = ''; - let start = 0; - const append = (end: number) => { - const tokens = literalArgv(command.slice(start, end)); - if (!tokens?.length) { - return false; - } + const result: string[][] = []; + for (const tokens of commands) { const values = tokens.map(({ value }) => value); while (/^[A-Za-z_]\w*=/.test(values[0] ?? '')) { values.shift(); @@ -79,39 +17,11 @@ export function literalTestCommands(command: string): string[][] | undefined { !values.length || ['cd', 'pushd', 'popd', '.', 'source', 'eval', 'export', 'set', 'unset'].includes(values[0]) ) { - return false; - } - commands.push(values); - return true; - }; - for (let index = 0; index < command.length; index++) { - const char = command[index]; - if (char === '\\') { return undefined; } - if (quote) { - if (char === quote) { - quote = ''; - } - } else if (char === '"' || char === "'") { - quote = char; - } else if (char === '#') { - return undefined; - } else if ( - char === ';' || - command.slice(index, index + 2) === '&&' || - command.slice(index, index + 2) === '||' - ) { - if (!append(index)) { - return undefined; - } - if (char !== ';') { - index++; - } - start = index + 1; - } + result.push(values); } - return !quote && append(command.length) ? commands : undefined; + return result; } /** Locate the runner's arguments after a supported package-manager wrapper. */ diff --git a/packages/cli/src/utils/shell-command.ts b/packages/cli/src/utils/shell-command.ts new file mode 100644 index 0000000000..55529f631f --- /dev/null +++ b/packages/cli/src/utils/shell-command.ts @@ -0,0 +1,107 @@ +export interface CommandToken { + value: string; + start: number; + end: number; +} + +/** Parse only a single literal argv command. Shell expansion, pipelines, and + * compound commands need review; treating them as argv could change behavior. */ +export function literalArgv(command: string): CommandToken[] | undefined { + const tokens: CommandToken[] = []; + let value = ''; + let start = 0; + let quote = ''; + let active = false; + for (let i = 0; i < command.length; i++) { + const character = command[i]; + if (quote) { + if (character === quote) { + quote = ''; + } else if ( + character === '\\' || + (quote === '"' && (character === '$' || character === '`')) + ) { + return undefined; + } else { + value += character; + } + } else if (character === "'" || character === '"') { + if (!active) { + start = i; + } + quote = character; + active = true; + } else if (character === '\n' || character === '\r') { + return undefined; + } else if (/\s/.test(character)) { + if (active) { + tokens.push({ value, start, end: i }); + } + active = false; + value = ''; + } else if (/[|&;<>($`\\)]/.test(character)) { + return undefined; + } else { + if (!active) { + start = i; + } + value += character; + active = true; + } + } + if (quote) { + return undefined; + } + if (active) { + tokens.push({ value, start, end: command.length }); + } + return tokens; +} + +/** Read literal command lists without executing a shell. Token offsets refer + * to the original script so callers can preserve its quoting and spacing. */ +export function literalCommands(command: string): CommandToken[][] | undefined { + const commands: CommandToken[][] = []; + let quote = ''; + let start = 0; + const append = (end: number) => { + const tokens = literalArgv(command.slice(start, end)); + if (!tokens?.length) { + return false; + } + for (const token of tokens) { + token.start += start; + token.end += start; + } + commands.push(tokens); + return true; + }; + for (let index = 0; index < command.length; index++) { + const char = command[index]; + if (char === '\\') { + return undefined; + } + if (quote) { + if (char === quote) { + quote = ''; + } + } else if (char === '"' || char === "'") { + quote = char; + } else if (char === '#') { + return undefined; + } else if ( + char === ';' || + command.slice(index, index + 2) === '&&' || + command.slice(index, index + 2) === '||' + ) { + if (!append(index)) { + return undefined; + } + if (char !== ';') { + index++; + } + start = index + 1; + } + } + return !quote && append(command.length) ? commands : undefined; +} From d9f9d2f194577dfe94b8476ded70d24ec4e109d9 Mon Sep 17 00:00:00 2001 From: MK Date: Tue, 20 Oct 2026 12:18:54 +0800 Subject: [PATCH 7/8] refactor(migrate): simplify existing hook detection --- .../__tests__/setup-plan-git-hooks.spec.ts | 57 +++++----- .../cli/src/migration/migrator/git-hooks.ts | 107 +++++++----------- packages/cli/src/migration/setup-plan.ts | 34 +++--- .../cli/src/migration/vitest-v5/commands.ts | 13 +-- packages/cli/src/utils/shell-command.ts | 32 ++++-- 5 files changed, 106 insertions(+), 137 deletions(-) diff --git a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts index d57c22e566..11c7069354 100644 --- a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts +++ b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts @@ -7,7 +7,10 @@ import * as prompts from '@voidzero-dev/vite-plus-prompts'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { HOOK_STUB_SCRIPT, hookScript } from '../../config/hooks.ts'; -import { PackageManager } from '../../types/index.js'; +import { PackageManager } from '../../types/index.ts'; +import { log } from '../../utils/terminal.ts'; +import { installGitHooks, isViteHooksSetupComplete } from '../migrator.ts'; +import { collectMigrationSetupPlan, type MigrationSetupPlan } from '../setup-plan.ts'; vi.mock('@voidzero-dev/vite-plus-prompts', async (importOriginal) => { const mod = await importOriginal(); @@ -24,25 +27,20 @@ vi.mock('../../utils/terminal.ts', async (importOriginal) => { return { ...mod, log: vi.fn() }; }); -const terminal = await import('../../utils/terminal.ts'); -const { collectMigrationSetupPlan } = await import('../setup-plan.js'); -const { installGitHooks, isViteHooksSetupComplete } = await import('../migrator.js'); - const mockSelect = vi.mocked(prompts.select); const mockConfirm = vi.mocked(prompts.confirm); -const mockLog = vi.mocked(terminal.log); +const mockLog = vi.mocked(log); -const SKIP_MESSAGE = 'Git hooks already configured, skipping'; -const skipMessage = expect.stringContaining(SKIP_MESSAGE); +const skipMessage = expect.stringContaining('Git hooks already configured, skipping'); -function git(cwd: string, ...args: string[]) { +function git(cwd: string, ...args: string[]): void { const result = spawnSync('git', args, { cwd, stdio: 'pipe' }); if (result.status !== 0) { throw new Error(`git ${args.join(' ')} failed: ${result.stderr.toString()}`); } } -function writePackageJson(dir: string, pkg: Record) { +function writePackageJson(dir: string, pkg: Record): void { fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify(pkg)); } @@ -51,20 +49,22 @@ function readPackageJson(dir: string): { scripts?: Record } { } /** Simulate what `vp config` leaves behind: dispatcher files and core.hooksPath. */ -function installDispatcher(dir: string, hookNames: string[]) { +function installDispatcher(dir: string, hookNames: string[]): void { const dispatcherDir = path.join(dir, '.vite-hooks', '_'); fs.mkdirSync(dispatcherDir, { recursive: true }); - fs.writeFileSync(path.join(dispatcherDir, 'h'), hookScript('.vite-hooks'), { mode: 0o755 }); - fs.chmodSync(path.join(dispatcherDir, 'h'), 0o755); + function writeScript(name: string, content: string): void { + const filePath = path.join(dispatcherDir, name); + fs.writeFileSync(filePath, content, { mode: 0o755 }); + fs.chmodSync(filePath, 0o755); + } + writeScript('h', hookScript('.vite-hooks')); for (const hookName of hookNames) { - const stubPath = path.join(dispatcherDir, hookName); - fs.writeFileSync(stubPath, HOOK_STUB_SCRIPT, { mode: 0o755 }); - fs.chmodSync(stubPath, 0o755); + writeScript(hookName, HOOK_STUB_SCRIPT); } git(dir, 'config', 'core.hooksPath', '.vite-hooks/_'); } -function expectNoHooksPrompt() { +function expectNoHooksPrompt(): void { expect(mockConfirm).not.toHaveBeenCalled(); expect(mockSelect).not.toHaveBeenCalled(); } @@ -82,20 +82,23 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { vi.clearAllMocks(); }); - function writeProjectHooks() { + function writeProjectHooks(): void { fs.mkdirSync(path.join(tmpDir, '.vite-hooks'), { recursive: true }); fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'commit-msg'), 'echo msg\n'); fs.writeFileSync(path.join(tmpDir, '.vite-hooks', 'pre-commit'), 'npm test\n'); } - function setUpFullyConfiguredHooks() { + function setUpFullyConfiguredHooks(): void { git(tmpDir, 'init', '-q'); writePackageJson(tmpDir, { scripts: { prepare: 'vp config' } }); writeProjectHooks(); installDispatcher(tmpDir, ['commit-msg', 'pre-commit']); } - function collect(options: { interactive: boolean; hooks?: boolean }) { + function collect(options: { + interactive: boolean; + hooks?: boolean; + }): Promise { return collectMigrationSetupPlan( tmpDir, PackageManager.pnpm, @@ -162,18 +165,10 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { expectNoHooksPrompt(); }); - it('still honors --no-hooks', async () => { - const plan = await collect({ interactive: true, hooks: false }); - - expect(plan.shouldSetupHooks).toBe(false); - expectNoHooksPrompt(); - expect(mockLog).not.toHaveBeenCalledWith(skipMessage); - }); - - it('still runs setup when --hooks is passed', async () => { - const plan = await collect({ interactive: true, hooks: true }); + it.each([true, false])('honors --hooks=%s when setup is complete', async (hooks) => { + const plan = await collect({ interactive: true, hooks }); - expect(plan.shouldSetupHooks).toBe(true); + expect(plan.shouldSetupHooks).toBe(hooks); expectNoHooksPrompt(); expect(mockLog).not.toHaveBeenCalledWith(skipMessage); }); diff --git a/packages/cli/src/migration/migrator/git-hooks.ts b/packages/cli/src/migration/migrator/git-hooks.ts index 72b5fa5b1e..02560157c3 100644 --- a/packages/cli/src/migration/migrator/git-hooks.ts +++ b/packages/cli/src/migration/migrator/git-hooks.ts @@ -15,7 +15,12 @@ import type { PackageManager, WorkspacePackage } from '../../types/index.ts'; import { documentationUrl } from '../../utils/documentation.ts'; import { findGitRoot } from '../../utils/git.ts'; import { editJsonFile, isJsonFile, readJsonFile } from '../../utils/json.ts'; -import { type CommandToken, literalArgv, literalCommands } from '../../utils/shell-command.ts'; +import { + type CommandToken, + getCommandStartIndex, + literalArgv, + literalCommands, +} from '../../utils/shell-command.ts'; import { hasStagedConfigInViteConfig, mergeStagedConfigToViteConfig, @@ -102,18 +107,8 @@ export function listExistingViteHookNames(projectPath: string): string[] { ); } -/** - * Whether Vite+ hooks are already fully set up for this project, i.e. running - * the hooks step again would have nothing left to do. Hook files in - * `.vite-hooks/` alone are not enough; full setup also requires: - * - * - a `prepare` or `postinstall` script that runs `vp config` (refreshes the - * dispatcher), and - * - at the git repository root, `core.hooksPath` pointing at `.vite-hooks/_` - * with the generated dispatcher (`_/h`) and a stub for every project hook, - * matching what `vp config` writes and executable on POSIX (git ignores - * non-executable hooks). - */ +/** Full setup requires project hooks, a refresh lifecycle, and a current, + * executable dispatcher selected by core.hooksPath at the repository root. */ export function isViteHooksSetupComplete(projectPath: string): boolean { if (!hasExistingViteHooksPolicy(projectPath)) { return false; @@ -137,14 +132,7 @@ export function isViteHooksSetupComplete(projectPath: string): boolean { return false; } const dispatcherDir = path.join(projectPath, '.vite-hooks', '_'); - const expectedFiles: [string, string][] = [ - ['h', hookScript('.vite-hooks')], - ...listExistingViteHookNames(projectPath).map((name): [string, string] => [ - name, - HOOK_STUB_SCRIPT, - ]), - ]; - return expectedFiles.every(([name, expectedContent]) => { + for (const name of ['h', ...listExistingViteHookNames(projectPath)]) { const filePath = path.join(dispatcherDir, name); const stats = fs.lstatSync(filePath, { throwIfNoEntry: false }); if (!stats?.isFile()) { @@ -155,49 +143,45 @@ export function isViteHooksSetupComplete(projectPath: string): boolean { if (process.platform !== 'win32' && (stats.mode & 0o111) === 0) { return false; } - // Stale or modified dispatcher files are refreshed by running setup again. - return fs.readFileSync(filePath, 'utf8') === expectedContent; - }); + const expectedContent = name === 'h' ? hookScript('.vite-hooks') : HOOK_STUB_SCRIPT; + if (fs.readFileSync(filePath, 'utf8') !== expectedContent) { + return false; + } + } + return true; } -/** - * Whether a lifecycle script that `vp config` supports (`prepare` or - * `postinstall`) already runs it to refresh the hook dispatcher. - */ function hasVpConfigLifecycleScript(scripts: Record | undefined): boolean { - return runsVpConfigHooks(scripts?.prepare) || runsVpConfigHooks(scripts?.postinstall); + return [scripts?.prepare, scripts?.postinstall].some((script) => + vpConfigHookCommands(script).some((command) => command.hooksEnabled), + ); } -/** - * Whether a script runs `vp config` with hooks enabled. `--no-hooks` skips the - * dispatcher install, so it does not count; the last of `--hooks` / - * `--no-hooks` wins, as in the `vp config` argument parser. - */ -function runsVpConfigHooks(script: string | undefined): boolean { - return vpConfigHookCommands(script).some( - (args) => - args.findLast(({ value }) => value === '--hooks' || value === '--no-hooks')?.value !== - '--no-hooks', - ); +interface VpConfigHookCommand { + args: CommandToken[]; + hooksEnabled: boolean; } /** Only recognize literal invocations in the project directory. Text passed * to echo, node, or another command must not count as a dispatcher refresh. */ -function vpConfigHookCommands(script: string | undefined): CommandToken[][] { - let commands = script ? literalCommands(script) : undefined; +function vpConfigHookCommands(script: string | undefined): VpConfigHookCommand[] { + if (!script) { + return []; + } + let commands = literalCommands(script); // Preserve a leading refresh even when later commands need shell evaluation. // setupGitHooks prepends this form when it cannot rewrite an existing script. - if (!commands && script?.includes('&&')) { - const first = literalArgv(script.slice(0, script.indexOf('&&'))); + const separator = script.indexOf('&&'); + if (!commands && separator !== -1) { + const first = literalArgv(script.slice(0, separator)); commands = first?.length ? [first] : undefined; } if (!commands) { return []; } - const result: CommandToken[][] = []; + const result: VpConfigHookCommand[] = []; for (const tokens of commands) { const values = tokens.map(({ value }) => value); - let start = 0; if (/^[A-Za-z_]\w*=/.test(values[0] ?? '')) { // Environment overrides can disable hooks or redirect Git operations. return result; @@ -207,15 +191,7 @@ function vpConfigHookCommands(script: string | undefined): CommandToken[][] { ) { return result; } - if (['pnpm', 'npm', 'yarn', 'bun', 'npx', 'bunx'].includes(values[0])) { - start++; - if (values[start] === 'exec' || values[start] === 'x') { - start++; - } - if (values[start] === '--') { - start++; - } - } + const start = getCommandStartIndex(values); if (values[start] !== 'vp' || values[start + 1] !== 'config') { continue; } @@ -230,7 +206,7 @@ function vpConfigHookCommands(script: string | undefined): CommandToken[][] { (parsed.value.hooksDir === undefined || normalizeGitHooksPath(parsed.value.hooksDir) === '.vite-hooks') ) { - result.push(args); + result.push({ args, hooksEnabled: parsed.value.hooks !== false }); } } return result; @@ -242,9 +218,9 @@ function vpConfigHookCommands(script: string | undefined): CommandToken[][] { * `vp config` invocation to rewrite. */ function enableVpConfigHooks(script: string): string | null { - const flags = vpConfigHookCommands(script) - .flat() - .filter(({ value }) => value === '--no-hooks'); + const flags = vpConfigHookCommands(script).flatMap(({ args }) => + args.filter(({ value }) => value === '--no-hooks'), + ); if (!flags.length) { return null; } @@ -450,14 +426,13 @@ export function setupGitHooks( if (!pkg.scripts) { pkg.scripts = {}; } - // Leave scripts alone when `prepare` or `postinstall` already runs it. if (!hasVpConfigLifecycleScript(pkg.scripts)) { - // Re-enable an existing `vp config --no-hooks` in prepare rather than - // adding a second `vp config` invocation. - const enabledPrepare = pkg.scripts.prepare && enableVpConfigHooks(pkg.scripts.prepare); - pkg.scripts.prepare = - enabledPrepare || - (pkg.scripts.prepare ? `vp config && ${pkg.scripts.prepare}` : 'vp config'); + const prepare = pkg.scripts.prepare; + if (prepare) { + pkg.scripts.prepare = enableVpConfigHooks(prepare) ?? `vp config && ${prepare}`; + } else { + pkg.scripts.prepare = 'vp config'; + } } return pkg; diff --git a/packages/cli/src/migration/setup-plan.ts b/packages/cli/src/migration/setup-plan.ts index a205a83773..35cb580ee0 100644 --- a/packages/cli/src/migration/setup-plan.ts +++ b/packages/cli/src/migration/setup-plan.ts @@ -48,28 +48,22 @@ async function collectGitHooksDecision( options: MigrationOptions, packages: readonly WorkspacePackage[] = [], ): Promise { - let shouldSetupHooks: boolean; - if (options.hooks === undefined && hasExistingViteHooksPolicy(rootDir)) { - // Project-owned hooks in .vite-hooks/ already express the intent to use - // Vite+ hooks, so don't ask again. - if (isViteHooksSetupComplete(rootDir)) { - log(`${muted('•')} Git hooks already configured, skipping`); - return false; - } - // Hook files exist but setup is incomplete: finish it through the usual - // checks. setupGitHooks keeps existing hook files unchanged. - shouldSetupHooks = true; - } else { - shouldSetupHooks = await promptGitHooks(options); + const hasExistingHooks = options.hooks === undefined && hasExistingViteHooksPolicy(rootDir); + if (hasExistingHooks && isViteHooksSetupComplete(rootDir)) { + log(`${muted('•')} Git hooks already configured, skipping`); + return false; } - if (shouldSetupHooks) { - const reason = preflightGitHooksSetup(rootDir, packageManager, packages); - if (reason) { - prompts.log.warn(`⚠ ${reason}`); - shouldSetupHooks = false; - } + + // Existing hook files opt into setup; preflight still protects their policy. + if (!hasExistingHooks && !(await promptGitHooks(options))) { + return false; + } + const reason = preflightGitHooksSetup(rootDir, packageManager, packages); + if (reason) { + prompts.log.warn(`⚠ ${reason}`); + return false; } - return shouldSetupHooks; + return true; } async function collectAgentInstructionPlan( diff --git a/packages/cli/src/migration/vitest-v5/commands.ts b/packages/cli/src/migration/vitest-v5/commands.ts index a3d213f620..475cf7e335 100644 --- a/packages/cli/src/migration/vitest-v5/commands.ts +++ b/packages/cli/src/migration/vitest-v5/commands.ts @@ -1,4 +1,4 @@ -import { literalArgv, literalCommands } from '../../utils/shell-command.ts'; +import { getCommandStartIndex, literalArgv, literalCommands } from '../../utils/shell-command.ts'; import type { RewriteResult, VitestV5Finding } from './ast.ts'; /** Read test commands without evaluating shell state or directory changes. */ @@ -26,16 +26,7 @@ export function literalTestCommands(command: string): string[][] | undefined { /** Locate the runner's arguments after a supported package-manager wrapper. */ export function vitestCommandArgsStart(values: readonly string[]): number { - let start = 0; - if (['pnpm', 'npm', 'yarn', 'bun', 'npx', 'bunx'].includes(values[0])) { - start = 1; - if (values[start] === 'exec' || values[start] === 'x') { - start++; - } - if (values[start] === '--') { - start++; - } - } + const start = getCommandStartIndex(values); if (values[start] === 'vitest') { return start + 1; } diff --git a/packages/cli/src/utils/shell-command.ts b/packages/cli/src/utils/shell-command.ts index 55529f631f..13697a409c 100644 --- a/packages/cli/src/utils/shell-command.ts +++ b/packages/cli/src/utils/shell-command.ts @@ -4,6 +4,21 @@ export interface CommandToken { end: number; } +/** Locate the executable after a supported package-manager wrapper. */ +export function getCommandStartIndex(argv: readonly string[]): number { + if (!['pnpm', 'npm', 'yarn', 'bun', 'npx', 'bunx'].includes(argv[0])) { + return 0; + } + let start = 1; + if (argv[start] === 'exec' || argv[start] === 'x') { + start++; + } + if (argv[start] === '--') { + start++; + } + return start; +} + /** Parse only a single literal argv command. Shell expansion, pipelines, and * compound commands need review; treating them as argv could change behavior. */ export function literalArgv(command: string): CommandToken[] | undefined { @@ -64,7 +79,7 @@ export function literalCommands(command: string): CommandToken[][] | undefined { const commands: CommandToken[][] = []; let quote = ''; let start = 0; - const append = (end: number) => { + function appendCommand(end: number): boolean { const tokens = literalArgv(command.slice(start, end)); if (!tokens?.length) { return false; @@ -75,7 +90,7 @@ export function literalCommands(command: string): CommandToken[][] | undefined { } commands.push(tokens); return true; - }; + } for (let index = 0; index < command.length; index++) { const char = command[index]; if (char === '\\') { @@ -89,12 +104,8 @@ export function literalCommands(command: string): CommandToken[][] | undefined { quote = char; } else if (char === '#') { return undefined; - } else if ( - char === ';' || - command.slice(index, index + 2) === '&&' || - command.slice(index, index + 2) === '||' - ) { - if (!append(index)) { + } else if (char === ';' || command.startsWith('&&', index) || command.startsWith('||', index)) { + if (!appendCommand(index)) { return undefined; } if (char !== ';') { @@ -103,5 +114,8 @@ export function literalCommands(command: string): CommandToken[][] | undefined { start = index + 1; } } - return !quote && append(command.length) ? commands : undefined; + if (quote || !appendCommand(command.length)) { + return undefined; + } + return commands; } From 03ad99bc5ad25995345cc418e00a612bf7d9a676 Mon Sep 17 00:00:00 2001 From: MK Date: Tue, 20 Oct 2026 14:07:17 +0800 Subject: [PATCH 8/8] test(migrate): use platform path in hooks warning assertion --- .../cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts index 11c7069354..d4a98ae2c9 100644 --- a/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts +++ b/packages/cli/src/migration/__tests__/setup-plan-git-hooks.spec.ts @@ -380,7 +380,9 @@ describe('collectMigrationSetupPlan existing Vite+ hooks', () => { expect(plan.shouldSetupHooks).toBe(false); expectNoHooksPrompt(); expect(prompts.log.warn).toHaveBeenCalledWith( - expect.stringContaining('Git hook dispatcher path ".vite-hooks/_" is unsafe'), + expect.stringContaining( + `Git hook dispatcher path "${path.join('.vite-hooks', '_')}" is unsafe`, + ), ); expect(fs.readFileSync(dispatcherDir, 'utf8')).toBe('preserve this file'); });