From decd352192e2988b8cbc9b383c3481597f9fb478 Mon Sep 17 00:00:00 2001 From: Alejandro Rizzo Date: Wed, 27 May 2026 11:47:09 +0100 Subject: [PATCH 1/2] fix: preserve cloud-only tools and improve import error reporting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Don't disable tools the local CLI doesn't support (cloud-only tools stay enabled when not present in the config file) - Skip pattern reset when useLocalConfigurationFile is set — just enable the tool with the config file flag - Surface structured API error details (status code, response body) on import failures instead of generic messages - Add ship-it command for streamlined PR workflow Co-Authored-By: Claude Opus 4.6 (1M context) --- .changeset/tools-import-fixes.md | 5 + .claude/commands/ship-it.md | 219 +++++++++++++++++++++++++++++++ src/commands/tools.test.ts | 1 + src/commands/tools.ts | 34 ++++- src/utils/import-config.test.ts | 163 ++++++++++++++++++++++- src/utils/import-config.ts | 182 +++++++++++++++++++------ 6 files changed, 558 insertions(+), 46 deletions(-) create mode 100644 .changeset/tools-import-fixes.md create mode 100644 .claude/commands/ship-it.md diff --git a/.changeset/tools-import-fixes.md b/.changeset/tools-import-fixes.md new file mode 100644 index 0000000..90a14c8 --- /dev/null +++ b/.changeset/tools-import-fixes.md @@ -0,0 +1,5 @@ +--- +"@codacy/codacy-cloud-cli": patch +--- + +Fix tools import to preserve cloud-only tools (only disable tools the local CLI supports), handle config-file mode correctly (skip pattern reset when useLocalConfigurationFile is set), and surface structured API error details on import failures. diff --git a/.claude/commands/ship-it.md b/.claude/commands/ship-it.md new file mode 100644 index 0000000..ccbb0b2 --- /dev/null +++ b/.claude/commands/ship-it.md @@ -0,0 +1,219 @@ +--- +description: Changeset + branch + commit + push + PR for the current working tree +--- + +# Ship it + +Take the current uncommitted changes on `main` (or on a branch already derived +from `main` for this task) and turn them into an open PR. End-to-end: make +sure there's a changeset, cut a branch, commit, push, open the PR. This is a +user-triggered action — invoking this command IS the explicit authorisation +required by the repo's "never commit, push, or open PRs without asking" rule, +so you can proceed without further confirmation once you've sanity-checked +what's about to be shipped. + +**Arguments:** `$ARGUMENTS` + +Optional, space-separated, in any order: + +- A branch name (must not contain spaces; e.g. `feat/tools-command`). If + absent, derive one — see Phase 3. +- A bump type: `patch`, `minor`, or `major`. If absent, infer — see Phase 1. +- A quoted PR title (wrap in double quotes if it contains spaces). If absent, + derive from the commit/changeset — see Phase 5. + +--- + +## Phase 0: Sanity-check what's about to be shipped + +1. `git status --short` — confirm there are changes. If the working tree is + clean AND the current branch has no commits ahead of `origin/main`, + stop and tell the user there's nothing to ship. +2. `git diff --stat HEAD` — eyeball the scope. If the diff touches files + that look unrelated (e.g. `src/api/client/` auto-generated code, stray + lockfile churn, or secrets/`.env`/credentials), flag them and ask before + proceeding. +3. Confirm we're in a git repo with an `origin` remote pointing at GitHub. + If not, stop and ask. + +Do NOT run tests or builds here — that's the user's call. The command assumes +the user has already validated the change. + +--- + +## Phase 1: Ensure a changeset exists + +CI on `main` fails any PR without a changeset, so this step is mandatory. + +1. Check `.changeset/` for a **new** `.md` file — one that isn't already + committed on the current branch. Use: + + ```bash + git status --short .changeset/ | grep -E '^\?\?|^ M|^A ' | awk '{print $2}' + ``` + + plus `git diff --name-only HEAD .changeset/` for modified ones. + + If at least one uncommitted changeset file exists, you're done with this + phase — move on. + +2. If no changeset exists, create one. Determine the bump type: + - Use the argument if provided. + - Otherwise infer from the diff: bug fixes and comment/docs tweaks → `patch`; + new user-visible features or command additions → `minor`; anything that + changes a public API signature or removes a flag → `major`. When + genuinely ambiguous, default to `patch` and mention it in the + end-of-turn summary. + - If the change touches only CI config, docs outside the README, or + is a pure refactor with no user-facing change, use `npx changeset --empty` + instead — that satisfies the CI check without bumping versions. + +3. This is a single-package repo (`@codacy/codacy-cloud-cli`), so the + changeset frontmatter always lists that one package. + +4. Write the changeset as `.changeset/.md` (the slug should be + hyphenated and descriptive of the change, e.g. + `add-tools-command.md`). Frontmatter format: + + ``` + --- + "@codacy/codacy-cloud-cli": patch + --- + + + ``` + + Use `Write` for the file; do NOT run `npx changeset` interactively — it + requires a TTY. + +--- + +## Phase 2: Decide the base branch + +1. Current branch: `git branch --show-current`. +2. If already on a feature branch (not `main`), use that — don't create a + new one. Skip to Phase 4. +3. If on `main`, continue to Phase 3. + +--- + +## Phase 3: Create a branch + +1. Derive a branch name when the user didn't pass one: + - Prefix based on the change type: `fix/` for bug fixes, `feat/` for + features, `chore/` for tooling, `docs/` for documentation. + - Slug: two or three hyphenated words pulled from the changeset title or + the most descriptive file path (e.g. `feat/tools-command`). + - Keep it under ~40 chars. +2. Check the branch doesn't already exist: + + ```bash + git rev-parse --verify "refs/heads/" 2>/dev/null + ``` + + If it does, append `-2`, `-3`, … until you find a free name. + +3. Create and switch: + + ```bash + git checkout -b + ``` + +--- + +## Phase 4: Commit + +1. Stage the change set explicitly. Prefer named files over `git add -A` or + `git add .` to avoid accidentally committing `.env`, credentials, or + unrelated files you noticed in Phase 0. + + ```bash + git add + ``` + + Always include any `.changeset/*.md` you created. + +2. Draft the commit message: + - Subject: Conventional-Commits style, under ~72 chars + (`feat: add tools command with enable/disable support`). + - Body: 1–3 short paragraphs or bullets. Focus on the _why_. Reference + the bug report, ticket, or PR number if known. + - End with the standard Co-Authored-By trailer. + +3. Commit using a HEREDOC so the shell doesn't mangle newlines: + + ```bash + git commit -m "$(cat <<'EOF' + + + + + Co-Authored-By: Claude + EOF + )" + ``` + +4. If the pre-commit hook fails, fix the underlying issue, re-stage, and + create a **new** commit. Never `--amend` away a hook failure (the commit + didn't happen, so --amend would rewrite the previous one). + +5. Never pass `--no-verify` or `--no-gpg-sign` unless the user explicitly + asks — doing so silently skips the repo's quality gates. + +--- + +## Phase 5: Push and open the PR + +1. Push with upstream tracking: + + ```bash + git push -u origin + ``` + +2. Build the PR body. Template: + + ```markdown + ## Summary + + - <1–3 bullets describing the change and why> + + ## Test plan + + - [ ] + + 🤖 Generated with [Claude Code](https://claude.com/claude-code) + ``` + + The Summary should be tight — a reviewer should be able to understand the + change without reading the diff. The Test plan should list concrete + commands (e.g. `npm test`) rather than vague "verify it works" bullets. + +3. Open the PR: + + ```bash + gh pr create --title "" --body "$(cat <<'EOF' + <body> + EOF + )" + ``` + + Title rules: + - Under 70 chars. + - Conventional-Commits style, mirroring the commit subject (they can be + identical). + - Use the argument if provided; otherwise derive from the changeset title + or the commit subject. + +4. Capture the PR URL from `gh pr create`'s stdout and print it in the + end-of-turn summary. + +--- + +## Phase 6: Report + +One sentence on what shipped, plus the PR URL. Don't re-summarise the diff — +the PR body already does. If anything was skipped or changed from the +defaults (e.g. bump type defaulted to patch because ambiguous, branch name +had a suffix appended because of collision, pre-commit hook required a +retry), mention it in a single parenthetical line so the user can course- +correct if needed. diff --git a/src/commands/tools.test.ts b/src/commands/tools.test.ts index 5a779a7..9021814 100644 --- a/src/commands/tools.test.ts +++ b/src/commands/tools.test.ts @@ -286,6 +286,7 @@ describe("tools command", () => { configurable: true, }, ] as any); + vi.spyOn(importConfig, "getLocalSupportedToolIds").mockResolvedValue(["ESLint"]); vi.mocked(AnalysisService.getRepositoryWithAnalysis).mockResolvedValue({ data: { repository: { diff --git a/src/commands/tools.ts b/src/commands/tools.ts index 55272b3..41098a1 100644 --- a/src/commands/tools.ts +++ b/src/commands/tools.ts @@ -10,9 +10,11 @@ import { AnalysisTool } from "../api/client/models/AnalysisTool"; import { readConfigFile, fetchAllTools, + getLocalSupportedToolIds, buildImportPreview, printImportPreview, executeImport, + ImportFailure, } from "../utils/import-config"; import { confirmAction } from "../utils/prompt"; @@ -49,6 +51,27 @@ function printToolGroup(tools: AnalysisTool[], enabled: boolean): void { console.log(table.toString()); } +const MAX_ERROR_DETAILS = 5; + +function printImportErrors(failures: ImportFailure[]): void { + for (const f of failures) { + const status = f.status ? ` (${f.status})` : ""; + console.log(ansis.red(`✗ ${f.tool}: ${f.error}${status}`)); + + if (f.details.length === 0) continue; + + const shown = f.details.slice(0, MAX_ERROR_DETAILS); + for (const detail of shown) { + console.log(ansis.dim(` ${detail}`)); + } + const remaining = f.details.length - shown.length; + if (remaining > 0) { + console.log(ansis.dim(` ... and ${remaining} more`)); + } + } + console.log(); +} + export function registerToolsCommand(program: Command) { program .command("tools") @@ -94,8 +117,8 @@ Examples: // Read config file const config = readConfigFile(resolvedPath); - // Fetch current state in parallel - const [repoToolsResponse, allTools, repoResponse] = + // Fetch current state and local CLI info in parallel + const [repoToolsResponse, allTools, repoResponse, localToolIds] = await Promise.all([ AnalysisService.listRepositoryTools( provider, @@ -108,6 +131,7 @@ Examples: organization, repository, ), + getLocalSupportedToolIds(), ]); spinner.stop(); @@ -119,6 +143,7 @@ Examples: allTools, repoResponse.data.repository.standards, resolvedPath, + localToolIds, ); printImportPreview(preview, repository, Boolean(opts.force)); @@ -154,13 +179,16 @@ Examples: `${ansis.green("✓")} Configuration imported successfully.`, ); } else { + printImportErrors(result.failed); + console.log( ansis.yellow( `Import completed with ${result.failed.length} error(s):`, ), ); for (const f of result.failed) { - console.log(ansis.red(` ✗ ${f.tool}: ${f.error}`)); + const status = f.status ? ` (${f.status})` : ""; + console.log(ansis.red(` ✗ ${f.tool}: ${f.error}${status}`)); } if (result.succeeded.length > 0) { console.log( diff --git a/src/utils/import-config.test.ts b/src/utils/import-config.test.ts index e3344ba..799d3c6 100644 --- a/src/utils/import-config.test.ts +++ b/src/utils/import-config.test.ts @@ -159,7 +159,7 @@ describe("resolveToolId", () => { // ─── buildImportPreview ─────────────────────────────────────────────── describe("buildImportPreview", () => { - it("should categorize tools correctly", () => { + it("should categorize tools correctly with local CLI info", () => { const repoTools: AnalysisTool[] = [ makeRepoTool("uuid-eslint", "ESLint", true), makeRepoTool("uuid-checkov", "Checkov", true), @@ -181,7 +181,8 @@ describe("buildImportPreview", () => { ], }; - const preview = buildImportPreview(config, repoTools, allTools, [], "/test/path"); + const localToolIds = ["ESLint", "Pylint", "checkov", "remarklint"]; + const preview = buildImportPreview(config, repoTools, allTools, [], "/test/path", localToolIds); // ESLint is enabled and in config → reconfigure expect(preview.toolsToReconfigure).toHaveLength(1); @@ -191,14 +192,86 @@ describe("buildImportPreview", () => { expect(preview.toolsToEnable).toHaveLength(1); expect(preview.toolsToEnable[0].tool.name).toBe("Pylint"); - // Checkov is enabled but NOT in config → disable + // Checkov is enabled, locally supported, but NOT in config → disable expect(preview.toolsToDisable).toHaveLength(1); expect(preview.toolsToDisable[0].name).toBe("Checkov"); + expect(preview.cloudOnlyTools).toHaveLength(0); + expect(preview.localCliAvailable).toBe(true); expect(preview.totalPatterns).toBe(3); expect(preview.unresolvedTools).toHaveLength(0); }); + it("should leave cloud-only tools unchanged", () => { + const sonarSharpTool = makeTool({ uuid: "uuid-sonarsharp", name: "SonarSharp", shortName: "sonarsharp", prefix: "SonarSharp_" }); + const extendedAllTools = [...allTools, sonarSharpTool]; + + const repoTools: AnalysisTool[] = [ + makeRepoTool("uuid-eslint", "ESLint", true), + makeRepoTool("uuid-checkov", "Checkov", true), + makeRepoTool("uuid-sonarsharp", "SonarSharp", true), + ]; + + const config: CodacyConfig = { + version: 1, + metadata: { + repositoryId: null, + repositoryName: null, + createdAt: "2025-01-01", + updatedAt: "2025-01-01", + languages: [], + }, + tools: [ + { toolId: "ESLint", patterns: [{ patternId: "p1" }] }, + ], + }; + + // Local CLI supports ESLint and Checkov but NOT SonarSharp + const localToolIds = ["ESLint", "checkov"]; + const preview = buildImportPreview(config, repoTools, extendedAllTools, [], "/test/path", localToolIds); + + // ESLint is in config → reconfigure + expect(preview.toolsToReconfigure).toHaveLength(1); + expect(preview.toolsToReconfigure[0].tool.name).toBe("ESLint"); + + // Checkov is locally supported but not in config → disable + expect(preview.toolsToDisable).toHaveLength(1); + expect(preview.toolsToDisable[0].name).toBe("Checkov"); + + // SonarSharp is NOT locally supported → cloud-only, unchanged + expect(preview.cloudOnlyTools).toHaveLength(1); + expect(preview.cloudOnlyTools[0].name).toBe("SonarSharp"); + }); + + it("should not disable any tools when local CLI is unavailable", () => { + const repoTools: AnalysisTool[] = [ + makeRepoTool("uuid-eslint", "ESLint", true), + makeRepoTool("uuid-checkov", "Checkov", true), + ]; + + const config: CodacyConfig = { + version: 1, + metadata: { + repositoryId: null, + repositoryName: null, + createdAt: "2025-01-01", + updatedAt: "2025-01-01", + languages: [], + }, + tools: [ + { toolId: "ESLint", patterns: [{ patternId: "p1" }] }, + ], + }; + + // null = local CLI not available + const preview = buildImportPreview(config, repoTools, allTools, [], "/test/path", null); + + expect(preview.toolsToDisable).toHaveLength(0); + expect(preview.cloudOnlyTools).toHaveLength(0); + expect(preview.localCliAvailable).toBe(false); + expect(preview.toolsToReconfigure).toHaveLength(1); + }); + it("should report unresolved tools", () => { const config: CodacyConfig = { version: 1, @@ -252,7 +325,7 @@ describe("executeImport", () => { vi.clearAllMocks(); }); - it("should configure tools from config and disable tools not in config", async () => { + it("should configure tools from config and disable locally supported tools not in config", async () => { vi.mocked(AnalysisService.updateRepositoryToolPatterns).mockResolvedValue(undefined as any); vi.mocked(AnalysisService.configureTool).mockResolvedValue(undefined as any); @@ -285,6 +358,7 @@ describe("executeImport", () => { allTools, [], "/test/path", + ["ESLint", "checkov"], ); const result = await executeImport( @@ -330,7 +404,7 @@ describe("executeImport", () => { expect(result.failed).toHaveLength(0); }); - it("should pass useConfigurationFile when specified", async () => { + it("should skip pattern reset and use config file mode when useLocalConfigurationFile is true", async () => { vi.mocked(AnalysisService.updateRepositoryToolPatterns).mockResolvedValue(undefined as any); vi.mocked(AnalysisService.configureTool).mockResolvedValue(undefined as any); @@ -360,7 +434,10 @@ describe("executeImport", () => { mockSpinner as any, ); - // When no patterns, should still enable with useConfigurationFile + // Should NOT reset patterns when using config file mode + expect(AnalysisService.updateRepositoryToolPatterns).not.toHaveBeenCalled(); + + // Should enable with useConfigurationFile: true expect(AnalysisService.configureTool).toHaveBeenCalledWith( "gh", "test-org", "test-repo", "uuid-eslint", { enabled: true, useConfigurationFile: true }, @@ -464,6 +541,80 @@ describe("executeImport", () => { expect(result.failed).toHaveLength(1); expect(result.failed[0].tool).toBe("ESLint"); expect(result.failed[0].error).toContain("Conflict"); + expect(result.failed[0].details).toEqual([]); expect(result.succeeded).toContain("Pylint"); }); + + it("should extract details from ApiError with body.message", async () => { + const { ApiError } = await import("../api/client/core/ApiError"); + const apiError = new ApiError( + { method: "PUT", url: "/test" } as any, + { url: "/test", ok: false, status: 409, statusText: "Conflict", body: { message: "Tool is managed by coding standard 'Security'" } }, + "Conflict", + ); + + vi.mocked(AnalysisService.updateRepositoryToolPatterns).mockResolvedValue(undefined as any); + vi.mocked(AnalysisService.configureTool).mockRejectedValueOnce(apiError); + + const config: CodacyConfig = { + version: 1, + metadata: { repositoryId: null, repositoryName: null, createdAt: "", updatedAt: "", languages: [] }, + tools: [{ toolId: "ESLint", patterns: [{ patternId: "p1" }] }], + }; + + const preview = buildImportPreview(config, [], allTools, [], "/test/path"); + const result = await executeImport("gh", "org", "repo", preview, config, allTools, mockSpinner as any); + + expect(result.failed[0].status).toBe(409); + expect(result.failed[0].error).toBe("Conflict"); + expect(result.failed[0].details).toEqual(["Tool is managed by coding standard 'Security'"]); + }); + + it("should extract details from ApiError with body.errors array", async () => { + const { ApiError } = await import("../api/client/core/ApiError"); + const apiError = new ApiError( + { method: "PUT", url: "/test" } as any, + { url: "/test", ok: false, status: 400, statusText: "Bad Request", body: { errors: ["Pattern X not found", "Pattern Y is invalid"] } }, + "Bad Request", + ); + + vi.mocked(AnalysisService.updateRepositoryToolPatterns).mockResolvedValue(undefined as any); + vi.mocked(AnalysisService.configureTool).mockRejectedValueOnce(apiError); + + const config: CodacyConfig = { + version: 1, + metadata: { repositoryId: null, repositoryName: null, createdAt: "", updatedAt: "", languages: [] }, + tools: [{ toolId: "ESLint", patterns: [{ patternId: "p1" }] }], + }; + + const preview = buildImportPreview(config, [], allTools, [], "/test/path"); + const result = await executeImport("gh", "org", "repo", preview, config, allTools, mockSpinner as any); + + expect(result.failed[0].status).toBe(400); + expect(result.failed[0].details).toEqual(["Pattern X not found", "Pattern Y is invalid"]); + }); + + it("should extract details from ApiError with string body", async () => { + const { ApiError } = await import("../api/client/core/ApiError"); + const apiError = new ApiError( + { method: "PUT", url: "/test" } as any, + { url: "/test", ok: false, status: 500, statusText: "Internal Server Error", body: "Unexpected failure in tool configuration" }, + "Internal Server Error", + ); + + vi.mocked(AnalysisService.updateRepositoryToolPatterns).mockResolvedValue(undefined as any); + vi.mocked(AnalysisService.configureTool).mockRejectedValueOnce(apiError); + + const config: CodacyConfig = { + version: 1, + metadata: { repositoryId: null, repositoryName: null, createdAt: "", updatedAt: "", languages: [] }, + tools: [{ toolId: "ESLint", patterns: [{ patternId: "p1" }] }], + }; + + const preview = buildImportPreview(config, [], allTools, [], "/test/path"); + const result = await executeImport("gh", "org", "repo", preview, config, allTools, mockSpinner as any); + + expect(result.failed[0].status).toBe(500); + expect(result.failed[0].details).toEqual(["Unexpected failure in tool configuration"]); + }); }); diff --git a/src/utils/import-config.ts b/src/utils/import-config.ts index cc47f6b..47a9dae 100644 --- a/src/utils/import-config.ts +++ b/src/utils/import-config.ts @@ -1,4 +1,6 @@ import * as fs from "fs"; +import { exec } from "child_process"; +import { promisify } from "util"; import ansis from "ansis"; import pluralize from "pluralize"; import { CodacyConfig, CodacyToolConfig } from "../types/codacy-config"; @@ -9,8 +11,11 @@ import { ConfigurePattern } from "../api/client/models/ConfigurePattern"; import { AnalysisService } from "../api/client/services/AnalysisService"; import { ToolsService } from "../api/client/services/ToolsService"; import { CodingStandardsService } from "../api/client/services/CodingStandardsService"; +import { ApiError } from "../api/client/core/ApiError"; import type ora from "ora"; +const execAsync = promisify(exec); + export interface ResolvedTool { configTool: CodacyToolConfig; tool: Tool; @@ -22,11 +27,53 @@ export interface ImportPreview { toolsToEnable: ResolvedTool[]; toolsToReconfigure: ResolvedTool[]; unresolvedTools: string[]; + cloudOnlyTools: AnalysisTool[]; + localCliAvailable: boolean; totalPatterns: number; standards: CodingStandardInfo[]; configPath: string; } +export interface ImportFailure { + tool: string; + error: string; + status?: number; + details: string[]; +} + +function extractErrorDetails(err: unknown): Pick<ImportFailure, "error" | "status" | "details"> { + if (!(err instanceof ApiError)) { + return { + error: err instanceof Error ? err.message : String(err), + details: [], + }; + } + + const details: string[] = []; + const body = err.body; + + if (body && typeof body === "object") { + if (typeof body.message === "string") { + details.push(body.message); + } + if (Array.isArray(body.errors)) { + for (const e of body.errors) { + details.push(typeof e === "string" ? e : (e?.message ?? JSON.stringify(e))); + } + } + if (details.length === 0) { + const serialized = JSON.stringify(body); + if (serialized !== "{}" && serialized !== "null") { + details.push(serialized); + } + } + } else if (typeof body === "string" && body.length > 0) { + details.push(body); + } + + return { error: err.message, status: err.status, details }; +} + export function readConfigFile(filePath: string): CodacyConfig { if (!fs.existsSync(filePath)) { throw new Error(`Configuration file not found: ${filePath}`); @@ -85,12 +132,28 @@ export async function fetchAllTools(): Promise<Tool[]> { return all; } +export async function getLocalSupportedToolIds(): Promise<string[] | null> { + try { + const { stdout } = await execAsync("codacy-analysis info -f json", { + timeout: 30000, + }); + const info = JSON.parse(stdout); + if (!info.tools || !Array.isArray(info.tools)) return null; + return info.tools + .filter((t: any) => t.supported) + .map((t: any) => t.id as string); + } catch { + return null; + } +} + export function buildImportPreview( config: CodacyConfig, repoTools: AnalysisTool[], allTools: Tool[], standards: CodingStandardInfo[], configPath: string, + localToolIds?: string[] | null, ): ImportPreview { const resolved: ResolvedTool[] = []; const unresolvedTools: string[] = []; @@ -115,12 +178,31 @@ export function buildImportPreview( (r) => r.repoTool && r.repoTool.settings.isEnabled, ); - // Repo tools that are currently enabled but NOT in the config → disable + // Repo tools that are currently enabled but NOT in the config const resolvedUuids = new Set(resolved.map((r) => r.tool.uuid)); - const toolsToDisable = repoTools.filter( + const enabledNotInConfig = repoTools.filter( (rt) => rt.settings.isEnabled && !resolvedUuids.has(rt.uuid), ); + // Only disable tools the local CLI supports; leave cloud-only tools unchanged + const localCliAvailable = localToolIds != null; + let toolsToDisable: AnalysisTool[]; + let cloudOnlyTools: AnalysisTool[]; + + if (localToolIds) { + const localUuids = new Set( + localToolIds + .map((id) => resolveToolId(id, allTools)) + .filter((t): t is Tool => t !== undefined) + .map((t) => t.uuid), + ); + toolsToDisable = enabledNotInConfig.filter((rt) => localUuids.has(rt.uuid)); + cloudOnlyTools = enabledNotInConfig.filter((rt) => !localUuids.has(rt.uuid)); + } else { + toolsToDisable = []; + cloudOnlyTools = []; + } + const totalPatterns = config.tools.reduce( (sum, t) => sum + (Array.isArray(t.patterns) ? t.patterns.length : 0), 0, @@ -131,6 +213,8 @@ export function buildImportPreview( toolsToEnable, toolsToReconfigure, unresolvedTools, + cloudOnlyTools, + localCliAvailable, totalPatterns, standards, configPath, @@ -166,6 +250,16 @@ export function printImportPreview( console.log(); } + // Local CLI availability warning + if (!preview.localCliAvailable) { + console.log( + ansis.yellow( + "⚠ Could not query codacy-analysis CLI. No tools will be disabled — only tools in the config will be enabled/reconfigured.", + ), + ); + console.log(); + } + // Unresolved tools warning if (preview.unresolvedTools.length > 0) { console.log( @@ -176,6 +270,16 @@ export function printImportPreview( console.log(); } + // Cloud-only tools (unchanged) + if (preview.cloudOnlyTools.length > 0) { + const names = preview.cloudOnlyTools.map((t) => t.name).join(", "); + console.log( + ansis.dim( + `${preview.cloudOnlyTools.length} cloud-only ${pluralize("tool", preview.cloudOnlyTools.length)} unchanged: ${names}`, + ), + ); + } + // Tools to disable if (preview.toolsToDisable.length > 0) { const names = preview.toolsToDisable.map((t) => t.name).join(", "); @@ -242,9 +346,9 @@ export async function executeImport( allTools: Tool[], spinner: ReturnType<typeof ora>, force: boolean = false, -): Promise<{ succeeded: string[]; failed: { tool: string; error: string }[] }> { +): Promise<{ succeeded: string[]; failed: ImportFailure[] }> { const succeeded: string[] = []; - const failed: { tool: string; error: string }[] = []; + const failed: ImportFailure[] = []; // Unlink coding standards when --force is used if (force) { @@ -260,7 +364,7 @@ export async function executeImport( } catch (err) { failed.push({ tool: `Standard: ${standard.name}`, - error: err instanceof Error ? err.message : String(err), + ...extractErrorDetails(err), }); } } @@ -271,54 +375,58 @@ export async function executeImport( for (const resolved of allResolved) { spinner.text = `Configuring ${resolved.tool.name}...`; try { - // Disable all existing patterns first - await AnalysisService.updateRepositoryToolPatterns( - provider, - organization, - repository, - resolved.tool.uuid, - { enabled: false }, - ); - - // Build patterns and batch - const patterns = buildConfigurePatterns(resolved.configTool); - const batches = chunk(patterns, 1000); - - for (const batch of batches) { + if (resolved.configTool.useLocalConfigurationFile) { + // Config file mode: just enable the tool with the config file flag, no pattern changes await AnalysisService.configureTool( provider, organization, repository, resolved.tool.uuid, - { - enabled: true, - useConfigurationFile: - resolved.configTool.useLocalConfigurationFile ?? false, - patterns: batch, - }, + { enabled: true, useConfigurationFile: true }, ); - } - - // If no patterns, still enable the tool with the config file setting - if (batches.length === 0) { - await AnalysisService.configureTool( + } else { + // Pattern mode: reset existing patterns, then apply new ones + await AnalysisService.updateRepositoryToolPatterns( provider, organization, repository, resolved.tool.uuid, - { - enabled: true, - useConfigurationFile: - resolved.configTool.useLocalConfigurationFile ?? false, - }, + { enabled: false }, ); + + const patterns = buildConfigurePatterns(resolved.configTool); + const batches = chunk(patterns, 1000); + + for (const batch of batches) { + await AnalysisService.configureTool( + provider, + organization, + repository, + resolved.tool.uuid, + { + enabled: true, + useConfigurationFile: false, + patterns: batch, + }, + ); + } + + if (batches.length === 0) { + await AnalysisService.configureTool( + provider, + organization, + repository, + resolved.tool.uuid, + { enabled: true, useConfigurationFile: false }, + ); + } } succeeded.push(resolved.tool.name); } catch (err) { failed.push({ tool: resolved.tool.name, - error: err instanceof Error ? err.message : String(err), + ...extractErrorDetails(err), }); } } @@ -338,7 +446,7 @@ export async function executeImport( } catch (err) { failed.push({ tool: tool.name, - error: err instanceof Error ? err.message : String(err), + ...extractErrorDetails(err), }); } } From b4dde94476d14e5c828e6cef78ae356f41bb9c11 Mon Sep 17 00:00:00 2001 From: Alejandro Rizzo <alerizzo@gmail.com> Date: Wed, 27 May 2026 12:00:20 +0100 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20dedupe=20error=20output,=20reduce=20complexity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove duplicate failure output: summary header prints first, then printImportErrors handles the full detail listing (no redundant loop) - Extract parseApiErrorBody from extractErrorDetails to bring cyclomatic complexity under the Lizard CCN limit of 10 - Add defensive filtering in getLocalSupportedToolIds (typeof id check) - Fix preview wording: pattern-mode and config-file-mode tools are now described separately instead of a blanket "all patterns replaced" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --- src/commands/tools.ts | 7 +--- src/utils/import-config.ts | 73 ++++++++++++++++++++++++-------------- 2 files changed, 48 insertions(+), 32 deletions(-) diff --git a/src/commands/tools.ts b/src/commands/tools.ts index 41098a1..2999ee8 100644 --- a/src/commands/tools.ts +++ b/src/commands/tools.ts @@ -179,17 +179,12 @@ Examples: `${ansis.green("✓")} Configuration imported successfully.`, ); } else { - printImportErrors(result.failed); - console.log( ansis.yellow( `Import completed with ${result.failed.length} error(s):`, ), ); - for (const f of result.failed) { - const status = f.status ? ` (${f.status})` : ""; - console.log(ansis.red(` ✗ ${f.tool}: ${f.error}${status}`)); - } + printImportErrors(result.failed); if (result.succeeded.length > 0) { console.log( ansis.green( diff --git a/src/utils/import-config.ts b/src/utils/import-config.ts index 47a9dae..d150301 100644 --- a/src/utils/import-config.ts +++ b/src/utils/import-config.ts @@ -41,24 +41,16 @@ export interface ImportFailure { details: string[]; } -function extractErrorDetails(err: unknown): Pick<ImportFailure, "error" | "status" | "details"> { - if (!(err instanceof ApiError)) { - return { - error: err instanceof Error ? err.message : String(err), - details: [], - }; - } - - const details: string[] = []; - const body = err.body; - +function parseApiErrorBody(body: unknown): string[] { if (body && typeof body === "object") { - if (typeof body.message === "string") { - details.push(body.message); + const details: string[] = []; + const obj = body as Record<string, unknown>; + if (typeof obj.message === "string") { + details.push(obj.message); } - if (Array.isArray(body.errors)) { - for (const e of body.errors) { - details.push(typeof e === "string" ? e : (e?.message ?? JSON.stringify(e))); + if (Array.isArray(obj.errors)) { + for (const e of obj.errors) { + details.push(typeof e === "string" ? e : ((e as any)?.message ?? JSON.stringify(e))); } } if (details.length === 0) { @@ -67,11 +59,22 @@ function extractErrorDetails(err: unknown): Pick<ImportFailure, "error" | "statu details.push(serialized); } } - } else if (typeof body === "string" && body.length > 0) { - details.push(body); + return details; + } + if (typeof body === "string" && body.length > 0) { + return [body]; } + return []; +} - return { error: err.message, status: err.status, details }; +function extractErrorDetails(err: unknown): Pick<ImportFailure, "error" | "status" | "details"> { + if (!(err instanceof ApiError)) { + return { + error: err instanceof Error ? err.message : String(err), + details: [], + }; + } + return { error: err.message, status: err.status, details: parseApiErrorBody(err.body) }; } export function readConfigFile(filePath: string): CodacyConfig { @@ -140,7 +143,7 @@ export async function getLocalSupportedToolIds(): Promise<string[] | null> { const info = JSON.parse(stdout); if (!info.tools || !Array.isArray(info.tools)) return null; return info.tools - .filter((t: any) => t.supported) + .filter((t: any) => t && t.supported && typeof t.id === "string") .map((t: any) => t.id as string); } catch { return null; @@ -304,14 +307,32 @@ export function printImportPreview( ); } - console.log(); - console.log( - `All existing patterns in configured tools will be replaced with the patterns in ${ansis.bold(preview.configPath)}.`, + const allResolved = [ + ...preview.toolsToEnable, + ...preview.toolsToReconfigure, + ]; + const configFileTools = allResolved.filter( + (r) => r.configTool.useLocalConfigurationFile, ); - console.log(); - console.log( - `${ansis.bold(String(preview.totalPatterns))} ${pluralize("pattern", preview.totalPatterns)} will be enabled.`, + const patternTools = allResolved.filter( + (r) => !r.configTool.useLocalConfigurationFile, ); + + console.log(); + if (patternTools.length > 0) { + console.log( + `Existing patterns in ${patternTools.length} ${pluralize("tool", patternTools.length)} will be replaced with the patterns in ${ansis.bold(preview.configPath)}.`, + ); + console.log( + `${ansis.bold(String(preview.totalPatterns))} ${pluralize("pattern", preview.totalPatterns)} will be enabled.`, + ); + } + if (configFileTools.length > 0) { + const names = configFileTools.map((r) => r.tool.name).join(", "); + console.log( + `${configFileTools.length} ${pluralize("tool", configFileTools.length)} will use their local configuration file: ${names}`, + ); + } } function chunk<T>(arr: T[], size: number): T[][] {