[git-hooks] Handle drifted hook replacement safely (#190)

coisa · coisa · commit 866e78b70ebd · 2026-04-23T22:05:31.000-03:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Keep packaged `.agents` payloads exportable and synchronize packaged skills and agents with repository-relative symlink targets so consumer repositories no longer receive broken absolute machine paths (#188)
+- Rewrite drifted Git hooks by removing the previous target first, restore the intended `0o755` executable mode, and report unwritable hook replacements cleanly when `.git/hooks` stays locked (#190)
 
 ## [1.20.0] - 2026-04-23
 
diff --git a/docs/commands/git-hooks.rst b/docs/commands/git-hooks.rst
@@ -11,6 +11,8 @@ The ``git-hooks`` command installs the hook templates maintained in
 
 1. Copies hook files from a source directory to the target hooks directory
 2. Sets executable permissions on copied hooks
+3. Replaces drifted hooks defensively by removing the previous target before
+   recopying it
 
 Usage
 -----
@@ -44,6 +46,11 @@ Options
 ``--interactive``
    Prompt before replacing a drifted Git hook.
 
+When a hook still cannot be rewritten because the target remains locked or
+unwritable, the command logs a clear error for that hook, continues processing
+the remaining hooks, and exits non-zero so ``dev-tools:sync`` reports the hook
+install problem clearly instead of aborting mid-copy.
+
 ``--json``
    Emit a structured machine-readable payload instead of the normal terminal
    output.
@@ -77,4 +84,5 @@ Exit Codes
    * - 0
      - Success. Hooks installed successfully.
    * - 1
-     - Failure. Copy error.
+     - Failure. Drift detected in ``--check`` mode or one or more hooks could
+       not be rewritten automatically.
diff --git a/docs/running/specialized-commands.rst b/docs/running/specialized-commands.rst
@@ -461,7 +461,9 @@ Installs packaged Fast Forward Git hooks.
 Important details:
 
 - copies hook files from source to target directory;
-- sets executable permissions on copied hooks;
+- sets executable permissions on copied hooks with ``0o755``;
+- removes an existing drifted hook before recopying it so stale target
+  permissions do not block replacements;
 - ``--source`` defaults to ``resources/git-hooks``;
 - ``--target`` defaults to ``.git/hooks``;
 - ``--no-overwrite`` preserves existing hook files.
diff --git a/src/Console/Command/GitHooksCommand.php b/src/Console/Command/GitHooksCommand.php
@@ -31,6 +31,7 @@
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Filesystem\Exception\IOExceptionInterface;
 use Symfony\Component\Filesystem\Path;
 
 /**
@@ -129,7 +130,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int
             ->files()
             ->in($sourcePath);
 
-        $status = self::SUCCESS;
+        $checkFailure = false;
+        $installFailure = false;
 
         foreach ($files as $file) {
             $hookPath = Path::join($targetPath, $file->getRelativePathname());
@@ -180,7 +182,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
                 }
 
                 if ($check) {
-                    $status = self::FAILURE;
+                    $checkFailure = true;
 
                     continue;
                 }
@@ -203,8 +205,11 @@ protected function execute(InputInterface $input, OutputInterface $output): int
                 }
             }
 
-            $this->filesystem->copy($file->getRealPath(), $hookPath, $overwrite || $interactive);
-            $this->filesystem->chmod($hookPath, 755, 0o755);
+            if (! $this->installHook($file->getRealPath(), $hookPath, $overwrite || $interactive, $input)) {
+                $installFailure = true;
+
+                continue;
+            }
 
             $this->success(
                 'Installed {hook_name} hook.',
@@ -216,7 +221,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
             );
         }
 
-        if (self::FAILURE === $status) {
+        if ($checkFailure) {
             return $this->failure(
                 'One or more Git hooks require synchronization updates.',
                 $input,
@@ -227,6 +232,17 @@ protected function execute(InputInterface $input, OutputInterface $output): int
             );
         }
 
+        if ($installFailure) {
+            return $this->failure(
+                'One or more Git hooks could not be installed automatically.',
+                $input,
+                [
+                    'target' => $targetPath,
+                ],
+                $targetPath,
+            );
+        }
+
         return $this->success(
             'Git hook synchronization completed successfully.',
             $input,
@@ -248,4 +264,42 @@ private function shouldReplaceHook(string $hookPath): bool
         return $this->getIO()
             ->askConfirmation(\sprintf('Replace drifted Git hook %s? [y/N] ', $hookPath), false);
     }
+
+    /**
+     * Installs a single hook and rewrites drifted targets defensively.
+     *
+     * @param string $sourcePath the packaged hook path
+     * @param string $hookPath the target repository hook path
+     * @param bool $replaceExisting whether an existing hook SHOULD be removed first
+     * @param InputInterface $input the originating command input
+     *
+     * @return bool true when the hook was installed successfully
+     */
+    private function installHook(string $sourcePath, string $hookPath, bool $replaceExisting, InputInterface $input): bool
+    {
+        try {
+            if ($replaceExisting && $this->filesystem->exists($hookPath)) {
+                $this->filesystem->remove($hookPath);
+            }
+
+            $this->filesystem->copy($sourcePath, $hookPath, false);
+            $this->filesystem->chmod(files: $hookPath, mode: 0o755);
+
+            return true;
+        } catch (IOExceptionInterface $exception) {
+            $this->logger->error(
+                'Failed to install {hook_name} hook automatically. Remove or unlock {hook_path} and rerun git-hooks.',
+                [
+                    'input' => $input,
+                    'hook_name' => $this->filesystem->basename($hookPath),
+                    'hook_path' => $hookPath,
+                    'error' => $exception->getMessage(),
+                    'file' => $exception->getPath() ?? $hookPath,
+                    'line' => null,
+                ],
+            );
+
+            return false;
+        }
+    }
 }
diff --git a/tests/Console/Command/GitHooksCommandTest.php b/tests/Console/Command/GitHooksCommandTest.php
@@ -39,6 +39,7 @@
 use Symfony\Component\Config\FileLocatorInterface;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Filesystem\Exception\IOException;
 use Symfony\Component\Finder\Finder;
 
 use function Safe\mkdir;
@@ -48,6 +49,7 @@
 
 #[CoversClass(GitHooksCommand::class)]
 #[UsesClass(FileDiff::class)]
+#[UsesClass(IOException::class)]
 #[UsesTrait(LogsCommandResults::class)]
 final class GitHooksCommandTest extends TestCase
 {
@@ -164,9 +166,9 @@ public function executeWillCopyPackagedHooks(): void
             ->willReturn('/app/.git/hooks');
         $this->filesystem->exists('/app/.git/hooks/post-merge')
             ->willReturn(false);
-        $this->filesystem->copy(Argument::containingString('/post-merge'), '/app/.git/hooks/post-merge', true)
+        $this->filesystem->copy(Argument::containingString('/post-merge'), '/app/.git/hooks/post-merge', false)
             ->shouldBeCalledOnce();
-        $this->filesystem->chmod('/app/.git/hooks/post-merge', 755, 0o755)
+        $this->filesystem->chmod('/app/.git/hooks/post-merge', 0o755)
             ->shouldBeCalledOnce();
         $this->logger->log('info', 'Installed {hook_name} hook.', Argument::type('array'))
             ->shouldBeCalledOnce();
@@ -327,6 +329,97 @@ public function executeWillSkipReplacingHookWhenInteractiveConfirmationIsDecline
         self::assertSame(GitHooksCommand::SUCCESS, $this->executeCommand());
     }
 
+    /**
+     * @return void
+     */
+    #[Test]
+    public function executeWillRemoveDriftedHookBeforeReplacingIt(): void
+    {
+        $this->input->getOption('source')
+            ->willReturn('resources/git-hooks');
+        $this->input->getOption('target')
+            ->willReturn('.git/hooks');
+        $this->input->getOption('no-overwrite')
+            ->willReturn(false);
+
+        $this->fileLocator->locate('resources/git-hooks')
+            ->willReturn($this->sourceDirectory);
+        $this->finderFactory->create()
+            ->willReturn(new Finder())
+            ->shouldBeCalledOnce();
+        $this->filesystem->getAbsolutePath('.git/hooks')
+            ->willReturn('/app/.git/hooks');
+        $this->filesystem->exists('/app/.git/hooks/post-merge')
+            ->willReturn(true);
+        $this->fileDiffer->diff(Argument::containingString('/post-merge'), '/app/.git/hooks/post-merge')
+            ->willReturn(new FileDiff(FileDiff::STATUS_CHANGED, 'Changed summary', null))
+            ->shouldBeCalledOnce();
+        $this->filesystem->remove('/app/.git/hooks/post-merge')
+            ->shouldBeCalledOnce();
+        $this->filesystem->copy(Argument::containingString('/post-merge'), '/app/.git/hooks/post-merge', false)
+            ->shouldBeCalledOnce();
+        $this->filesystem->chmod('/app/.git/hooks/post-merge', 0o755)
+            ->shouldBeCalledOnce();
+        $this->logger->log('info', 'Installed {hook_name} hook.', Argument::type('array'))
+            ->shouldBeCalledOnce();
+        $this->logger->log('info', 'Git hook synchronization completed successfully.', Argument::type('array'))
+            ->shouldBeCalledOnce();
+
+        self::assertSame(GitHooksCommand::SUCCESS, $this->executeCommand());
+    }
+
+    /**
+     * @return void
+     */
+    #[Test]
+    public function executeWillReportInstallFailureWhenReplacementStillCannotBeWritten(): void
+    {
+        $this->input->getOption('source')
+            ->willReturn('resources/git-hooks');
+        $this->input->getOption('target')
+            ->willReturn('.git/hooks');
+        $this->input->getOption('no-overwrite')
+            ->willReturn(false);
+
+        $this->fileLocator->locate('resources/git-hooks')
+            ->willReturn($this->sourceDirectory);
+        $this->finderFactory->create()
+            ->willReturn(new Finder())
+            ->shouldBeCalledOnce();
+        $this->filesystem->getAbsolutePath('.git/hooks')
+            ->willReturn('/app/.git/hooks');
+        $this->filesystem->exists('/app/.git/hooks/post-merge')
+            ->willReturn(true);
+        $this->fileDiffer->diff(Argument::containingString('/post-merge'), '/app/.git/hooks/post-merge')
+            ->willReturn(new FileDiff(FileDiff::STATUS_CHANGED, 'Changed summary', null))
+            ->shouldBeCalledOnce();
+        $this->filesystem->remove('/app/.git/hooks/post-merge')
+            ->shouldBeCalledOnce();
+        $this->filesystem->copy(Argument::containingString('/post-merge'), '/app/.git/hooks/post-merge', false)
+            ->willThrow(new IOException('Target file could not be opened for writing.', 0, null, '/app/.git/hooks/post-merge'))
+            ->shouldBeCalledOnce();
+        $this->filesystem->basename('/app/.git/hooks/post-merge')
+            ->willReturn('post-merge')
+            ->shouldBeCalledOnce();
+        $this->logger->error(
+            'Failed to install {hook_name} hook automatically. Remove or unlock {hook_path} and rerun git-hooks.',
+            Argument::that(
+                static fn(array $context): bool => $context['input'] instanceof InputInterface
+                    && 'post-merge' === $context['hook_name']
+                    && '/app/.git/hooks/post-merge' === $context['hook_path']
+                    && '/app/.git/hooks/post-merge' === $context['file']
+                    && null === $context['line']
+                    && str_contains($context['error'], 'Target file could not be opened for writing.')
+            ),
+        )->shouldBeCalledOnce();
+        $this->logger->error('One or more Git hooks could not be installed automatically.', Argument::type('array'))
+            ->shouldBeCalledOnce();
+        $this->filesystem->chmod(Argument::cetera())
+            ->shouldNotBeCalled();
+
+        self::assertSame(GitHooksCommand::FAILURE, $this->executeCommand());
+    }
+
     /**
      * @return int
      */
