From 342fe1f84931ff511aeea52aef89aaa3d179dba8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 7 Oct 2026 20:48:40 +0000 Subject: [PATCH 1/3] Initial plan From a0afac12916e3d5e747b2a248f99473f40382ea9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 7 Oct 2026 21:01:39 +0000 Subject: [PATCH 2/3] Preserve unmanaged Git hooks and diagnose Git LFS ownership conflicts Co-authored-by: iclanton <5010588+iclanton@users.noreply.github.com> --- ...anaged-git-lfs-hooks_2026-10-07-21-00.json | 10 + .../common/git-hooks/commit-msg.sample | 33 +- libraries/rush-lib/src/logic/GitHooks.ts | 197 ++++++++++ .../src/logic/base/BaseInstallManager.ts | 123 +------ .../rush-lib/src/logic/test/GitHooks.test.ts | 335 ++++++++++++++++++ 5 files changed, 575 insertions(+), 123 deletions(-) create mode 100644 common/changes/@microsoft/rush/fix-managed-git-lfs-hooks_2026-10-07-21-00.json create mode 100644 libraries/rush-lib/src/logic/GitHooks.ts create mode 100644 libraries/rush-lib/src/logic/test/GitHooks.test.ts diff --git a/common/changes/@microsoft/rush/fix-managed-git-lfs-hooks_2026-10-07-21-00.json b/common/changes/@microsoft/rush/fix-managed-git-lfs-hooks_2026-10-07-21-00.json new file mode 100644 index 00000000000..5adcd95edea --- /dev/null +++ b/common/changes/@microsoft/rush/fix-managed-git-lfs-hooks_2026-10-07-21-00.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@microsoft/rush", + "comment": "Preserve unmanaged Git hooks during install/update, detect hook ownership conflicts, and diagnose Git LFS wrapper incompatibility with reused checkout workspaces.", + "type": "patch" + } + ], + "packageName": "@microsoft/rush" +} diff --git a/libraries/rush-lib/assets/rush-init/common/git-hooks/commit-msg.sample b/libraries/rush-lib/assets/rush-init/common/git-hooks/commit-msg.sample index 59cacb80ca1..e6eb66071aa 100644 --- a/libraries/rush-lib/assets/rush-init/common/git-hooks/commit-msg.sample +++ b/libraries/rush-lib/assets/rush-init/common/git-hooks/commit-msg.sample @@ -1,8 +1,37 @@ #!/bin/sh # # This is an example Git hook for use with Rush. To enable this hook, rename this file -# to "commit-msg" and then run "rush install", which will copy it from common/git-hooks -# to the .git/hooks folder. +# to "commit-msg" and then run "rush install", which will install a delegate in the +# Git hooks folder that invokes common/git-hooks/commit-msg. +# +# HOOK OWNERSHIP AND GIT LFS +# +# Rush preserves hooks installed by other tools. If a common/git-hooks file has the +# same name as an unmanaged installed hook, Rush reports an error without changing +# any hooks. Review and migrate custom logic before removing a conflicting hook. +# --bypass-policy skips hook installation on a conflict; it does not overwrite hooks. +# Rush refreshes its unmodified delegates (including those from older Rush versions) +# and removes obsolete non-LFS delegates. Locally edited delegates are preserved. +# +# Git LFS owns pre-push, post-checkout, post-commit, and post-merge. If you do not +# need custom implementations of these hooks, leave these names OUT of common/git-hooks +# and run "git lfs install --local". Rush will preserve the canonical LFS hooks; +# placeholders are not needed to enable LFS uploads. +# +# If you opt into a Rush-managed hook with one of these names, the delegate invokes +# the repository hook and then Git LFS (if installed). Rush warns that these wrappers +# are NOT accepted by "git lfs install --local". In particular, Azure Pipelines LFS +# checkout can fail on a reused agent before any repository script runs. Custom Rush +# hooks with these names require a checkout flow that does not reinitialize LFS hooks. +# +# To recover an existing workspace, first remove unnecessary LFS hook placeholders +# from common/git-hooks. Then, before the next checkout, remove only the corresponding +# Rush-generated delegates from the Git hooks folder and run "git lfs install --local". +# This may require agent maintenance or a fresh workspace. Rush deliberately preserves +# obsolete LFS delegates with a warning until you do this, to avoid disabling uploads. +# Do not use "git lfs install --force" if it would overwrite legitimate custom hooks. +# Older Rush versions emptied the entire hooks folder, so upgrading alone cannot +# restore LFS hooks already deleted by an earlier install: initialize LFS again. # # TO LEARN MORE ABOUT GIT HOOKS # diff --git a/libraries/rush-lib/src/logic/GitHooks.ts b/libraries/rush-lib/src/logic/GitHooks.ts new file mode 100644 index 00000000000..e32513b09f3 --- /dev/null +++ b/libraries/rush-lib/src/logic/GitHooks.ts @@ -0,0 +1,197 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. +// See LICENSE in the project root for license information. + +import * as path from 'node:path'; +import { + AlreadyReportedError, + FileSystem, + NewlineKind, + Path, + PosixModeBits +} from '@rushstack/node-core-library'; +import { Colorize, type ITerminal } from '@rushstack/terminal'; + +import type { RushConfiguration } from '../api/RushConfiguration'; +import { Git } from './Git'; +import { RushConstants } from './RushConstants'; + +const gitLfsHooks: ReadonlySet = new Set(['post-checkout', 'post-commit', 'post-merge', 'pre-push']); +const generatedHookMarker: string = '# Generated by Rush. Do not edit.\n'; + +/** + * Installs delegates without taking ownership of hooks installed by other tools. + */ +export class GitHooks { + public static async installAsync( + rushConfiguration: RushConfiguration, + terminal: ITerminal, + bypassPolicy: boolean + ): Promise { + const hookSource: string = path.join(rushConfiguration.commonFolder, 'git-hooks'); + const git: Git = new Git(rushConfiguration); + const hookDestination: string | undefined = git.getHooksFolder(); + if (!hookDestination) { + return; + } + + // Only install files that look like Git hook names, ignoring ".sample" files. + const hookFilenames: string[] = FileSystem.exists(hookSource) + ? FileSystem.readFolderItemNames(hookSource).filter((name) => /^[a-z-]+$/.test(name)) + : []; + + if (!(await git.getIsHooksPathDefaultAsync())) { + // Do not clean up the default hooks folder when another hook manager is active. + if (hookFilenames.length > 0) { + const hooksPath: string = await git.getConfigHooksPathAsync(); + GitHooks._reportConflict( + terminal, + bypassPolicy, + 'Rush cannot install the "common/git-hooks" scripts because your Git configuration ' + + `specifies "core.hooksPath=${hooksPath}". You can remove the setting by running:\n` + + ' git config --unset core.hooksPath' + ); + } + + return; + } + + const hookRelativePath: string = Path.convertToSlashes(path.relative(hookDestination, hookSource)); + const installedHookNames: Set = new Set(); + const managedHookNames: Set = new Set(); + if (FileSystem.exists(hookDestination)) { + for (const item of FileSystem.readFolderItems(hookDestination)) { + installedHookNames.add(item.name); + // Never follow symlinks, even when they point to a Rush-generated script. + if (item.isFile() && /^[a-z-]+$/.test(item.name)) { + const content: string = FileSystem.readFile(path.join(hookDestination, item.name)); + const legacyContent: string = GitHooks._getHookScript(hookRelativePath, item.name); + if (content === legacyContent || content === GitHooks._addMarker(legacyContent)) { + managedHookNames.add(item.name); + } + } + } + } + + // Preflight all collisions before modifying either directory. + const conflicts: string[] = hookFilenames.filter( + (name) => installedHookNames.has(name) && !managedHookNames.has(name) + ); + if (conflicts.length > 0) { + GitHooks._reportConflict( + terminal, + bypassPolicy, + `Rush will not overwrite hooks that it does not own in "${hookDestination}": ${conflicts.join(', ')}.\n` + + 'Move the custom logic into common/git-hooks and remove the conflicting installed hooks only after ' + + 'reviewing them. For Git LFS hooks, instead remove the same-name files from common/git-hooks ' + + 'and let "git lfs install --local" manage them.' + ); + return; + } + + const lfsHookNames: Set = new Set(hookFilenames.filter((name) => gitLfsHooks.has(name))); + for (const name of managedHookNames) { + if (!hookFilenames.includes(name)) { + if (gitLfsHooks.has(name)) { + // A stale LFS delegate may be the only hook uploading LFS objects. Preserve it until the + // user can replace it with a canonical LFS hook, rather than silently disabling uploads. + lfsHookNames.add(name); + } else { + FileSystem.deleteFile(path.join(hookDestination, name)); + } + } + } + + if (lfsHookNames.size > 0) { + terminal.writeWarningLine( + `Rush-managed Git LFS hooks (${Array.from(lfsHookNames).join(', ')}) are incompatible with ` + + '"git lfs install --local", including Azure Pipelines LFS checkout on reused workspaces. ' + + 'To use that checkout flow, remove these hook names from common/git-hooks, then remove only ' + + 'their Rush-generated delegates from the Git hooks directory and run "git lfs install --local" ' + + 'before the next checkout. Do not use --force if it would overwrite custom hooks.' + ); + } + + if (hookFilenames.length === 0) { + return; + } + + terminal.writeLine('\n' + Colorize.bold('Found files in the "common/git-hooks" folder.')); + FileSystem.ensureFolder(hookDestination); + for (const filename of hookFilenames) { + const hookFilePath: string = path.join(hookSource, filename); + const originalHookFileContent: string = FileSystem.readFile(hookFilePath); + FileSystem.writeFile(hookFilePath, originalHookFileContent, { + convertLineEndings: NewlineKind.Lf + }); + const originalPosixModeBits: PosixModeBits = FileSystem.getPosixModeBits(hookFilePath); + FileSystem.changePosixModeBits( + hookFilePath, + // eslint-disable-next-line no-bitwise + originalPosixModeBits | PosixModeBits.UserRead | PosixModeBits.UserExecute + ); + + const destinationPath: string = path.join(hookDestination, filename); + if (managedHookNames.has(filename)) { + // Installed delegates are read-only. Remove only the verified Rush-owned file before replacing it. + FileSystem.deleteFile(destinationPath); + } + + FileSystem.writeFile( + destinationPath, + GitHooks._addMarker(GitHooks._getHookScript(hookRelativePath, filename)), + { convertLineEndings: NewlineKind.Lf } + ); + FileSystem.changePosixModeBits( + destinationPath, + // eslint-disable-next-line no-bitwise + PosixModeBits.UserRead | PosixModeBits.UserExecute + ); + } + + terminal.writeLine('Successfully installed these Git hook scripts: ' + hookFilenames.join(', ') + '\n'); + } + + private static _reportConflict(terminal: ITerminal, bypassPolicy: boolean, message: string): void { + if (bypassPolicy) { + terminal.writeWarningLine( + message + '\nSkipping Git hook installation because --bypass-policy was specified.' + ); + } else { + terminal.writeErrorLine( + message + + `\nTo temporarily skip Git hook installation, invoke Rush with "${RushConstants.bypassPolicyFlagLongName}".` + ); + throw new AlreadyReportedError(); + } + } + + private static _addMarker(script: string): string { + return script.replace('#!/usr/bin/env bash\n', '#!/usr/bin/env bash\n' + generatedHookMarker); + } + + private static _getHookScript(hookRelativePath: string, filename: string): string { + // Keep the unmarked template identical to older Rush versions so that only unmodified legacy + // delegates are recognized as Rush-owned. A similar-looking or edited script is not ours to delete. + const gitLfsHookHandling: string = gitLfsHooks.has(filename) + ? ` +# Inspired by https://github.com/git-lfs/git-lfs/issues/2865#issuecomment-365742940 +if command -v git-lfs &> /dev/null; then + git lfs ${filename} "$@" +fi +` + : ''; + + return `#!/usr/bin/env bash +set -e +SCRIPT_DIR="$( cd "$( dirname "\${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" +SCRIPT_IMPLEMENTATION_PATH="$SCRIPT_DIR/${hookRelativePath}/${filename}" + +if [[ -f "$SCRIPT_IMPLEMENTATION_PATH" ]]; then + "$SCRIPT_IMPLEMENTATION_PATH" $@ +else + echo "The ${filename} Git hook no longer exists in your version of the repo. Run 'rush install' or 'rush update' to refresh your installed Git hooks." >&2 +fi +${gitLfsHookHandling} +`; + } +} diff --git a/libraries/rush-lib/src/logic/base/BaseInstallManager.ts b/libraries/rush-lib/src/logic/base/BaseInstallManager.ts index a515a84a258..9186ab46c22 100644 --- a/libraries/rush-lib/src/logic/base/BaseInstallManager.ts +++ b/libraries/rush-lib/src/logic/base/BaseInstallManager.ts @@ -18,11 +18,8 @@ import { import { FileSystem, JsonFile, - PosixModeBits, - NewlineKind, AlreadyReportedError, type FileSystemStats, - Path, type FolderItem, Async } from '@rushstack/node-core-library'; @@ -32,7 +29,7 @@ import { ApprovedPackagesChecker } from '../ApprovedPackagesChecker'; import type { AsyncRecycler } from '../../utilities/AsyncRecycler'; import type { BaseShrinkwrapFile } from './BaseShrinkwrapFile'; import { EnvironmentConfiguration } from '../../api/EnvironmentConfiguration'; -import { Git } from '../Git'; +import { GitHooks } from '../GitHooks'; import { type LastInstallFlag, getCommonTempFlag, @@ -69,8 +66,6 @@ export const pnpmIgnoreCompatibilityDbParameter: string = '--config.ignoreCompat const pnpmCacheDirParameter: string = '--config.cacheDir'; const pnpmStateDirParameter: string = '--config.stateDir'; -const gitLfsHooks: ReadonlySet = new Set(['post-checkout', 'post-commit', 'post-merge', 'pre-push']); - /** * This class implements common logic between "rush install" and "rush update". */ @@ -448,7 +443,7 @@ export abstract class BaseInstallManager { // than whatever pnpm would emit. detectAndReportWorkspaceCycles(this.rushConfiguration, terminal); - await this.#installGitHooksAsync(); + await GitHooks.installAsync(this.rushConfiguration, terminal, this.options.bypassPolicy); const approvedPackagesChecker: ApprovedPackagesChecker = new ApprovedPackagesChecker( this.rushConfiguration @@ -721,120 +716,6 @@ export abstract class BaseInstallManager { return { shrinkwrapIsUpToDate, npmrcHash, projectImpactGraphIsUpToDate, variantIsUpToDate }; } - /** - * Git hooks are only installed if the repo opts in by including files in /common/git-hooks - */ - async #installGitHooksAsync(): Promise { - const hookSource: string = path.join(this.rushConfiguration.commonFolder, 'git-hooks'); - const git: Git = new Git(this.rushConfiguration); - const hookDestination: string | undefined = git.getHooksFolder(); - - if (FileSystem.exists(hookSource) && hookDestination) { - const allHookFilenames: string[] = FileSystem.readFolderItemNames(hookSource); - // Ignore the ".sample" file(s) in this folder. - const hookFilenames: string[] = allHookFilenames.filter((x) => !/\.sample$/.test(x)); - if (hookFilenames.length > 0) { - // eslint-disable-next-line no-console - console.log('\n' + Colorize.bold('Found files in the "common/git-hooks" folder.')); - - if (!(await git.getIsHooksPathDefaultAsync())) { - const hooksPath: string = await git.getConfigHooksPathAsync(); - const color: (str: string) => string = this.options.bypassPolicy ? Colorize.yellow : Colorize.red; - // eslint-disable-next-line no-console - console.error( - color( - [ - ' ', - `Rush cannot install the "common/git-hooks" scripts because your Git configuration `, - `specifies "core.hooksPath=${hooksPath}". You can remove the setting by running:`, - ' ', - ' git config --unset core.hooksPath', - ' ' - ].join('\n') - ) - ); - if (this.options.bypassPolicy) { - // If "--bypass-policy" is specified, skip installation of hooks because Rush doesn't - // own the hooks folder - return; - } - // eslint-disable-next-line no-console - console.error( - color( - [ - '(Or, to temporarily ignore this problem, invoke Rush with the ' + - `"${RushConstants.bypassPolicyFlagLongName}" option.)`, - ' ' - ].join('\n') - ) - ); - throw new AlreadyReportedError(); - } - - // Clear the currently installed git hooks and install fresh copies - FileSystem.ensureEmptyFolder(hookDestination); - - // Find the relative path from Git hooks directory to the directory storing the actual scripts. - const hookRelativePath: string = Path.convertToSlashes(path.relative(hookDestination, hookSource)); - - // Only copy files that look like Git hook names - const filteredHookFilenames: string[] = hookFilenames.filter((x) => /^[a-z\-]+/.test(x)); - for (const filename of filteredHookFilenames) { - const hookFilePath: string = `${hookSource}/${filename}`; - // Make sure the actual script in the hookSource directory has correct Linux compatible line endings - const originalHookFileContent: string = FileSystem.readFile(hookFilePath); - FileSystem.writeFile(hookFilePath, originalHookFileContent, { - convertLineEndings: NewlineKind.Lf - }); - // Make sure the actual script in the hookSource directory has required permission bits - const originalPosixModeBits: PosixModeBits = FileSystem.getPosixModeBits(hookFilePath); - FileSystem.changePosixModeBits( - hookFilePath, - // eslint-disable-next-line no-bitwise - originalPosixModeBits | PosixModeBits.UserRead | PosixModeBits.UserExecute - ); - - const gitLfsHookHandling: string = gitLfsHooks.has(filename) - ? ` -# Inspired by https://github.com/git-lfs/git-lfs/issues/2865#issuecomment-365742940 -if command -v git-lfs &> /dev/null; then - git lfs ${filename} "$@" -fi -` - : ''; - - const hookFileContent: string = `#!/usr/bin/env bash -set -e -SCRIPT_DIR="$( cd "$( dirname "\${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" -SCRIPT_IMPLEMENTATION_PATH="$SCRIPT_DIR/${hookRelativePath}/${filename}" - -if [[ -f "$SCRIPT_IMPLEMENTATION_PATH" ]]; then - "$SCRIPT_IMPLEMENTATION_PATH" $@ -else - echo "The ${filename} Git hook no longer exists in your version of the repo. Run 'rush install' or 'rush update' to refresh your installed Git hooks." >&2 -fi -${gitLfsHookHandling} -`; - // Create the hook file. Important: For Bash scripts, the EOL must not be CRLF. - FileSystem.writeFile(path.join(hookDestination, filename), hookFileContent, { - convertLineEndings: NewlineKind.Lf - }); - - FileSystem.changePosixModeBits( - path.join(hookDestination, filename), - // eslint-disable-next-line no-bitwise - PosixModeBits.UserRead | PosixModeBits.UserExecute - ); - } - - // eslint-disable-next-line no-console - console.log( - 'Successfully installed these Git hook scripts: ' + filteredHookFilenames.join(', ') + '\n' - ); - } - } - } - /** * Used when invoking the NPM tool. Appends the common configuration options * to the command-line. diff --git a/libraries/rush-lib/src/logic/test/GitHooks.test.ts b/libraries/rush-lib/src/logic/test/GitHooks.test.ts new file mode 100644 index 00000000000..ccb8dbbe00a --- /dev/null +++ b/libraries/rush-lib/src/logic/test/GitHooks.test.ts @@ -0,0 +1,335 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. +// See LICENSE in the project root for license information. + +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { AlreadyReportedError, FileSystem } from '@rushstack/node-core-library'; +import { StringBufferTerminalProvider, Terminal } from '@rushstack/terminal'; + +import type { RushConfiguration } from '../../api/RushConfiguration'; +import { Git } from '../Git'; +import { GitHooks } from '../GitHooks'; + +const lfsHookNames: string[] = ['post-checkout', 'post-commit', 'post-merge', 'pre-push']; +const marker: string = '# Generated by Rush. Do not edit.\n'; +const itWithGitLfs: typeof it = spawnSync('git', ['lfs', 'version']).status === 0 ? it : it.skip; + +describe(GitHooks.name, () => { + let repoFolder: string; + let hookSource: string; + let hookDestination: string; + let rushConfiguration: RushConfiguration; + let terminalProvider: StringBufferTerminalProvider; + let terminal: Terminal; + + beforeEach(() => { + repoFolder = fs.mkdtempSync(path.join(os.tmpdir(), 'rush-git-hooks-')); + hookSource = path.join(repoFolder, 'common/git-hooks'); + hookDestination = path.join(repoFolder, '.git/hooks'); + rushConfiguration = { + commonFolder: path.join(repoFolder, 'common'), + rushJsonFolder: repoFolder + } as RushConfiguration; + terminalProvider = new StringBufferTerminalProvider(); + terminal = new Terminal(terminalProvider); + jest.spyOn(Git.prototype, 'getHooksFolder').mockReturnValue(hookDestination); + jest.spyOn(Git.prototype, 'getIsHooksPathDefaultAsync').mockResolvedValue(true); + }); + + afterEach(() => { + jest.restoreAllMocks(); + FileSystem.deleteFolder(repoFolder); + }); + + function writeHook(folder: string, name: string, content: string = '#!/bin/sh\nexit 0\n'): void { + const filePath: string = path.join(folder, name); + FileSystem.deleteFile(filePath); + FileSystem.writeFile(filePath, content, { ensureFolderExists: true }); + } + + function readHook(name: string): string { + return FileSystem.readFile(path.join(hookDestination, name)); + } + + async function installAsync(bypassPolicy: boolean = false): Promise { + await GitHooks.installAsync(rushConfiguration, terminal, bypassPolicy); + } + + function runGit(...args: string[]): string { + const result = spawnSync('git', args, { + cwd: repoFolder, + encoding: 'utf8', + env: { + ...process.env, + GIT_CONFIG_NOSYSTEM: '1', + GIT_CONFIG_GLOBAL: os.devNull + } + }); + if (result.status !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${result.error || result.stderr}`); + } + + return result.stdout; + } + + it('preserves unmanaged hooks, samples, and subdirectories', async () => { + writeHook(hookSource, 'pre-commit', '#!/bin/sh\r\nexit 0\r\n'); + writeHook(hookSource, 'commit-msg.sample'); + for (const name of [...lfsHookNames, 'commit-msg', 'pre-commit.sample']) { + writeHook(hookDestination, name, `unmanaged ${name}`); + } + + writeHook(path.join(hookDestination, 'other-tool'), 'config', 'other tool data'); + await installAsync(); + for (const name of [...lfsHookNames, 'commit-msg', 'pre-commit.sample']) { + expect(readHook(name)).toBe(`unmanaged ${name}`); + } + + expect(FileSystem.readFile(path.join(hookDestination, 'other-tool/config'))).toBe('other tool data'); + expect(readHook('pre-commit')).toContain(marker); + expect(readHook('pre-commit')).toContain('../../common/git-hooks/pre-commit'); + expect(FileSystem.readFile(path.join(hookSource, 'pre-commit'))).not.toContain('\r'); + expect(terminalProvider.getWarningOutput()).toBe(''); + }); + + it('refreshes its own hooks and removes obsolete delegates', async () => { + writeHook(hookSource, 'pre-commit'); + writeHook(hookSource, 'commit-msg'); + await installAsync(); + const originalPreCommit: string = readHook('pre-commit'); + FileSystem.deleteFile(path.join(hookSource, 'commit-msg')); + await installAsync(); + expect(readHook('pre-commit')).toBe(originalPreCommit); + expect(FileSystem.exists(path.join(hookDestination, 'commit-msg'))).toBe(false); + }); + + it.each(['empty', 'samples only', 'missing'])('cleans up when the source folder is %s', async (state) => { + writeHook(hookSource, 'pre-commit'); + await installAsync(); + FileSystem.deleteFolder(hookSource); + if (state === 'empty') { + FileSystem.ensureFolder(hookSource); + } else if (state === 'samples only') { + writeHook(hookSource, 'commit-msg.sample'); + } + + writeHook(hookDestination, 'pre-push', 'unmanaged LFS hook'); + await installAsync(); + expect(FileSystem.exists(path.join(hookDestination, 'pre-commit'))).toBe(false); + expect(readHook('pre-push')).toBe('unmanaged LFS hook'); + }); + + it('recognizes unmodified legacy delegates but preserves edited delegates', async () => { + writeHook(hookSource, 'pre-commit'); + writeHook(hookSource, 'commit-msg'); + await installAsync(); + const legacyHook: string = `#!/usr/bin/env bash +set -e +SCRIPT_DIR="$( cd "$( dirname "\${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" +SCRIPT_IMPLEMENTATION_PATH="$SCRIPT_DIR/../../common/git-hooks/pre-commit" + +if [[ -f "$SCRIPT_IMPLEMENTATION_PATH" ]]; then + "$SCRIPT_IMPLEMENTATION_PATH" $@ +else + echo "The pre-commit Git hook no longer exists in your version of the repo. Run 'rush install' or 'rush update' to refresh your installed Git hooks." >&2 +fi + +`; + writeHook(hookDestination, 'pre-commit', legacyHook); + const editedHook: string = readHook('commit-msg') + '\necho custom logic\n'; + writeHook(hookDestination, 'commit-msg', editedHook); + FileSystem.deleteFile(path.join(hookSource, 'commit-msg')); + await installAsync(); + expect(readHook('pre-commit')).toBe( + legacyHook.replace('#!/usr/bin/env bash\n', '#!/usr/bin/env bash\n' + marker) + ); + expect(readHook('commit-msg')).toBe(editedHook); + writeHook(hookDestination, 'pre-commit', legacyHook); + FileSystem.deleteFolder(hookSource); + await installAsync(); + expect(FileSystem.exists(path.join(hookDestination, 'pre-commit'))).toBe(false); + expect(readHook('commit-msg')).toBe(editedHook); + }); + + it('reports all collisions before changing any hook', async () => { + writeHook(hookSource, 'pre-commit'); + await installAsync(); + FileSystem.deleteFile(path.join(hookSource, 'pre-commit')); + writeHook(hookSource, 'commit-msg', '#!/bin/sh\r\nexit 0\r\n'); + writeHook(hookSource, 'pre-push'); + writeHook(hookDestination, 'pre-push', 'canonical LFS hook'); + await expect(installAsync()).rejects.toThrow(AlreadyReportedError); + expect(readHook('pre-push')).toBe('canonical LFS hook'); + expect(FileSystem.exists(path.join(hookDestination, 'commit-msg'))).toBe(false); + expect(readHook('pre-commit')).toContain(marker); + expect(FileSystem.readFile(path.join(hookSource, 'commit-msg'))).toContain('\r\n'); + expect(terminalProvider.getErrorOutput()).toContain('pre-push'); + expect(terminalProvider.getErrorOutput()).toContain('git lfs install --local'); + }); + + it('skips installation on a collision when bypassing policy', async () => { + writeHook(hookSource, 'pre-commit'); + writeHook(hookSource, 'commit-msg'); + writeHook(hookDestination, 'pre-commit', 'another hook manager'); + await installAsync(true); + expect(readHook('pre-commit')).toBe('another hook manager'); + expect(FileSystem.exists(path.join(hookDestination, 'commit-msg'))).toBe(false); + expect(terminalProvider.getWarningOutput()).toContain('Skipping Git hook installation'); + }); + + it('does not take ownership of a directory with a matching hook name', async () => { + writeHook(hookSource, 'pre-commit'); + FileSystem.ensureFolder(path.join(hookDestination, 'pre-commit')); + await expect(installAsync()).rejects.toThrow(AlreadyReportedError); + expect(FileSystem.getStatistics(path.join(hookDestination, 'pre-commit')).isDirectory()).toBe(true); + }); + + (process.platform === 'win32' ? it.skip : it)('does not follow symlinked hooks', async () => { + writeHook(hookSource, 'pre-commit'); + await installAsync(); + const generatedHook: string = readHook('pre-commit'); + const linkTarget: string = path.join(repoFolder, 'linked-hook'); + FileSystem.move({ sourcePath: path.join(hookDestination, 'pre-commit'), destinationPath: linkTarget }); + fs.symlinkSync(linkTarget, path.join(hookDestination, 'pre-commit')); + await expect(installAsync()).rejects.toThrow(AlreadyReportedError); + expect(FileSystem.readFile(linkTarget)).toBe(generatedHook); + expect(FileSystem.getLinkStatistics(path.join(hookDestination, 'pre-commit')).isSymbolicLink()).toBe( + true + ); + }); + + it.each([true, false])('respects core.hooksPath (bypassPolicy=%s)', async (bypassPolicy) => { + writeHook(hookSource, 'pre-commit'); + jest.spyOn(Git.prototype, 'getIsHooksPathDefaultAsync').mockResolvedValue(false); + jest.spyOn(Git.prototype, 'getConfigHooksPathAsync').mockResolvedValue('custom-hooks'); + if (bypassPolicy) { + await installAsync(true); + expect(terminalProvider.getWarningOutput()).toContain('core.hooksPath=custom-hooks'); + } else { + await expect(installAsync()).rejects.toThrow(AlreadyReportedError); + expect(terminalProvider.getErrorOutput()).toContain('core.hooksPath=custom-hooks'); + } + + expect(FileSystem.exists(hookDestination)).toBe(false); + }); + + it('does not clean up delegates when a custom hooksPath is configured', async () => { + writeHook(hookSource, 'pre-commit'); + await installAsync(); + FileSystem.deleteFolder(hookSource); + jest.spyOn(Git.prototype, 'getIsHooksPathDefaultAsync').mockResolvedValue(false); + await installAsync(); + expect(readHook('pre-commit')).toContain(marker); + }); + + it('does nothing outside a Git worktree', async () => { + writeHook(hookSource, 'pre-commit'); + jest.spyOn(Git.prototype, 'getHooksFolder').mockReturnValue(undefined); + await installAsync(); + expect(FileSystem.exists(hookDestination)).toBe(false); + }); + + it.each(lfsHookNames)( + 'warns about %s wrappers and retains their LFS delegation when obsolete', + async (name) => { + writeHook(hookSource, name); + await installAsync(); + const delegate: string = readHook(name); + expect(delegate).toContain(`git lfs ${name} "$@"`); + expect(terminalProvider.getWarningOutput()).toContain('Azure Pipelines'); + expect(terminalProvider.getWarningOutput()).toContain(name); + FileSystem.deleteFolder(hookSource); + // Older Rush versions generated the same delegate without an ownership marker. + writeHook(hookDestination, name, delegate.replace(marker, '')); + await installAsync(); + expect(readHook(name)).toBe(delegate.replace(marker, '')); + } + ); + + itWithGitLfs.each(['lfs first', 'rush first'])( + 'supports repeated LFS checkout/install cycles (%s)', + async (order) => { + runGit('init'); + writeHook(hookSource, 'pre-commit'); + if (order === 'rush first') { + await installAsync(); + } + + runGit('lfs', 'install', '--local'); + const canonicalHooks: string[] = lfsHookNames.map(readHook); + for (let cycle: number = 0; cycle < 3; cycle++) { + await installAsync(); + expect(lfsHookNames.map(readHook)).toEqual(canonicalHooks); + runGit('lfs', 'install', '--local'); + expect(lfsHookNames.map(readHook)).toEqual(canonicalHooks); + } + + // A real commit exercises the Rush pre-commit hook and the canonical LFS post-commit hook. + runGit( + '-c', + 'user.name=Rush Test', + '-c', + 'user.email=rush-test@example.com', + 'commit', + '--allow-empty', + '-m', + 'test' + ); + } + ); + + itWithGitLfs('rejects a canonical LFS collision without corrupting later checkouts', async () => { + runGit('init'); + runGit('lfs', 'install', '--local'); + const canonicalHooks: string[] = lfsHookNames.map(readHook); + writeHook(hookSource, 'pre-push'); + await expect(installAsync()).rejects.toThrow(AlreadyReportedError); + expect(lfsHookNames.map(readHook)).toEqual(canonicalHooks); + runGit('lfs', 'install', '--local'); + }); + + itWithGitLfs('still uploads LFS objects after installing Rush hooks', async () => { + runGit('init'); + runGit('lfs', 'install', '--local'); + runGit('lfs', 'track', '*.bin'); + writeHook(hookSource, 'pre-commit'); + await installAsync(); + const content: string = 'An LFS object that must be uploaded, not just a pointer.'; + FileSystem.writeFile(path.join(repoFolder, 'asset.bin'), content); + runGit('add', '.gitattributes', 'asset.bin'); + runGit( + '-c', + 'user.name=Rush Test', + '-c', + 'user.email=rush-test@example.com', + 'commit', + '-m', + 'LFS object' + ); + const pointer: string = runGit('show', 'HEAD:asset.bin'); + const oid: string = /oid sha256:([a-f0-9]+)/.exec(pointer)![1]; + const remoteFolder: string = path.join(repoFolder, 'remote.git'); + runGit('init', '--bare', remoteFolder); + runGit('remote', 'add', 'origin', remoteFolder); + runGit('push', 'origin', 'HEAD:refs/heads/main'); + const objectPath: string = path.join(remoteFolder, 'lfs/objects', oid.slice(0, 2), oid.slice(2, 4), oid); + expect(FileSystem.readFile(objectPath)).toBe(content); + }); + + itWithGitLfs('diagnoses incompatible wrappers and supports the documented recovery', async () => { + runGit('init'); + writeHook(hookSource, 'pre-push'); + await installAsync(); + expect(() => runGit('lfs', 'install', '--local')).toThrow('Hook already exists: pre-push'); + expect(terminalProvider.getWarningOutput()).toContain('before the next checkout'); + FileSystem.deleteFile(path.join(hookSource, 'pre-push')); + FileSystem.deleteFile(path.join(hookDestination, 'pre-push')); + runGit('lfs', 'install', '--local'); + const canonicalHooks: string[] = lfsHookNames.map(readHook); + await installAsync(); + runGit('lfs', 'install', '--local'); + expect(lfsHookNames.map(readHook)).toEqual(canonicalHooks); + }); +}); From 40b0abd14d24e2c72002fade4f9269bd1b8ca238 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 7 Oct 2026 21:11:47 +0000 Subject: [PATCH 3/3] Handle case-insensitive hook collisions and satisfy Rush lint rules Co-authored-by: iclanton <5010588+iclanton@users.noreply.github.com> --- libraries/rush-lib/src/logic/GitHooks.ts | 86 +++++++++++-------- .../rush-lib/src/logic/test/GitHooks.test.ts | 30 +++++++ 2 files changed, 79 insertions(+), 37 deletions(-) diff --git a/libraries/rush-lib/src/logic/GitHooks.ts b/libraries/rush-lib/src/logic/GitHooks.ts index e32513b09f3..ffd6856e108 100644 --- a/libraries/rush-lib/src/logic/GitHooks.ts +++ b/libraries/rush-lib/src/logic/GitHooks.ts @@ -2,6 +2,7 @@ // See LICENSE in the project root for license information. import * as path from 'node:path'; + import { AlreadyReportedError, FileSystem, @@ -43,7 +44,7 @@ export class GitHooks { // Do not clean up the default hooks folder when another hook manager is active. if (hookFilenames.length > 0) { const hooksPath: string = await git.getConfigHooksPathAsync(); - GitHooks._reportConflict( + reportConflict( terminal, bypassPolicy, 'Rush cannot install the "common/git-hooks" scripts because your Git configuration ' + @@ -56,16 +57,14 @@ export class GitHooks { } const hookRelativePath: string = Path.convertToSlashes(path.relative(hookDestination, hookSource)); - const installedHookNames: Set = new Set(); const managedHookNames: Set = new Set(); if (FileSystem.exists(hookDestination)) { for (const item of FileSystem.readFolderItems(hookDestination)) { - installedHookNames.add(item.name); // Never follow symlinks, even when they point to a Rush-generated script. if (item.isFile() && /^[a-z-]+$/.test(item.name)) { const content: string = FileSystem.readFile(path.join(hookDestination, item.name)); - const legacyContent: string = GitHooks._getHookScript(hookRelativePath, item.name); - if (content === legacyContent || content === GitHooks._addMarker(legacyContent)) { + const legacyContent: string = getHookScript(hookRelativePath, item.name); + if (content === legacyContent || content === addMarker(legacyContent)) { managedHookNames.add(item.name); } } @@ -73,11 +72,26 @@ export class GitHooks { } // Preflight all collisions before modifying either directory. - const conflicts: string[] = hookFilenames.filter( - (name) => installedHookNames.has(name) && !managedHookNames.has(name) - ); + const conflicts: string[] = hookFilenames.filter((name) => { + if (managedHookNames.has(name)) { + return false; + } + + try { + // Resolve the actual destination, including differently cased names on case-insensitive + // filesystems and dangling symlinks that an existence check would miss. + FileSystem.getLinkStatistics(path.join(hookDestination, name)); + return true; + } catch (error) { + if (FileSystem.isNotExistError(error as Error)) { + return false; + } + + throw error; + } + }); if (conflicts.length > 0) { - GitHooks._reportConflict( + reportConflict( terminal, bypassPolicy, `Rush will not overwrite hooks that it does not own in "${hookDestination}": ${conflicts.join(', ')}.\n` + @@ -136,11 +150,9 @@ export class GitHooks { FileSystem.deleteFile(destinationPath); } - FileSystem.writeFile( - destinationPath, - GitHooks._addMarker(GitHooks._getHookScript(hookRelativePath, filename)), - { convertLineEndings: NewlineKind.Lf } - ); + FileSystem.writeFile(destinationPath, addMarker(getHookScript(hookRelativePath, filename)), { + convertLineEndings: NewlineKind.Lf + }); FileSystem.changePosixModeBits( destinationPath, // eslint-disable-next-line no-bitwise @@ -150,38 +162,39 @@ export class GitHooks { terminal.writeLine('Successfully installed these Git hook scripts: ' + hookFilenames.join(', ') + '\n'); } +} - private static _reportConflict(terminal: ITerminal, bypassPolicy: boolean, message: string): void { - if (bypassPolicy) { - terminal.writeWarningLine( - message + '\nSkipping Git hook installation because --bypass-policy was specified.' - ); - } else { - terminal.writeErrorLine( - message + - `\nTo temporarily skip Git hook installation, invoke Rush with "${RushConstants.bypassPolicyFlagLongName}".` - ); - throw new AlreadyReportedError(); - } +function reportConflict(terminal: ITerminal, bypassPolicy: boolean, message: string): void { + if (bypassPolicy) { + terminal.writeWarningLine( + message + '\nSkipping Git hook installation because --bypass-policy was specified.' + ); + } else { + terminal.writeErrorLine( + message + + `\nTo temporarily skip Git hook installation, invoke Rush with "${RushConstants.bypassPolicyFlagLongName}".` + ); + throw new AlreadyReportedError(); } +} - private static _addMarker(script: string): string { - return script.replace('#!/usr/bin/env bash\n', '#!/usr/bin/env bash\n' + generatedHookMarker); - } +function addMarker(script: string): string { + return script.replace('#!/usr/bin/env bash\n', '#!/usr/bin/env bash\n' + generatedHookMarker); +} - private static _getHookScript(hookRelativePath: string, filename: string): string { - // Keep the unmarked template identical to older Rush versions so that only unmodified legacy - // delegates are recognized as Rush-owned. A similar-looking or edited script is not ours to delete. - const gitLfsHookHandling: string = gitLfsHooks.has(filename) - ? ` +function getHookScript(hookRelativePath: string, filename: string): string { + // Keep the unmarked template identical to older Rush versions so that only unmodified legacy + // delegates are recognized as Rush-owned. A similar-looking or edited script is not ours to delete. + const gitLfsHookHandling: string = gitLfsHooks.has(filename) + ? ` # Inspired by https://github.com/git-lfs/git-lfs/issues/2865#issuecomment-365742940 if command -v git-lfs &> /dev/null; then git lfs ${filename} "$@" fi ` - : ''; + : ''; - return `#!/usr/bin/env bash + return `#!/usr/bin/env bash set -e SCRIPT_DIR="$( cd "$( dirname "\${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" SCRIPT_IMPLEMENTATION_PATH="$SCRIPT_DIR/${hookRelativePath}/${filename}" @@ -193,5 +206,4 @@ else fi ${gitLfsHookHandling} `; - } } diff --git a/libraries/rush-lib/src/logic/test/GitHooks.test.ts b/libraries/rush-lib/src/logic/test/GitHooks.test.ts index ccb8dbbe00a..deb269d5991 100644 --- a/libraries/rush-lib/src/logic/test/GitHooks.test.ts +++ b/libraries/rush-lib/src/logic/test/GitHooks.test.ts @@ -186,6 +186,21 @@ fi expect(FileSystem.getStatistics(path.join(hookDestination, 'pre-commit')).isDirectory()).toBe(true); }); + it('detects collisions using filesystem path resolution rather than directory-entry casing', async () => { + writeHook(hookSource, 'pre-commit'); + writeHook(hookDestination, 'Pre-Commit', 'unmanaged hook'); + const existingPath: string = path.join(hookDestination, 'Pre-Commit'); + const getLinkStatistics: typeof FileSystem.getLinkStatistics = FileSystem.getLinkStatistics; + jest.spyOn(FileSystem, 'getLinkStatistics').mockImplementation((filePath) => { + // Simulate a case-insensitive filesystem on every test platform. + return getLinkStatistics( + filePath === path.join(hookDestination, 'pre-commit') ? existingPath : filePath + ); + }); + await expect(installAsync()).rejects.toThrow(AlreadyReportedError); + expect(readHook('Pre-Commit')).toBe('unmanaged hook'); + }); + (process.platform === 'win32' ? it.skip : it)('does not follow symlinked hooks', async () => { writeHook(hookSource, 'pre-commit'); await installAsync(); @@ -200,6 +215,21 @@ fi ); }); + (process.platform === 'win32' ? it.skip : it)( + 'preserves dangling symlinks without creating their targets', + async () => { + writeHook(hookSource, 'pre-commit'); + FileSystem.ensureFolder(hookDestination); + const linkTarget: string = path.join(repoFolder, 'missing-hook'); + fs.symlinkSync(linkTarget, path.join(hookDestination, 'pre-commit')); + await expect(installAsync()).rejects.toThrow(AlreadyReportedError); + expect(FileSystem.exists(linkTarget)).toBe(false); + expect(FileSystem.getLinkStatistics(path.join(hookDestination, 'pre-commit')).isSymbolicLink()).toBe( + true + ); + } + ); + it.each([true, false])('respects core.hooksPath (bypassPolicy=%s)', async (bypassPolicy) => { writeHook(hookSource, 'pre-commit'); jest.spyOn(Git.prototype, 'getIsHooksPathDefaultAsync').mockResolvedValue(false);