fix: a malformed macro command list no longer makes the macro unrepairable

src/engine/MacroChoiceEngine.malformed.test.ts

@@ -0,0 +1,173 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+vi.mock("../formatters/completeFormatter", () => ({
+	CompleteFormatter: class CompleteFormatterMock {},
+}));
+vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() }));
+vi.mock("../main", () => ({ default: class QuickAddMock {} }));
+
+import type { App } from "obsidian";
+import type IMacroChoice from "../types/choices/IMacroChoice";
+import type { IChoiceExecutor } from "../IChoiceExecutor";
+import { CommandType } from "../types/macros/CommandType";
+import { MacroChoiceEngine } from "./MacroChoiceEngine";
+import { log } from "../logger/logManager";
+import { settingsStore } from "../settingsStore";
+
+function runMacro(macro: unknown) {
+	const choice = {
+		id: "choice-id",
+		name: "Test choice",
+		type: "Macro",
+		command: false,
+		runOnStartup: false,
+		macro,
+	} as unknown as IMacroChoice;
+	const choiceExecutor: IChoiceExecutor = {
+		execute: vi.fn(),
+		variables: new Map<string, unknown>(),
+	};
+	return new MacroChoiceEngine(
+		{} as App,
+		{ settings: settingsStore.getState() } as never,
+		choice,
+		choiceExecutor,
+		new Map<string, unknown>(),
+	).run();
+}
+
+const wait = (id: string, time = 1) => ({
+	id,
+	name: "Wait",
+	type: CommandType.Wait,
+	time,
+});
+
+/**
+ * The run path, over the same malformed shapes the macro EDITOR handles
+ * (#1593). Two of them were broken outside the UI entirely, because the old
+ * guard was a truthiness test.
+ */
+describe("MacroChoiceEngine.run over a malformed command list (#1593)", () => {
+	let errors: string[];
+
+	beforeEach(() => {
+		errors = [];
+		vi.spyOn(log, "logError").mockImplementation((msg: string) => {
+			errors.push(msg);
+		});
+	});
+
+	it.each([
+		["an array-turned-object", { "0": wait("hidden") }],
+		["a string", "not a list"],
+		["a number", 7],
+	])("FAILS, with an actionable message, for %s", async (_l, commands) => {
+		// Before: `{"0":...}` reached `for..of` and surfaced a bare
+		// "i is not iterable"; "not a list" reached it INTACT (strings are
+		// iterable) so the macro reported success having run nothing at all.
+		//
+		// It throws rather than logging and returning, so `quickadd:run` reports
+		// ok:false and automation cannot carry on as if the macro had run.
+		await expect(
+			runMacro({ id: "m", name: "M", commands }),
+		).rejects.toThrow(/Could not read the commands for macro 'Test choice'/);
+	});
+
+	it("names data.json, so the message is actionable", async () => {
+		await expect(
+			runMacro({ id: "m", name: "M", commands: "not a list" }),
+		).rejects.toThrow(/data\.json/);
+	});
+
+	// `commands: []` is what QuickAddMacro's constructor produces, so every
+	// freshly created macro - and every launch with an unpopulated run-on-startup
+	// macro - would otherwise raise a 15-second red notice.
+	it("stays silent for an empty list, which is the healthy default", async () => {
+		await runMacro({ id: "m", name: "M", commands: [] });
+		expect(errors).toEqual([]);
+	});
+
+	it.each([
+		["commands is null", { id: "m", name: "M", commands: null }],
+		["commands is absent", { id: "m", name: "M" }],
+		["the macro is missing", null],
+	])("reports a missing macro when %s", async (_label, macro) => {
+		await runMacro(macro);
+		expect(errors).toEqual([
+			"No commands in the macro for choice 'Test choice'",
+		]);
+	});
+
+	it("runs the real commands either side of a hole", async () => {
+		await expect(
+			runMacro({ id: "m", name: "M", commands: [wait("a"), null, wait("b")] }),
+		).resolves.toBeUndefined();
+		expect(errors).toEqual([]);
+	});
+});
+
+describe("MacroChoiceEngine conditional branches over a malformed value (#1593)", () => {
+	let errors: string[];
+
+	beforeEach(() => {
+		errors = [];
+		vi.spyOn(log, "logError").mockImplementation((msg: string) => {
+			errors.push(msg);
+		});
+		vi.spyOn(log, "logWarning").mockImplementation(() => {});
+	});
+
+	const conditional = (thenCommands: unknown, elseCommands: unknown) => ({
+		id: "cond",
+		name: "If",
+		type: CommandType.Conditional,
+		// An undefined variable evaluates false, so the ELSE branch is taken.
+		condition: {
+			mode: "variable",
+			variableName: "undefinedVar",
+			operator: "isTruthy",
+			valueType: "string",
+		},
+		thenCommands,
+		elseCommands,
+	});
+
+	it("fails instead of skipping an unreadable branch silently", async () => {
+		await expect(
+			runMacro({
+				id: "m",
+				name: "M",
+				commands: [conditional([], { "0": wait("hidden") })],
+			}),
+		).rejects.toThrow(/Could not read the else commands/);
+	});
+
+	// Returning would only exit executeConditional: the outer loop would run
+	// every command AFTER the conditional, which were only ever meant to follow
+	// a branch that never ran.
+	it("does not run the commands after an unreadable conditional", async () => {
+		const after = { id: "after", name: "Wait", type: CommandType.Wait, time: 1 };
+		await expect(
+			runMacro({
+				id: "m",
+				name: "M",
+				commands: [conditional([], "not a list"), after],
+			}),
+		).rejects.toThrow(/Could not read the else commands/);
+	});
+
+	// A conditional with no else branch is entirely normal, and must stay quiet.
+	it.each([
+		["an empty array", []],
+		["undefined", undefined],
+		["null", null],
+	])("stays silent for a branch that is %s", async (_label, elseCommands) => {
+		await runMacro({
+			id: "m",
+			name: "M",
+			commands: [conditional([], elseCommands)],
+		});
+		expect(errors).toEqual([]);
+	});
+});

