diff --git a/apps/website/docs/guide/02-commands/09-subcommand-hierarchy.mdx b/apps/website/docs/guide/02-commands/09-subcommand-hierarchy.mdx index 6cfaa290..c002c566 100644 --- a/apps/website/docs/guide/02-commands/09-subcommand-hierarchy.mdx +++ b/apps/website/docs/guide/02-commands/09-subcommand-hierarchy.mdx @@ -80,6 +80,46 @@ This produces: - `/ops status` - `/ops deploy` +## Non-Leaf Command Files + +A hierarchical node with children is a non-leaf node. When a directory +such as `[admin]` contains child subcommands or groups, its +`command.ts` file can define command metadata and configuration, but it +must not export executable handlers such as `chatInput`, `message`, or +`autocomplete`. + +Executable handlers belong in leaf nodes, such as `[logs]/command.ts` +or `logs.subcommand.ts`. + +Incorrect: + +```text title="Directory Structure" +src/app/commands/ +└── [admin]/ + ├── command.ts # Incorrect: exports chatInput while admin has children + └── [logs]/ + └── command.ts +``` + +Correct: + +```text title="Directory Structure" +src/app/commands/ +└── [admin]/ + ├── command.ts # Correct: metadata/config only + └── [logs]/ + └── command.ts # Correct: executable handlers live here +``` + +You can also use a shorthand leaf: + +```text title="Directory Structure" +src/app/commands/ +└── [admin]/ + ├── command.ts # metadata/config only + └── logs.subcommand.ts +``` + ## Middleware Scope In Hierarchical Trees Hierarchical commands use the same middleware model as flat commands: diff --git a/apps/website/docs/guide/08-advanced/02-file-naming-conventions.mdx b/apps/website/docs/guide/08-advanced/02-file-naming-conventions.mdx index 9a295574..ac1fd776 100644 --- a/apps/website/docs/guide/08-advanced/02-file-naming-conventions.mdx +++ b/apps/website/docs/guide/08-advanced/02-file-naming-conventions.mdx @@ -102,6 +102,13 @@ This transforms into: - `/settings notifications enable` - `/settings notifications disable` +`[command]/command.ts` may represent a parent command node. If that +command directory contains child subcommands or groups, its +`command.ts` file must only define metadata or configuration and must +not export executable handlers such as `chatInput`, `message`, or +`autocomplete`. Place those handlers in leaf command files, such as +`profile.subcommand.ts` or `[disable]/command.ts`. + Middleware inside these trees follows the same same-directory rule: - `+middleware.ts` applies only to leaves defined in that directory diff --git a/packages/commandkit/src/app/handlers/AppCommandHandler.hierarchical.test.ts b/packages/commandkit/src/app/handlers/AppCommandHandler.hierarchical.test.ts index 92b3fca7..dd3dc3a1 100644 --- a/packages/commandkit/src/app/handlers/AppCommandHandler.hierarchical.test.ts +++ b/packages/commandkit/src/app/handlers/AppCommandHandler.hierarchical.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, test } from 'vitest'; +import { afterEach, describe, expect, test, vi } from 'vitest'; import { Client, Collection, @@ -9,6 +9,7 @@ import { import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'; import { dirname, join } from 'node:path'; import { CommandKit } from '../../commandkit'; +import { Logger } from '../../logger/Logger'; import { AppCommandHandler } from './AppCommandHandler'; import { CommandsRouter } from '../router'; @@ -254,4 +255,90 @@ export async function message() {} await client.destroy(); } }); + + test('warns and ignores executable handlers from non-leaf nodes', async () => { + const warn = vi.spyOn(Logger, 'warn').mockImplementation(() => {}); + + const { client, handler } = await createHandlerWithCommands([ + [ + '[admin]/command.mjs', + ` +export const command = { + description: "Admin metadata", + dm_permission: false +}; + +export async function chatInput() {} +export async function message() {} +export async function autocomplete() {} +`, + ], + [ + '[admin]/[logs]/command.mjs', + ` +export const command = { + description: "Admin logs" +}; + +export async function chatInput() {} +export async function message() {} +`, + ], + ]); + + try { + expect(warn).toHaveBeenCalledWith( + expect.arrayContaining([ + 'Ignoring ', + ' exported by ', + ' hierarchical node ', + '. Move ', + ', ', + ', or ', + ' handlers to a ', + ' command node.', + ]), + expect.stringContaining('executable handlers'), + expect.stringContaining('[non-leaf]'), + expect.stringContaining('[admin]'), + expect.stringContaining('chatInput'), + expect.stringContaining('message'), + expect.stringContaining('autocomplete'), + expect.stringContaining('[leaf]'), + ); + + expect(warn.mock.calls[0]).toEqual( + expect.arrayContaining([ + expect.arrayContaining([ + ' hierarchical node ', + '. Move ', + ' command node.', + ]), + expect.stringContaining('[admin]'), + ]), + ); + + const hierarchicalNodes = handler.getHierarchicalNodesArray(); + const adminNode = hierarchicalNodes.find((command) => { + return command.command.name === 'admin'; + }); + + expect(adminNode?.data.command.description).toBe('Admin metadata'); + expect(adminNode?.data.command.dm_permission).toBe(false); + expect(adminNode?.data.chatInput).toBeUndefined(); + expect(adminNode?.data.message).toBeUndefined(); + expect(adminNode?.data.autocomplete).toBeUndefined(); + + const runtimeRouteKeys = handler + .getRuntimeCommandsArray() + .map((command) => { + return (command.data.command as Record).__routeKey; + }); + + expect(runtimeRouteKeys).toEqual(['admin.logs']); + } finally { + warn.mockRestore(); + await client.destroy(); + } + }); }); diff --git a/packages/commandkit/src/app/handlers/AppCommandHandler.ts b/packages/commandkit/src/app/handlers/AppCommandHandler.ts index 6f0908bf..fb4448c8 100644 --- a/packages/commandkit/src/app/handlers/AppCommandHandler.ts +++ b/packages/commandkit/src/app/handlers/AppCommandHandler.ts @@ -1301,6 +1301,9 @@ export class AppCommandHandler { commandFileData.message || commandFileData.autocomplete ); + const sanitizedCommandFileData: AppCommandNative = { + ...commandFileData, + }; if (!isRootHierarchyLeaf && hasContextMenuHandlers) { throw new Error( @@ -1315,9 +1318,11 @@ export class AppCommandHandler { } if (!node.executable && hasExecutableSlashHandlers) { - throw new Error( - `Invalid export for hierarchical node ${routeKey}: non-leaf hierarchical nodes cannot export executable slash/prefix handlers`, - ); + Logger.warn`Ignoring ${colors.yellow('executable handlers')} exported by ${colors.cyan('[non-leaf]')} hierarchical node ${colors.magenta(`[${routeKey}]`)}. Move ${colors.yellow('chatInput')}, ${colors.yellow('message')}, or ${colors.yellow('autocomplete')} handlers to a ${colors.green('[leaf]')} command node.`; + + delete sanitizedCommandFileData.chatInput; + delete sanitizedCommandFileData.message; + delete sanitizedCommandFileData.autocomplete; } const loadedCommand: LoadedCommand = { @@ -1325,7 +1330,7 @@ export class AppCommandHandler { command, metadata: resolvedMetadata, data: { - ...commandFileData, + ...sanitizedCommandFileData, metadata: resolvedMetadata, command: { ...commandJson, diff --git a/packages/commandkit/src/app/router/CommandsRouter.test.ts b/packages/commandkit/src/app/router/CommandsRouter.test.ts index f7e0fc3d..4b73a0ab 100644 --- a/packages/commandkit/src/app/router/CommandsRouter.test.ts +++ b/packages/commandkit/src/app/router/CommandsRouter.test.ts @@ -157,6 +157,29 @@ describe('CommandsRouter', () => { ]); }); + test('warns when a non-leaf command node has a definition file', async () => { + const entrypoint = await createCommandsFixture([ + ['[admin]/command.ts'], + ['[admin]/[logs]/command.ts'], + ]); + + const router = new CommandsRouter({ entrypoint }); + const result = await router.scan(); + const diagnostics = result.diagnostics ?? []; + const diagnostic = diagnostics.find((diag) => { + return diag.code === 'NON_LEAF_NODE_MAY_HAVE_HANDLERS'; + }); + + expect(Object.keys(result.compiledRoutes ?? {})).toEqual(['admin.logs']); + expect(diagnostic).toMatchObject({ + message: + 'Hierarchical node "admin" has child nodes. Ensure its command.ts file does not export chatInput, message, or autocomplete handlers. Executable handlers must be defined on leaf nodes.', + }); + expect(normalizePath(diagnostic?.path ?? '')).toBe( + normalizePath(join(entrypoint, '[admin]/command.ts')), + ); + }); + test('emits diagnostics for invalid hierarchical structures', async () => { const entrypoint = await createCommandsFixture([ ['{oops}/group.ts'], @@ -172,6 +195,7 @@ describe('CommandsRouter', () => { const diagnosticCodes = (result.diagnostics ?? []).map((diag) => diag.code); expect(diagnosticCodes).toContain('ROOT_GROUP_NOT_ALLOWED'); + expect(diagnosticCodes).toContain('NON_LEAF_NODE_MAY_HAVE_HANDLERS'); expect(diagnosticCodes).toContain('DUPLICATE_SIBLING_TOKEN'); expect(diagnosticCodes).toContain('MIXED_ROOT_CHILDREN'); expect(diagnosticCodes).toContain('MISSING_COMMAND_DEFINITION'); diff --git a/packages/commandkit/src/app/router/CommandsRouter.ts b/packages/commandkit/src/app/router/CommandsRouter.ts index 2972f49c..02de9034 100644 --- a/packages/commandkit/src/app/router/CommandsRouter.ts +++ b/packages/commandkit/src/app/router/CommandsRouter.ts @@ -830,6 +830,14 @@ export class CommandsRouter { node.executable = !!node.definitionPath && node.kind !== 'group' && !hasChildren; + if (hasChildren && node.definitionPath && node.kind !== 'group') { + this.addDiagnostic( + 'NON_LEAF_NODE_MAY_HAVE_HANDLERS', + `Hierarchical node "${node.route.join('.')}" has child nodes. Ensure its command.ts file does not export chatInput, message, or autocomplete handlers. Executable handlers must be defined on leaf nodes.`, + node.definitionPath, + ); + } + if (node.kind === 'subcommand' && hasChildren) { this.addDiagnostic( 'SUBCOMMAND_CANNOT_HAVE_CHILDREN',