fix(commands): route parent names in process and fire the full hook name

A parent name handed to runCommand resolved to the synthesized
hierarchical dispatcher, whose execute re-enters through the command
line's entry point: it exits the process on failure, tracks analytics
and consent, and fires hooks twice. The two shipped call sites,
"autocomplete" from post-install and "device" from the device log
stream, went that way. The dispatcher now routes a parent name to its
subcommand with the same rules the command line uses, and dispatches
that subcommand in process with the remaining arguments.

An in-process dispatch of a subcommand also only fired the truncated
hook name (before-open for open|ios), while the command line fires the
full name around it. runCommand now fires before-/after-<full name>
around the subcommand's own pair, in the command line's order, so a
project's before-open-ios hook runs however the command was reached.

Author: edusperoni

defining-commands.md

@@ -866,8 +866,9 @@ running afterwards:
   the caller decides what happens next.
 - **Analytics do not fire.** An in-process dispatch is not a new invocation of
   the CLI, and the consent check can prompt on a terminal the caller has put
-  into raw mode. Hooks do fire: a project's `before-open-ios` hook is part of
-  what `open|ios` means, however the command was reached.
+  into raw mode. Hooks do fire, under the same names the command line fires:
+  `open|ios` fires `before-open-ios` and then `before-open` (and `after-open`,
+  `after-open-ios` on the way out), however the command was reached.
 
 The options service is put back the way it was found. Merging a command's
 declarations into it rewrites the values the host process is still running on
@@ -882,7 +883,10 @@ that holds an injector can call `CommandsService.runCommand` directly.
 ### Asking another command
 
 Both methods take a registered name, or — the typed way — a definition or
-`Command()` class. A name is looked up in the registry; a definition runs as
+`Command()` class. A name is looked up in the registry, and a parent name is
+routed to its subcommand the way the command line routes it —
+`runCommand("device")` runs `device|*list`, `runCommand("device", ["log"])`
+runs `device|log`; a definition runs as
 given, whether or not it is registered, so `runCommand(prepareCommandDefinition)`
 runs exactly what you hold and cannot go stale the way a string can. Its first
 name still identifies it for hooks and reporting.
@@ -907,17 +911,17 @@ This is how one command builds on another's precondition. `embed` prepares the
 project, so "could `embed` run" starts with "could `prepare` run" — and the way
 to ask that is to ask `prepare`, not to import its `canExecute` and hand it
 services. The named command is resolved and its options primed exactly as
-`runCommand` does, then its own `canExecute` returns the verdict. It builds its
-own setup from its own services; nothing crosses between the two commands but
-the name and the arguments.
+`runCommand` does, then its own `canExecute` returns its verdict or throws. It
+builds its own setup from its own services; nothing crosses between the two
+commands but the name and the arguments.
 
 Pass only the arguments the child's own `arguments` policy accepts. The child
 enforces that policy before its `canExecute`, so forwarding a caller's whole
 argument list to a child that declares fewer is a rejection, not a wider check.
 
-`canExecuteCommand` is a thin call onto
-`CommandsService.canExecuteCommandInProcess`, and follows `runCommand` in
-everything else: the same injector rule, the same option priming and
+`canExecuteCommand` is a thin call onto `CommandsService.canExecuteCommand`
+(which the deprecated `canExecuteCommandInProcess` also calls), and follows
+`runCommand` in everything else: the same injector rule, the same option priming and
 restoration.
 
 ### Key shortcuts

lib/common/contracts/commands-service.ts

