CLI-9 PR review

sophio-japharidze-sonarsource · sophio-japharidze-sonarsource · commit d44fbcc8f850 · 2026-03-19T09:26:26.000+01:00
diff --git a/CLAUDE.md b/CLAUDE.md
@@ -23,7 +23,7 @@ bun run test:all      # Unit + integration + script tests
 
 Each command lives in `src/cli/commands/`. The command tree is defined in `src/cli/command-tree.ts` and the entry point is `src/index.ts`.
 
-To add a new command: add it to `src/cli/command-tree.ts` and implement the logic in a new file under `src/cli/commands/`.
+To add a new command: add it to `src/cli/command-tree.ts` and implement the logic in a new file under `src/cli/commands/`. The integrate git command uses a `GitRepo` type (`src/cli/commands/_common/git-repo.ts`) for hook strategy detection (pre-commit framework, Husky, or native) and lazy hooks-dir resolution.
 
 ## Error handling
 
diff --git a/src/cli/commands/_common/discovery.ts b/src/cli/commands/_common/discovery.ts
@@ -162,7 +162,7 @@ export async function discoverProject(startDir: string): Promise<DiscoveredProje
   return config;
 }
 
-function findGitRoot(startDir: string): { gitRoot: string; isGit: boolean } {
+export function findGitRoot(startDir: string): { gitRoot: string; isGit: boolean } {
   let dir = startDir;
 
   for (;;) {
diff --git a/src/cli/commands/_common/git-repo.ts b/src/cli/commands/_common/git-repo.ts
@@ -0,0 +1,111 @@
+/*
+ * SonarQube CLI
+ * Copyright (C) 2026 SonarSource Sàrl
+ * mailto:info AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+// Git repository abstraction for hook installation: root dir, hooks path, and framework detection.
+
+import { existsSync, statSync } from 'node:fs';
+import { isAbsolute, join } from 'node:path';
+import { CommandFailedError } from './error';
+import { spawnProcess } from '../../../lib/process';
+
+const PRE_COMMIT_CONFIG_FILE = '.pre-commit-config.yaml';
+const toForwardSlash = (p: string) => p.replaceAll('\\', '/');
+
+/**
+ * Resolves the directory git uses for hooks (core.hooksPath or .git/hooks).
+ */
+export async function resolveGitHooksDir(root: string): Promise<string> {
+  let configResult;
+  try {
+    configResult = await spawnProcess('git', ['config', 'core.hooksPath'], { cwd: root });
+  } catch {
+    configResult = null;
+  }
+  if (configResult?.exitCode === 0) {
+    const configured = configResult.stdout.trim();
+    if (configured) {
+      return isAbsolute(configured) ? configured : join(root, configured);
+    }
+  }
+
+  const dotGit = join(root, '.git');
+  try {
+    if (statSync(dotGit).isDirectory()) {
+      return join(dotGit, 'hooks');
+    }
+  } catch {
+    // .git is a file (worktree) or missing — use git rev-parse
+  }
+
+  let result;
+  try {
+    result = await spawnProcess('git', ['rev-parse', '--git-path', 'hooks'], { cwd: root });
+  } catch (error) {
+    const message = error instanceof Error ? error.message : String(error);
+    throw new CommandFailedError(`Failed to run git [${message}]`);
+  }
+  if (result.exitCode !== 0) {
+    const detail = [result.stderr, result.stdout].filter((s) => s.length > 0).join('\n');
+    throw new CommandFailedError(
+      `Could not resolve git hooks directory (exit code ${result.exitCode}) ${detail}`,
+    );
+  }
+  const resolved = result.stdout.trim();
+  return isAbsolute(resolved) ? resolved : join(root, resolved);
+}
+
+/**
+ * Represents a git repository at a given root. Use to decide hook installation strategy
+ * without resolving all state up front (e.g. only resolve hooks dir when not using pre-commit).
+ */
+export class GitRepo {
+  readonly rootDir: string;
+  private _hooksDir: Promise<string> | null = null;
+
+  constructor(rootDir: string) {
+    this.rootDir = rootDir;
+  }
+
+  /** True if the repo uses the pre-commit framework (.pre-commit-config.yaml). */
+  usesPreCommitFramework(): boolean {
+    return existsSync(join(this.rootDir, PRE_COMMIT_CONFIG_FILE));
+  }
+
+  private async getHooksDirOnce(): Promise<string> {
+    this._hooksDir ??= resolveGitHooksDir(this.rootDir);
+    return this._hooksDir;
+  }
+
+  /** True if git's hooks path points to .husky (Husky is in use). */
+  async usesHusky(): Promise<boolean> {
+    const hooksDir = await this.getHooksDirOnce();
+    return toForwardSlash(hooksDir).startsWith(toForwardSlash(join(this.rootDir, '.husky')));
+  }
+
+  /** Resolved git hooks directory (core.hooksPath or .git/hooks). */
+  async getHooksDir(): Promise<string> {
+    return this.getHooksDirOnce();
+  }
+
+  /** Path to the Husky hook file for the given hook name (e.g. 'pre-commit', 'pre-push'). */
+  getHuskyHookPath(hook: string): string {
+    return join(this.rootDir, '.husky', hook);
+  }
+}
diff --git a/src/cli/commands/analyze/secrets.ts b/src/cli/commands/analyze/secrets.ts
@@ -315,13 +315,9 @@ function handleScanFailure(
   }
 
   if (exitCode === EXIT_CODE_SECRETS_FOUND) {
-    error(`Secrets found (${scanDurationMs}ms)`);
-    logger.error(`Secrets found, exit code: ${exitCode}`);
     throw new CommandFailedError(`Secrets found (${scanDurationMs}ms)`, exitCode);
   }
 
-  error(`Scan error (exit code ${exitCode}, ${scanDurationMs}ms)`);
-  logger.error(`Scan failed with exit code: ${exitCode}`);
   throw new CommandFailedError(`Scan error (exit code ${exitCode})`, exitCode);
 }
 
diff --git a/src/cli/commands/integrate/git/git-husky.ts b/src/cli/commands/integrate/git/git-husky.ts
@@ -23,20 +23,25 @@
 import { info, success } from '../../../../ui';
 import { HOOK_MARKER, getHuskySnippet } from './git-shell-fragments';
 import type { GitHookType } from '.';
+import { readFile, writeFile } from 'node:fs/promises';
 
 export async function installViaHusky(huskyHookPath: string, hook: GitHookType): Promise<void> {
-  const fs = await import('node:fs/promises');
   let content: string;
   try {
-    content = await fs.readFile(huskyHookPath, 'utf-8');
-  } catch {
-    content = '';
+    content = await readFile(huskyHookPath, 'utf-8');
+  } catch (error) {
+    const err = error as NodeJS.ErrnoException;
+    if (err.code === 'ENOENT') {
+      content = '';
+    } else {
+      throw error;
+    }
   }
   if (content.includes(HOOK_MARKER)) {
     info(`Secrets check already present in .husky/${hook}.`);
     return;
   }
   const newContent = content ? content.trimEnd() + getHuskySnippet(hook) : getHuskySnippet(hook);
-  await fs.writeFile(huskyHookPath, newContent, { encoding: 'utf-8', mode: 0o755 });
+  await writeFile(huskyHookPath, newContent, { encoding: 'utf-8', mode: 0o755 });
   success(`${hook} hook installed (Husky detected: added to .husky/${hook}).`);
 }
diff --git a/src/cli/commands/integrate/git/git-precommit-framework.ts b/src/cli/commands/integrate/git/git-precommit-framework.ts
@@ -25,7 +25,7 @@ import { join } from 'node:path';
 import yaml from 'js-yaml';
 import { spawnProcess } from '../../../../lib/process';
 import { CommandFailedError } from '../../_common/error';
-import { error, success } from '../../../../ui';
+import { success } from '../../../../ui';
 import type { GitHookType } from '.';
 
 export const PRE_COMMIT_CONFIG_FILE = '.pre-commit-config.yaml';
@@ -113,10 +113,18 @@ export function upsertPreCommitConfig(root: string, stage: GitHookType): void {
 }
 
 async function runPreCommitCommand(args: string[], cwd: string): Promise<void> {
-  const result = await spawnProcess('pre-commit', args, { cwd });
+  let result;
+  try {
+    result = await spawnProcess('pre-commit', args, { cwd });
+  } catch (error) {
+    const message = error instanceof Error ? error.message : String(error);
+    throw new CommandFailedError(`Failed to run pre-commit [${message}]`);
+  }
   if (result.exitCode !== 0) {
     const detail = [result.stderr, result.stdout].filter(Boolean).join('\n');
-    throw new Error(`pre-commit ${args.join(' ')} failed (exit code ${result.exitCode}) ${detail}`);
+    throw new CommandFailedError(
+      `pre-commit ${args.join(' ')} failed (exit code ${result.exitCode}) ${detail}`,
+    );
   }
 }
 
@@ -144,7 +152,6 @@ export async function installViaPreCommitFramework(root: string, hook: GitHookTy
     await runPreCommitInstall(root, hook);
   } catch {
     const errorMessage = `Updated ${PRE_COMMIT_CONFIG_FILE} but pre-commit commands failed. Install the pre-commit framework (e.g. pip install pre-commit) and run: pre-commit uninstall && pre-commit clean && pre-commit install${hook === 'pre-push' ? ' && pre-commit install --hook-type pre-push' : ''}`;
-    error(errorMessage);
     throw new CommandFailedError(errorMessage);
   }
   success(`${hook} hook installed (pre-commit framework: added to ${PRE_COMMIT_CONFIG_FILE}).`);
diff --git a/src/cli/commands/integrate/git/index.ts b/src/cli/commands/integrate/git/index.ts
@@ -20,16 +20,16 @@
 
 // Integrate command - install git hooks for secrets scanning
 
-import { existsSync, mkdirSync, readFileSync, statSync } from 'node:fs';
-import { isAbsolute, join } from 'node:path';
+import { existsSync, mkdirSync, readFileSync } from 'node:fs';
+import { join } from 'node:path';
 import { platform } from 'node:os';
 import { GLOBAL_HOOKS_DIR } from '../../../../lib/config-constants';
-import logger from '../../../../lib/logger';
 import { resolveAuth } from '../../../../lib/auth-resolver';
-import { discoverProject } from '../../_common/discovery';
+import { findGitRoot } from '../../_common/discovery';
 import { CommandFailedError, InvalidOptionError } from '../../_common/error';
 import { performSecretInstall } from '../../install/secrets';
 import { spawnProcess } from '../../../../lib/process';
+import * as fs from 'node:fs/promises';
 import {
   blank,
   confirmPrompt,
@@ -41,6 +41,7 @@ import {
   text,
   warn,
 } from '../../../../ui';
+import { GitRepo, resolveGitHooksDir } from '../../_common/git-repo';
 import { HOOK_MARKER, getHookScript } from './git-shell-fragments';
 import { installViaHusky } from './git-husky';
 import {
@@ -81,47 +82,7 @@ interface HookInstallation {
   hooksDir: string;
 }
 
-export async function resolveGitHooksDir(root: string): Promise<string> {
-  // core.hooksPath takes precedence over everything — it's what git actually uses to find hooks.
-  // Husky sets this to .husky; other tools (e.g. lefthook) may point elsewhere.
-  let configResult;
-  try {
-    configResult = await spawnProcess('git', ['config', 'core.hooksPath'], { cwd: root });
-  } catch {
-    configResult = null;
-  }
-  if (configResult?.exitCode === 0) {
-    const configured = configResult.stdout.trim();
-    if (configured) {
-      return isAbsolute(configured) ? configured : join(root, configured);
-    }
-  }
-
-  const dotGit = join(root, '.git');
-  try {
-    // Standard repo: .git is a directory — hooks live directly inside it, no subprocess needed
-    if (statSync(dotGit).isDirectory()) {
-      return join(dotGit, 'hooks');
-    }
-  } catch {
-    // .git doesn't exist; fall through to git rev-parse
-  }
-  // Worktree or submodule: .git is a file pointer — ask git for the real hooks path
-  let result;
-  try {
-    result = await spawnProcess('git', ['rev-parse', '--git-path', 'hooks'], { cwd: root });
-  } catch {
-    const errorMessage = 'git is not installed or not on PATH';
-    throw new CommandFailedError(errorMessage);
-  }
-  if (result.exitCode !== 0) {
-    const detail = [result.stderr, result.stdout].filter(Boolean).join('\n');
-    const errorMessage = `Could not resolve git hooks directory (exit code ${result.exitCode}) ${detail}`;
-    throw new CommandFailedError(errorMessage);
-  }
-  const resolved = result.stdout.trim();
-  return isAbsolute(resolved) ? resolved : join(root, resolved);
-}
+export { resolveGitHooksDir } from '../../_common/git-repo';
 
 export async function detectSonarHookInstallation(root: string): Promise<HookInstallation> {
   let hooksDir: string;
@@ -235,12 +196,11 @@ export async function installViaGitHooks(
 ): Promise<void> {
   mkdirSync(hooksDir, { recursive: true });
   const hookPath = join(hooksDir, hook);
-  const fs = await import('node:fs/promises');
   if (existsSync(hookPath)) {
     const existing = await fs.readFile(hookPath, 'utf-8');
     if (!existing.includes(HOOK_MARKER) && !force) {
       warn(`A different ${hook} hook already exists at ${hookPath}.`);
-      text('  Use --force to replace it, or add the secrets check manually.');
+      text('  Use --force to replace it.');
       throw new CommandFailedError(
         `Refusing to overwrite existing ${hook} hook at ${hookPath}. Use --force to replace.`,
       );
@@ -291,14 +251,13 @@ async function integrateGitGlobal(options: IntegrateGitOptions): Promise<void> {
       'core.hooksPath',
       toForwardSlash(GLOBAL_HOOKS_DIR),
     ]);
-  } catch {
-    const msg = 'git is not installed or not on PATH';
-    throw new CommandFailedError(msg);
+  } catch (error) {
+    const message = error instanceof Error ? error.message : String(error);
+    throw new CommandFailedError(`Failed to run git [${message}]`);
   }
   if (gitResult.exitCode !== 0) {
     const detail = [gitResult.stderr, gitResult.stdout].filter(Boolean).join('\n');
     const msg = `git config --global core.hooksPath failed (exit code ${gitResult.exitCode}): ${detail}`;
-    logger.error(msg);
     throw new CommandFailedError(msg);
   }
 
@@ -322,14 +281,14 @@ export async function integrateGit(options: IntegrateGitOptions): Promise<void>
     return integrateGitGlobal(options);
   }
 
-  const projectInfo = await discoverProject(process.cwd());
-  if (!projectInfo.isGitRepo) {
+  const { gitRoot, isGit } = findGitRoot(process.cwd());
+  if (!isGit) {
     const errorMessage =
       'No git repository found. Please run this command from inside a git repository, or use --global to install a global hook.';
     throw new CommandFailedError(errorMessage);
   }
 
-  text(`We will install the hook in this repository: ${projectInfo.rootDir}`);
+  text(`We will install the hook in this repository: ${gitRoot}`);
   blank();
 
   if (!options.nonInteractive) {
@@ -340,29 +299,22 @@ export async function integrateGit(options: IntegrateGitOptions): Promise<void>
   }
   blank();
 
-  const installation = await detectSonarHookInstallation(projectInfo.rootDir);
-  const useHusky = toForwardSlash(installation.hooksDir).startsWith(
-    toForwardSlash(join(projectInfo.rootDir, '.husky')),
-  );
-  const usePreCommitConfig = existsSync(join(projectInfo.rootDir, PRE_COMMIT_CONFIG_FILE));
-
+  const gitRepo = new GitRepo(gitRoot);
   const hook = await resolveHookType(options);
   text(`Hook: ${hook}`);
   blank();
 
   await ensureSonarSecrets();
 
-  const huskyHookPath = join(projectInfo.rootDir, '.husky', hook);
-
-  if (usePreCommitConfig) {
-    await installViaPreCommitFramework(projectInfo.rootDir, hook);
-  } else if (useHusky) {
-    await installViaHusky(huskyHookPath, hook);
+  if (gitRepo.usesPreCommitFramework()) {
+    await installViaPreCommitFramework(gitRepo.rootDir, hook);
+  } else if (await gitRepo.usesHusky()) {
+    await installViaHusky(gitRepo.getHuskyHookPath(hook), hook);
   } else {
-    await installViaGitHooks(installation.hooksDir, hook, options.force);
+    await installViaGitHooks(await gitRepo.getHooksDir(), hook, options.force);
   }
 
   showPostInstallInfo(hook);
-  await showInstallationStatus(projectInfo.rootDir);
+  await showInstallationStatus(gitRoot);
   showVerificationGuide(hook);
 }
diff --git a/tests/unit/git-precommit-framework.test.ts b/tests/unit/git-precommit-framework.test.ts
diff --git a/tests/unit/integrate-git.test.ts b/tests/unit/integrate-git.test.ts
diff --git a/tests/unit/secret-scan.test.ts b/tests/unit/secret-scan.test.ts

Original file line number	Diff line number	Diff line change
`@@ -162,7 +162,7 @@ export async function discoverProject(startDir: string): Promise<DiscoveredProje`
`162`	`162`	`return config;`
`163`	`163`	`}`
`164`	`164`
`165`		`-function findGitRoot(startDir: string): { gitRoot: string; isGit: boolean } {`
	`165`	`+export function findGitRoot(startDir: string): { gitRoot: string; isGit: boolean } {`
`166`	`166`	`let dir = startDir;`
`167`	`167`
`168`	`168`	`for (;;) {`
Original file line number	Diff line number	Diff line change
`@@ -315,13 +315,9 @@ function handleScanFailure(`
`315`	`315`	`}`
`316`	`316`
`317`	`317`	`if (exitCode === EXIT_CODE_SECRETS_FOUND) {`
`318`		- error(`Secrets found (${scanDurationMs}ms)`);
`319`		- logger.error(`Secrets found, exit code: ${exitCode}`);
`320`	`318`	throw new CommandFailedError(`Secrets found (${scanDurationMs}ms)`, exitCode);
`321`	`319`	`}`
`322`	`320`
`323`		- error(`Scan error (exit code ${exitCode}, ${scanDurationMs}ms)`);
`324`		- logger.error(`Scan failed with exit code: ${exitCode}`);
`325`	`321`	throw new CommandFailedError(`Scan error (exit code ${exitCode})`, exitCode);
`326`	`322`	`}`
`327`	`323`