src/engine/MacroChoiceEngine.ts

@@ -59,6 +59,11 @@ import { evaluateCondition } from "./helpers/conditionalEvaluator";
 import { handleMacroAbort } from "../utils/macroAbortHandler";
 import { buildOpenFileOptions } from "./helpers/openFileOptions";
 import { createVariablesProxy } from "../utils/variablesProxy";
+import {
+	commandListOf,
+	hasCommandList,
+	isUnreadableCommandList,
+} from "../utils/macroUtils";
 
 type ConditionalScriptRunner = () => Promise<unknown>;
 type UserScriptFunction = (
@@ -263,14 +268,39 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 	}
 
 	async run(): Promise<void> {
-		if (!this.macro || !this.macro.commands) {
-			log.logError(
-				`No commands in the macro for choice '${this.choice.name}'`
+		// `!this.macro.commands` is truthiness, so it used to wave through the two
+		// shapes that then failed outside the UI (#1593): an array-turned-object
+		// reached `for..of` and threw a bare "i is not iterable" at the user, and a
+		// string reached it INTACT (strings are iterable), so the macro reported
+		// success having walked its own characters and done nothing at all.
+		//
+		// THROWS rather than returning: none of the macro ran, so this is a
+		// failure and every caller has to see it as one. Returning quietly would
+		// let `quickadd:run` answer `ok: true` and automation carry on as if the
+		// macro had done its job (#1606's contract). The throw surfaces as one
+		// Notice through the executor's error path.
+		if (isUnreadableCommandList(this.macro?.commands)) {
+			throw new Error(
+				`Could not read the commands for macro '${this.choice.name}'. The saved value is not a list of commands - QuickAdd has not changed it. It is in .obsidian/plugins/quickadd/data.json.`
 			);
+		}
+
+		const commands = commandListOf(this.macro?.commands);
+		if (commands.length === 0) {
+			// `commands: []` is the HEALTHY default (QuickAddMacro's constructor),
+			// so an empty list stays as quiet as it was before this guard existed -
+			// otherwise every freshly created macro, and every launch with an
+			// unpopulated run-on-startup macro, would raise a 15s error notice.
+			// Only a MISSING macro is worth saying anything about.
+			if (!hasCommandList(this.macro?.commands)) {
+				log.logError(
+					`No commands in the macro for choice '${this.choice.name}'`
+				);
+			}
 			return;
 		}
 
-		await this.executeCommands(this.macro.commands);
+		await this.executeCommands(commands);
 	}
 
 	public getOutput(): unknown {
@@ -755,6 +785,21 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 			? command.thenCommands
 			: command.elseCommands;
 
+		// An absent branch is normal (a conditional with no else), so it stays
+		// silent. A branch we could not read is not: say so rather than skipping
+		// it as if the user had left it empty (#1593).
+		//
+		// THROWS rather than returning, for the same reason run() does, and one
+		// more: returning here only exits executeConditional, so the outer loop
+		// would carry on with every command AFTER the conditional - running
+		// file-writing and script commands that were only ever meant to follow a
+		// branch that never ran.
+		if (isUnreadableCommandList(branch)) {
+			throw new Error(
+				`Could not read the ${shouldRunThenBranch ? "then" : "else"} commands for '${command.name}'. The saved value is not a list of commands - QuickAdd has not changed it. It is in .obsidian/plugins/quickadd/data.json.`
+			);
+		}
+
 		if (!Array.isArray(branch) || branch.length === 0) {
 			return;
 		}

src/engine/SingleMacroEngine.member-access.test.ts

@@ -143,6 +143,89 @@ describe("SingleMacroEngine member access", () => {
 		});
 	});
 
+	// #1593: `commandListOf` converts the NON-ARRAY shapes, but a `null` hole
+	// INSIDE a real array sails straight through .map/.filter. This is the one
+	// entrypoint that was asymmetric: the same macro run WITHOUT `::member` was
+	// fine, because MacroChoiceEngine.executeCommands is record-guarded.
+	describe("over a malformed command list (#1593)", () => {
+		it("resolves the member through a list with a hole in it", async () => {
+			const userScript = createUserScript("user-script", "script.js");
+			const macroChoice = baseMacroChoice([
+				null as unknown as ICommand,
+				userScript,
+			]);
+
+			const engineInstance = macroEngineFactory();
+			macroEngineFactory = () => engineInstance;
+			mockGetUserScript.mockResolvedValue({ f: vi.fn().mockReturnValue("ok") });
+
+			const engine = new SingleMacroEngine(
+				app,
+				plugin,
+				[macroChoice],
+				choiceExecutor,
+			);
+
+			await expect(engine.runAndGetOutput("My Macro::f")).resolves.toBe("ok");
+		});
+
+		it.each([
+			["a string", "not a list"],
+			["an array-turned-object", { "0": {} }],
+			["null", null],
+		])("falls back to running the whole macro when commands is %s", async (
+			_label,
+			commands,
+		) => {
+			const macroChoice = baseMacroChoice(commands as unknown as ICommand[]);
+
+			const engineInstance = macroEngineFactory();
+			engineInstance.run = vi.fn().mockResolvedValue(undefined);
+			engineInstance.getOutput = vi.fn().mockReturnValue(undefined);
+			macroEngineFactory = () => engineInstance;
+			vi.spyOn(log, "logWarning").mockImplementation(() => {});
+
+			const engine = new SingleMacroEngine(
+				app,
+				plugin,
+				[macroChoice],
+				choiceExecutor,
+			);
+
+			// No throw: it cannot find a user-script command, so it runs the macro.
+			await expect(engine.runAndGetOutput("My Macro::f")).resolves.toBe("");
+			expect(engineInstance.run).toHaveBeenCalledTimes(1);
+		});
+
+		it("re-resolves past a hole a pre-command introduced", async () => {
+			const pre = { id: "pre", name: "Wait", type: CommandType.Wait } as ICommand;
+			const userScript = createUserScript("user-script", "script.js");
+			const macroChoice = baseMacroChoice([pre, userScript]);
+
+			const engineInstance = macroEngineFactory();
+			engineInstance.runSubset = vi.fn().mockImplementation(() => {
+				// A pre-command rewrote the list and left a hole in it.
+				macroChoice.macro.commands = [
+					pre,
+					null as unknown as ICommand,
+					userScript,
+				];
+				return Promise.resolve(undefined);
+			});
+			macroEngineFactory = () => engineInstance;
+			mockGetUserScript.mockResolvedValue({ f: vi.fn().mockReturnValue("ok") });
+
+			const engine = new SingleMacroEngine(
+				app,
+				plugin,
+				[macroChoice],
+				choiceExecutor,
+			);
+
+			await expect(engine.runAndGetOutput("My Macro::f")).resolves.toBe("ok");
+		});
+	});
+
 	it("runs the macro when no member access is requested", async () => {
 		const userScript = createUserScript("user-script", "script.js");
 		const macroChoice = baseMacroChoice([userScript]);