@@ -24,7 +24,9 @@ export abstract class CommandsService {
 	 *
 	 * `command` is a registered name, looked up in the registry, or a
 	 * definition or `Command()` class, which runs as given whether or not it is
-	 * registered — the typed way to refer to a command.
+	 * registered — the typed way to refer to a command. A parent name is routed
+	 * to its subcommand as the command line routes it, and a subcommand fires
+	 * its full hook name (`before-open-ios`) as well as its parent's.
 	 */
 	abstract runCommand(
 		command: CommandReference,
@@ -34,7 +36,8 @@ export abstract class CommandsService {
 	/**
 	 * Asks a registered command whether it could run on `args`, without running
 	 * it. The command is resolved and its options primed exactly as for
-	 * `runCommand`, and its own `canExecute` returns the verdict. The child
+	 * `runCommand`, and its own `canExecute` returns its verdict or throws — an
+	 * arguments-policy violation, a setup failure or `ctx.fail`. The child
 	 * builds its own setup from its own services, so nothing crosses between
 	 * the two but the name and the arguments; pass only the arguments the
 	 * child's own `arguments` policy accepts.

lib/common/definitions/commands-service.d.ts

@@ -2,7 +2,7 @@ interface ICommandsService {
 	currentCommandData: ICommandData;
 	/**
 	 * Whether the command running right now was dispatched by
-	 * executeCommandInProcess rather than by the command line — what tells a
+	 * runCommand rather than by the command line — what tells a
 	 * command that it is borrowing a host process instead of owning one.
 	 */
 	readonly isExecutingInProcess: boolean;

lib/common/services/commands-service.ts

@@ -1,6 +1,9 @@
 const jaroWinklerDistance = require("../vendor/jaro-winkler_distance");
 import * as helpers from "../helpers";
-import { CommandsDelimiters } from "../constants";
+import {
+	CommandsDelimiters,
+	ERROR_NO_VALID_SUBCOMMAND_FORMAT,
+} from "../constants";
 import { EOL } from "os";
 import * as _ from "lodash";
 import { IOptions, IOptionsTracker } from "../../declarations";
@@ -121,9 +124,7 @@ export class CommandsService
 			await this.$optionsTracker.trackOptions(this.$options);
 		}
 
-		const shouldExecuteHooks =
-			!this.$staticConfig.disableCommandHooks &&
-			(command.enableHooks === undefined || command.enableHooks === true);
+		const shouldExecuteHooks = this.shouldExecuteHooks(command);
 		let hookCommandName = commandName;
 		if (shouldExecuteHooks) {
 			// Handle correctly hierarchical commands
@@ -132,15 +133,8 @@ export class CommandsService
 				commandArguments,
 			);
 			if (hierarchicalCommandName) {
-				hookCommandName = helpers.stringReplaceAll(
+				hookCommandName = this.toHookCommandName(
 					hierarchicalCommandName.commandName,
-					CommandsDelimiters.DefaultHierarchicalCommand,
-					CommandsDelimiters.HooksCommand,
-				);
-				hookCommandName = helpers.stringReplaceAll(
-					hookCommandName,
-					CommandsDelimiters.HierarchicalCommand,
-					CommandsDelimiters.HooksCommand,
 				);
 			}
 
@@ -157,6 +151,57 @@ export class CommandsService
 		}
 	}
 
+	private toHookCommandName(commandName: string): string {
+		const hookCommandName = helpers.stringReplaceAll(
+			commandName,
+			CommandsDelimiters.DefaultHierarchicalCommand,
+			CommandsDelimiters.HooksCommand,
+		);
+		return helpers.stringReplaceAll(
+			hookCommandName,
+			CommandsDelimiters.HierarchicalCommand,
+			CommandsDelimiters.HooksCommand,
+		);
+	}
+
+	/**
+	 * The command line reaches a subcommand through its parent's dispatcher,
+	 * which fires the subcommand's full hook name (`before-open-ios`) around
+	 * the subcommand's own dispatch, whose name the hooks service truncates at
+	 * the `|` (`before-open`). An in-process dispatch goes straight to the
+	 * subcommand, so it fires the outer pair itself.
+	 */
+	private async runResolvedCommandInProcess(
+		command: ICommand,
+		commandName: string,
+		commandArguments: string[],
+	): Promise<void> {
+		const subcommandHookName =
+			commandName.includes(CommandsDelimiters.HierarchicalCommand) &&
+			this.shouldExecuteHooks(command)
+				? this.toHookCommandName(commandName)
+				: undefined;
+
+		if (subcommandHookName) {
+			await this.$hooksService.executeBeforeHooks(subcommandHookName);
+		}
+
+		await this.runResolvedCommand(command, commandName, commandArguments, {
+			trackAnalytics: false,
+		});
+
+		if (subcommandHookName) {
+			await this.$hooksService.executeAfterHooks(subcommandHookName);
+		}
+	}
+
+	private shouldExecuteHooks(command: ICommand): boolean {
+		return (
+			!this.$staticConfig.disableCommandHooks &&
+			(command.enableHooks === undefined || command.enableHooks === true)
+		);
+	}
+
 	private printHelpSuggestion(commandName?: string): Promise<void> {
 		const command = commandName
 			? helpers.stringReplaceAll(
@@ -256,9 +301,10 @@ export class CommandsService
 		let commandName = typeof reference === "string" ? reference : undefined;
 		this.inProcessDepth++;
 		try {
-			const resolved = this.resolveReference(reference);
+			const resolved = this.resolveReference(reference, commandArguments);
 			const command = resolved.command;
 			commandName = resolved.commandName;
+			commandArguments = resolved.commandArguments;
 
 			this.commands.push({ commandName, commandArguments });
 			const restoreOptions = this.primeOptions(command);
@@ -280,9 +326,11 @@ export class CommandsService
 					);
 				}
 
-				await this.runResolvedCommand(command, commandName, commandArguments, {
-					trackAnalytics: false,
-				});
+				await this.runResolvedCommandInProcess(
+					command,
+					commandName,
+					commandArguments,
+				);
 			} finally {
 				restoreOptions();
 				this.commands.pop();
@@ -301,7 +349,7 @@ export class CommandsService
 	/**
 	 * The `canExecute` half of {@link runCommand}: the named command is resolved
 	 * and its options are primed the same way, and its own `canExecute` returns
-	 * the verdict. The child builds its own setup from its own services —
+	 * its verdict or throws. The child builds its own setup from its own services —
 	 * nothing is threaded in from the caller — which is what lets one command
 	 * reuse another's precondition without importing its handlers.
 	 */
@@ -311,7 +359,9 @@ export class CommandsService
 	): Promise<boolean> {
 		this.inProcessDepth++;
 		try {
-			const { commandName, command } = this.resolveReference(reference);
+			const resolved = this.resolveReference(reference, commandArguments);
+			const { commandName, command } = resolved;
+			commandArguments = resolved.commandArguments;
 
 			this.commands.push({ commandName, commandArguments });
 			const restoreOptions = this.primeOptions(command);
@@ -347,20 +397,22 @@ export class CommandsService
 		return this.canExecuteCommand(commandName, commandArguments);
 	}
 
-	/**
-	 * Merging a command's options into the parser rewrites the values the host
-	 * process is still running on: a declared default replaces the CLI-wide one
-	 * and the host keeps reading the replacement long after the command is
-	 * done. An in-process dispatch has to put the parser back where it found it.
-	 */
 	/**
 	 * A name is looked up in the registry; a definition or class is run as the
 	 * caller holds it, registered or not, so what runs is what was referenced.
 	 * Its first name still identifies it for hooks and reporting.
+	 *
+	 * A parent name is routed to its subcommand here rather than run: the
+	 * parent's synthesized dispatcher re-enters through `tryExecuteCommand`,
+	 * which exits the process on failure and tracks analytics.
 	 */
-	private resolveReference(reference: CommandReference): {
+	private resolveReference(
+		reference: CommandReference,
+		commandArguments: string[],
+	): {
 		commandName: string;
 		command: ICommand;
+		commandArguments: string[];
 	} {
 		if (typeof reference === "string") {
 			const command = this.$injector.resolveCommand(reference);
@@ -370,7 +422,28 @@ export class CommandsService
 				);
 			}
 
-			return { commandName: reference, command };
+			if (command.isHierarchicalCommand) {
+				const subcommand = this.$injector.buildHierarchicalCommand(
+					reference,
+					commandArguments,
+				);
+				const subcommandInstance =
+					subcommand && this.$injector.resolveCommand(subcommand.commandName);
+				if (!subcommandInstance) {
+					this.$errors.failWithHelp(
+						ERROR_NO_VALID_SUBCOMMAND_FORMAT,
+						reference,
+					);
+				}
+
+				return {
+					commandName: subcommand.commandName,
+					command: subcommandInstance,
+					commandArguments: subcommand.remainingArguments,
+				};
+			}
+
+			return { commandName: reference, command, commandArguments };
 		}
 
 		const definition = toCommandDefinition(reference);
@@ -386,9 +459,16 @@ export class CommandsService
 				? definition.name[0]
 				: definition.name,
 			command: createCommandFromDefinition(definition, <any>this.$injector),
+			commandArguments,
 		};
 	}
 
+	/**
+	 * Merging a command's options into the parser rewrites the values the host
+	 * process is still running on: a declared default replaces the CLI-wide one
+	 * and the host keeps reading the replacement long after the command is
+	 * done. An in-process dispatch has to put the parser back where it found it.
+	 */
 	private primeOptions(command: ICommand): () => void {
 		if (command.isHierarchicalCommand) {
 			return () => undefined;

test/commands-service.ts

@@ -336,7 +336,12 @@ describe("commands-service", () => {
 
 			await service.executeCommandInProcess("open|ios");
 
-			assert.deepEqual(record.hooks, ["before:open|ios", "after:open|ios"]);
+			assert.deepEqual(record.hooks, [
+				"before:open-ios",
+				"before:open|ios",
+				"after:open|ios",
+				"after:open-ios",
+			]);
 			assert.deepEqual(record.analytics, []);
 		});
 	});