tools --import: skip standard-enforced tools and patterns

[andrzej-janczak]

Problem: tools --import builds its disable set from the config file alone. When the repository follows a coding standard, the set includes tools that the standard enforces. The server rejects each of those with 409 (engine ToolEnabledStandardsWriter), so every import against such a repository fails partially, and the caller only learns which items were locked after the fact.

Why it matters: the autoconfig agent (autoconfig-setup-container, skill configure-codacy-cloud) runs this command with a repository token. Observed on prod runs (codacy-acme runtime, Snakestack): the first import hits 409 for standard-enforced tools, the agent then probes codacy pattern <tool> <id> -o json | jq .enabledBy one pattern at a time to find the locked set, and re-imports. That costs hundreds of API calls and minutes per run, and in one run pushed the job past its timeout. A repository token cannot read /coding-standards* (401 ProjectTokenNotAllowed), so the standard's content is not available upfront. The only signal it can read is enabledBy on the repository tools list and on the per-tool patterns list, which the import already has or can fetch cheaply.

Fix:

• Tool level: tools with non-empty settings.enabledBy are removed from the disable set before any API call.
• Pattern level: for each tool the import will reconfigure, fetch its currently enabled patterns once (paginated, enabled=true) and report the standard-enforced ones. No pattern write changes: the bulk reset already leaves standard-enforced patterns enabled server-side, and the import only sends enabled:true entries, so this path never 409s. Tools driven by their own configuration file are not fetched.
• Both kinds land in a new skipped[] (tool, optional patternId, standards, reason), printed inside the existing standards warning in the preview, counted in the summary line, and returned by -o json as {succeeded, failed, skipped} with everything else routed to stderr so stdout is clean JSON.
• --force bypasses the skip: standards are unlinked first, so enabledBy is stale and the previous behavior applies.

Not changed: the existing 409 handling in failed[] stays as a fallback, unlink behavior is untouched. Cost: at most one extra paginated listing per reconfigured tool.

Follow-up (separate PRs): the skill drops the per-pattern probe loop and maps skipped[] + failed[] into its conflicts[]; --reanalyze-and-wait gets a periodic progress line so non-TTY callers do not see silence.

[codacy-production]

Not up to standards ⛔

🔴 Issues 3 medium

Alerts:
⚠ 3 issues (≤ 0 issues of at least minor severity)

Results:
3 new issues

Category	Results
Complexity	3 medium

View in Codacy

🟢 Metrics 19 complexity · 16 duplication

Metric	Results
Complexity	19
Duplication	16

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

[codacy-production]

Pull Request Overview

The pull request introduces logic to skip standard-enforced tools during imports to avoid conflict errors, but the current implementation has several high-severity risks. The Codacy analysis indicates the changes are not up to standards, primarily due to increased complexity and code duplication.

Two major concerns must be addressed before merging: first, the fetchEnabledToolPatterns logic lacks tests for pagination, which could lead to incomplete data in large repositories. Second, the reconfiguration loop performs sequential API calls and lacks a safety check on optional properties, posing both performance and stability risks. The src/utils/import-config.ts file has reached a level of complexity that warrants refactoring, as it now handles too many responsibilities including orchestration, file I/O, and UI rendering.

About this PR

• The fetchEnabledToolPatterns function implements a pagination loop using cursors, but the current test suite only provides single-page mock responses. This leaves the pagination logic unverified.

2 comments outside of the diff

src/commands/tools.test.ts

_{line 1 ⚪ LOW RISK}
Nitpick: This test file is becoming difficult to navigate. Consider splitting it into separate files based on functional areas (e.g., tools.list.test.ts vs tools.import.test.ts) to keep the test suites focused and maintainable.

src/utils/import-config.ts

_{line 1 ⚪ LOW RISK}
This file has exceeded the 500-line threshold. Consider splitting it into focused modules: for example, moving the preview and execution logic into separate files, or extracting the API fetching helpers.

Test suggestions

• Verify that standard-locked tools are excluded from the disable API call and added to 'skipped'.
• Verify that standard-locked patterns are identified via API fetch and reported in the 'skipped' list.
• Verify that the --force flag disables standard-checking and proceeds with unlinking.
• Verify that the JSON output format includes the new 'skipped' array.
• Verify that fetchEnabledToolPatterns correctly handles paginated responses via cursors.

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Verify that fetchEnabledToolPatterns correctly handles paginated responses via cursors.

_{TIP How was this review? Give us feedback}

[codacy-production]

_{🟡 MEDIUM RISK}

The buildImportPreview function now has 10 parameters (limit 8), 94 lines of code (limit 50), and a cyclomatic complexity of 9 (limit 8). Refactoring the signature to use parameter objects and extracting the standard-enforced skip logic into a helper function would significantly improve readability and testability.

See Issue in Codacy
See Issue in Codacy
See Issue in Codacy

[codacy-production]

_{🔴 HIGH RISK}

Suggestion: These independent API calls should be performed in parallel to reduce the overall latency of the tools --import preview phase. Additionally, the 'patterns' property is optional; mapping over it without a check (as seen on line 274) will cause the command to crash if a tool in the config file omits that key. Use optional chaining or an array check to safely handle this.

Suggested change

	for (const r of toolsToReconfigure) {
	if (r.configTool.useLocalConfigurationFile) continue;
	const configuredPatternIds = new Set(r.configTool.patterns.map((p) => p.patternId));
	const currentlyEnabled = await fetchEnabledToolPatterns(
	provider,
	organization,
	repository,
	r.tool.uuid,
	);
	// bulk reset leaves standard-enforced patterns enabled server-side; report them only
	const locked = currentlyEnabled.filter(
	(cp) => patternEnforcedBy(cp).length > 0 && !configuredPatternIds.has(cp.patternDefinition.id),
	);
	for (const cp of locked) {
	skipped.push({
	tool: r.tool.name,
	patternId: cp.patternDefinition.id,
	standards: patternEnforcedBy(cp),
	reason: "enforced by coding standard",
	});
	}
	}
	const reconfigurationSkips = await Promise.all(toolsToReconfigure.map(async (r) => {
	if (r.configTool.useLocalConfigurationFile) return [];
	const currentlyEnabled = await fetchEnabledToolPatterns(provider, organization, repository, r.tool.uuid);
	const configuredPatternIds = new Set((r.configTool.patterns || []).map((p) => p.patternId));
	return currentlyEnabled
	.filter((cp) => patternEnforcedBy(cp).length > 0 && !configuredPatternIds.has(cp.patternDefinition.id))
	.map((cp) => ({
	tool: r.tool.name,
	patternId: cp.patternDefinition.id,
	standards: patternEnforcedBy(cp),
	reason: "enforced by coding standard",
	}));
	}));
	skipped.push(...reconfigurationSkips.flat());