From 7879215cca3e0a2d9a6ac34c43929c467b113a89 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sun, 15 Mar 2026 21:16:00 +0100 Subject: [PATCH] Add CLI module deduplication and improve conflict reporting Individual CLI modules now silently take precedence over array-sourced modules (e.g. from cli-defaults) when their commands overlap. Conflict errors between non-array modules include both package names for easier debugging. Commands reference their parent module instead of storing a raw package name string. createCliModule now validates that packageJson has a name. Signed-off-by: Patrik Oldsberg Made-with: Cursor --- packages/cli-defaults/report.api.md | 13 ++ packages/cli-defaults/src/index.ts | 5 + .../cli-internal/src/InternalCommandNode.ts | 3 +- .../src/cli-module/createCliModule.ts | 6 + .../cli/src/wiring/CliInitializer.test.ts | 134 ++++++++++++++++++ packages/cli/src/wiring/CliInitializer.ts | 59 +++++--- packages/cli/src/wiring/CommandGraph.ts | 45 +++++- 7 files changed, 244 insertions(+), 21 deletions(-) create mode 100644 packages/cli-defaults/report.api.md diff --git a/packages/cli-defaults/report.api.md b/packages/cli-defaults/report.api.md new file mode 100644 index 0000000000..573f8ee454 --- /dev/null +++ b/packages/cli-defaults/report.api.md @@ -0,0 +1,13 @@ +## API Report File for "@backstage/cli-defaults" + +> Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/). + +```ts +import { CliModule } from '@backstage/cli-node'; + +// @public +const _default: CliModule[]; +export default _default; + +// (No @packageDocumentation comment for this package) +``` diff --git a/packages/cli-defaults/src/index.ts b/packages/cli-defaults/src/index.ts index 95c9052b1d..0126d0c074 100644 --- a/packages/cli-defaults/src/index.ts +++ b/packages/cli-defaults/src/index.ts @@ -25,6 +25,11 @@ import newModule from '@backstage/cli-module-new'; import testJest from '@backstage/cli-module-test-jest'; import translations from '@backstage/cli-module-translations'; +/** + * The default set of CLI modules for the Backstage CLI. + * + * @public + */ export default [ auth, build, diff --git a/packages/cli-internal/src/InternalCommandNode.ts b/packages/cli-internal/src/InternalCommandNode.ts index 3d93a5128d..bfa1f90baa 100644 --- a/packages/cli-internal/src/InternalCommandNode.ts +++ b/packages/cli-internal/src/InternalCommandNode.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { CliCommand } from '@backstage/cli-node'; +import { CliCommand, CliModule } from '@backstage/cli-node'; import { OpaqueType } from '@internal/opaque'; /** @internal */ @@ -47,6 +47,7 @@ export const OpaqueCommandLeafNode = OpaqueType.create<{ readonly version: 'v1'; readonly name: string; readonly command: CliCommand; + readonly module?: CliModule; }; }>({ type: '@backstage/CommandLeafNode', diff --git a/packages/cli-node/src/cli-module/createCliModule.ts b/packages/cli-node/src/cli-module/createCliModule.ts index 89ed21876f..7d52e9bf43 100644 --- a/packages/cli-node/src/cli-module/createCliModule.ts +++ b/packages/cli-node/src/cli-module/createCliModule.ts @@ -55,6 +55,12 @@ export function createCliModule(options: { addCommand: (command: CliCommand) => void; }) => Promise; }): CliModule { + if (!options.packageJson.name) { + throw new Error( + 'The packageJson provided to createCliModule must have a name', + ); + } + const commands: CliCommand[] = []; const commandsPromise = options .init({ addCommand: command => commands.push(command) }) diff --git a/packages/cli/src/wiring/CliInitializer.test.ts b/packages/cli/src/wiring/CliInitializer.test.ts index b5ac9cfaaa..703ec972fc 100644 --- a/packages/cli/src/wiring/CliInitializer.test.ts +++ b/packages/cli/src/wiring/CliInitializer.test.ts @@ -19,6 +19,24 @@ import { createCliModule } from './factory'; process.exit = jest.fn() as any; +describe('createCliModule', () => { + it('should throw if packageJson has no name', () => { + expect(() => + createCliModule({ + packageJson: { name: '' }, + init: async () => {}, + }), + ).toThrow('The packageJson provided to createCliModule must have a name'); + + expect(() => + createCliModule({ + packageJson: {} as any, + init: async () => {}, + }), + ).toThrow('The packageJson provided to createCliModule must have a name'); + }); +}); + describe('CliInitializer', () => { beforeEach(() => { jest.resetAllMocks(); @@ -239,4 +257,120 @@ describe('CliInitializer', () => { await initializer.run(); expect(process.exit).toHaveBeenCalledWith(0); }); + + it('should silently override array-sourced module with conflicting individual module while keeping siblings', async () => { + expect.assertions(3); + process.argv = ['node', 'cli', 'test']; + const initializer = new CliInitializer(); + + const individualModule = createCliModule({ + packageJson: { name: '@backstage/individual' }, + init: async reg => + reg.addCommand({ + path: ['test'], + description: 'individual test', + execute: ({ args }) => { + expect(args).toEqual([]); + return Promise.resolve(); + }, + }), + }); + + const conflictingArrayModule = createCliModule({ + packageJson: { name: '@backstage/array-conflict' }, + init: async reg => + reg.addCommand({ + path: ['test'], + description: 'array test (should be skipped)', + execute: () => Promise.resolve(), + }), + }); + + const nonConflictingArrayModule = createCliModule({ + packageJson: { name: '@backstage/array-sibling' }, + init: async reg => + reg.addCommand({ + path: ['other'], + description: 'other command', + execute: () => Promise.resolve(), + }), + }); + + initializer.add(individualModule); + initializer.add([conflictingArrayModule, nonConflictingArrayModule]); + + await initializer.run(); + expect(process.exit).toHaveBeenCalledWith(0); + + // Verify the sibling command is available by running it + process.argv = ['node', 'cli', 'other']; + const initializer2 = new CliInitializer(); + initializer2.add(individualModule); + initializer2.add([conflictingArrayModule, nonConflictingArrayModule]); + await initializer2.run(); + expect(process.exit).toHaveBeenCalledTimes(2); + }); + + it('should error with package names when two individual modules conflict', async () => { + const initializer = new CliInitializer(); + + initializer.add( + createCliModule({ + packageJson: { name: '@backstage/module-a' }, + init: async reg => + reg.addCommand({ + path: ['conflicting'], + description: 'from module A', + execute: () => Promise.resolve(), + }), + }), + ); + + initializer.add( + createCliModule({ + packageJson: { name: '@backstage/module-b' }, + init: async reg => + reg.addCommand({ + path: ['conflicting'], + description: 'from module B', + execute: () => Promise.resolve(), + }), + }), + ); + + await expect(initializer.run()).rejects.toThrow( + 'Command "conflicting" from "@backstage/module-b" conflicts with an existing command from "@backstage/module-a"', + ); + }); + + it('should error with package names when two array-sourced modules conflict', async () => { + const initializer = new CliInitializer(); + + const moduleA = createCliModule({ + packageJson: { name: '@backstage/array-a' }, + init: async reg => + reg.addCommand({ + path: ['shared'], + description: 'from array A', + execute: () => Promise.resolve(), + }), + }); + + const moduleB = createCliModule({ + packageJson: { name: '@backstage/array-b' }, + init: async reg => + reg.addCommand({ + path: ['shared'], + description: 'from array B', + execute: () => Promise.resolve(), + }), + }); + + initializer.add([moduleA]); + initializer.add([moduleB]); + + await expect(initializer.run()).rejects.toThrow( + 'Command "shared" from "@backstage/array-b" conflicts with an existing command from "@backstage/array-a"', + ); + }); }); diff --git a/packages/cli/src/wiring/CliInitializer.ts b/packages/cli/src/wiring/CliInitializer.ts index 4e1067d780..e2c46cf99b 100644 --- a/packages/cli/src/wiring/CliInitializer.ts +++ b/packages/cli/src/wiring/CliInitializer.ts @@ -22,7 +22,6 @@ import { } from '@internal/cli'; import type { CommandNode } from '@internal/cli'; import type { CliModule } from '@backstage/cli-node'; -import { CommandRegistry } from './CommandRegistry'; import { Command } from 'commander'; import { version } from './version'; import chalk from 'chalk'; @@ -44,28 +43,42 @@ type UninitializedFeature = | CliModule[] | Promise<{ default: CliModule | CliModule[] }>; +interface TaggedFeature { + feature: CliModule; + fromArray: boolean; +} + export class CliInitializer { private graph = new CommandGraph(); - private commandRegistry = new CommandRegistry(this.graph); - #uninitiazedFeatures: Promise[] = []; + #uninitiazedFeatures: Promise[] = []; add(feature: UninitializedFeature) { if (isPromise(feature)) { this.#uninitiazedFeatures.push( - feature.then(f => unwrapFeature(f.default)), + feature.then(f => { + const unwrapped = unwrapFeature(f.default); + if (Array.isArray(unwrapped)) { + return unwrapped.map(m => ({ feature: m, fromArray: true })); + } + return [{ feature: unwrapped, fromArray: false }]; + }), ); } else if (Array.isArray(feature)) { - this.#uninitiazedFeatures.push(Promise.resolve(feature)); + this.#uninitiazedFeatures.push( + Promise.resolve(feature.map(m => ({ feature: m, fromArray: true }))), + ); } else { - this.#uninitiazedFeatures.push(Promise.resolve(feature)); + this.#uninitiazedFeatures.push( + Promise.resolve([{ feature, fromArray: false }]), + ); } } async #register(feature: CliModule) { if (OpaqueCliModule.isType(feature)) { - const internal = OpaqueCliModule.toInternal(feature); - for (const command of await internal.commands) { - this.commandRegistry.addCommand(command); + for (const command of await OpaqueCliModule.toInternal(feature) + .commands) { + this.graph.add(command, feature); } } else { throw new Error(`Unsupported feature type: ${(feature as any).$$type}`); @@ -73,15 +86,29 @@ export class CliInitializer { } async #doInit() { - const resolved = await Promise.all(this.#uninitiazedFeatures); - for (const featureOrArray of resolved) { - const features = Array.isArray(featureOrArray) - ? featureOrArray - : [featureOrArray]; - for (const feature of features) { - await this.#register(feature); + const resolvedGroups = await Promise.all(this.#uninitiazedFeatures); + const allFeatures = resolvedGroups.flat(); + + // Collect command paths from individually-added modules + const individualPaths = new Set(); + for (const { feature, fromArray } of allFeatures) { + if (!fromArray && OpaqueCliModule.isType(feature)) { + const cmds = await OpaqueCliModule.toInternal(feature).commands; + for (const cmd of cmds) { + individualPaths.add(cmd.path.join(' ')); + } } } + + for (const { feature, fromArray } of allFeatures) { + if (fromArray && OpaqueCliModule.isType(feature)) { + const cmds = await OpaqueCliModule.toInternal(feature).commands; + if (cmds.some(cmd => individualPaths.has(cmd.path.join(' ')))) { + continue; + } + } + await this.#register(feature); + } } /** diff --git a/packages/cli/src/wiring/CommandGraph.ts b/packages/cli/src/wiring/CommandGraph.ts index b3db5d68bb..b201a3fba3 100644 --- a/packages/cli/src/wiring/CommandGraph.ts +++ b/packages/cli/src/wiring/CommandGraph.ts @@ -17,8 +17,9 @@ import { CommandNode, OpaqueCommandTreeNode, OpaqueCommandLeafNode, + OpaqueCliModule, } from '@internal/cli'; -import { CliCommand } from './types'; +import { CliCommand, CliModule } from './types'; /** * A sparse graph of commands. @@ -30,7 +31,7 @@ export class CommandGraph { * Adds a command to the graph. The graph is sparse, so we use the path to determine the nodes * to traverse. Only leaf nodes should have a command/action. */ - add(command: CliCommand) { + add(command: CliCommand, module?: CliModule) { const path = command.path; let current = this.graph; for (let i = 0; i < path.length - 1; i++) { @@ -50,7 +51,11 @@ export class CommandGraph { current.push(next); } else if (OpaqueCommandLeafNode.isType(next)) { throw new Error( - `Command already exists at path: "${path.slice(0, i).join(' ')}"`, + formatConflictError( + path, + module, + OpaqueCommandLeafNode.toInternal(next).module, + ), ); } current = OpaqueCommandTreeNode.toInternal(next).children; @@ -64,13 +69,18 @@ export class CommandGraph { }); if (last && OpaqueCommandLeafNode.isType(last)) { throw new Error( - `Command already exists at path: "${path.slice(0, -1).join(' ')}"`, + formatConflictError( + path, + module, + OpaqueCommandLeafNode.toInternal(last).module, + ), ); } else { current.push( OpaqueCommandLeafNode.createInstance('v1', { name: lastName, command, + module, }), ); } @@ -119,3 +129,30 @@ export class CommandGraph { return current; } } + +function getModuleName(module?: CliModule): string | undefined { + if (module && OpaqueCliModule.isType(module)) { + return OpaqueCliModule.toInternal(module).packageName; + } + return undefined; +} + +function formatConflictError( + path: string[], + newModule?: CliModule, + existingModule?: CliModule, +): string { + const cmd = path.join(' '); + const newPkg = getModuleName(newModule); + const existingPkg = getModuleName(existingModule); + if (newPkg && existingPkg) { + return `Command "${cmd}" from "${newPkg}" conflicts with an existing command from "${existingPkg}"`; + } + if (newPkg) { + return `Command "${cmd}" from "${newPkg}" conflicts with an existing command`; + } + if (existingPkg) { + return `Command "${cmd}" conflicts with an existing command from "${existingPkg}"`; + } + return `Command "${cmd}" conflicts with an existing command`; +}