From 45eda89dc7a054f05f2e9edf652f825f8df09342 Mon Sep 17 00:00:00 2001
From: Christian Bager Bach Houmann <christian@bagerbach.com>
Date: Mon, 27 Jul 2026 17:21:15 +0200
Subject: [PATCH 1/8] refactor(macros): give a macro's command list total read
 accessors

`IMacro.commands` is declared `ICommand[]` and nothing enforces it: data.json
is hand-edited, imported, half-written and sync-merged. Adds the same accessor
split `Multi.choices` got in #1583 - `commandListOf` (total READ view),
`hasCommandList` (WRITE guard), `isUnreadableCommandList` (the line between
degrading quietly and telling the user) - plus `normalizeCommandList`, the
editor-seam repair that keeps a duplicate-id or id-less command under a fresh
uuid instead of hiding it.

Also makes `regenerateIds` total: it is a WRITE path reached from
duplicateChoice, so a non-array `commands` is now left exactly as found and a
hole in the list is stepped over rather than dereferenced.

Refs #1593
---
 src/utils/macroUtils.test.ts | 212 +++++++++++++++++++++++++++++++++++
 src/utils/macroUtils.ts      | 154 ++++++++++++++++++++++++-
 2 files changed, 364 insertions(+), 2 deletions(-)
 create mode 100644 src/utils/macroUtils.test.ts

diff --git a/src/utils/macroUtils.test.ts b/src/utils/macroUtils.test.ts
new file mode 100644
index 000000000..a41bef40b
--- /dev/null
+++ b/src/utils/macroUtils.test.ts
@@ -0,0 +1,212 @@
+import { describe, expect, it } from "vitest";
+
+import type { ICommand } from "../types/macros/ICommand";
+import type { IMacro } from "../types/macros/IMacro";
+import {
+	commandListOf,
+	hasCommandList,
+	isCommandLike,
+	isUnreadableCommandList,
+	normalizeCommandList,
+	regenerateIds,
+} from "./macroUtils";
+
+const wait = (id: string, time = 100): ICommand =>
+	({ id, name: "Wait", type: "Wait", time }) as unknown as ICommand;
+
+/**
+ * The `commands` values `data.json` actually shows up with, and whether each one
+ * could still be CARRYING commands we cannot read. Shared by every case below so
+ * a new shape is added in one place. Mirrors MALFORMED_CHILDREN_SHAPES.
+ */
+const MALFORMED_COMMAND_SHAPES: { key: string; value: unknown; carrying: boolean }[] = [
+	{ key: "missing", value: undefined, carrying: false },
+	{ key: "null", value: null, carrying: false },
+	{ key: "emptyObject", value: {}, carrying: false },
+	{ key: "emptyString", value: "", carrying: false },
+	{ key: "zero", value: 0, carrying: false },
+	{ key: "false", value: false, carrying: false },
+	{ key: "arrayLikeObject", value: { "0": wait("hidden") }, carrying: true },
+	{ key: "string", value: "not a list", carrying: true },
+	{ key: "jsonString", value: '[{"id":"x","type":"Wait"}]', carrying: true },
+	{ key: "number", value: 7, carrying: true },
+];
+
+describe("commandListOf", () => {
+	it("hands back the live array when there is one", () => {
+		const commands = [wait("a")];
+		expect(commandListOf(commands)).toBe(commands);
+	});
+
+	it.each(MALFORMED_COMMAND_SHAPES)("reads $key as no commands", ({ value }) => {
+		expect(commandListOf(value)).toEqual([]);
+	});
+
+	it("never hands back the same [] twice, so no caller can leak a shared array", () => {
+		expect(commandListOf(null)).not.toBe(commandListOf(null));
+	});
+});
+
+describe("hasCommandList", () => {
+	it("is true only for a real array", () => {
+		expect(hasCommandList([])).toBe(true);
+		expect(hasCommandList([wait("a")])).toBe(true);
+	});
+
+	it.each(MALFORMED_COMMAND_SHAPES)("refuses to let a write rebuild $key", ({ value }) => {
+		expect(hasCommandList(value)).toBe(false);
+	});
+});
+
+describe("isUnreadableCommandList", () => {
+	it("is false for a readable array", () => {
+		expect(isUnreadableCommandList([])).toBe(false);
+		expect(isUnreadableCommandList([wait("a")])).toBe(false);
+	});
+
+	it.each(MALFORMED_COMMAND_SHAPES)(
+		"says whether $key could be carrying commands",
+		({ value, carrying }) => {
+			expect(isUnreadableCommandList(value)).toBe(carrying);
+		},
+	);
+});
+
+describe("isCommandLike", () => {
+	it("accepts objects and rejects the holes a truncated write leaves", () => {
+		expect(isCommandLike(wait("a"))).toBe(true);
+		expect(isCommandLike(null)).toBe(false);
+		expect(isCommandLike(undefined)).toBe(false);
+		expect(isCommandLike("stray")).toBe(false);
+		expect(isCommandLike(3)).toBe(false);
+	});
+});
+
+describe("normalizeCommandList", () => {
+	it("is identity for a healthy list - same array, nothing changed", () => {
+		const commands = [wait("a"), wait("b")];
+		const result = normalizeCommandList(commands);
+		expect(result.changed).toBe(false);
+		expect(result.commands).toBe(commands);
+	});
+
+	it("keeps a duplicate-id command under a fresh id instead of dropping it", () => {
+		const first = wait("dup", 100);
+		const second = wait("dup", 200);
+		const { commands, changed } = normalizeCommandList([first, second]);
+
+		expect(changed).toBe(true);
+		expect(commands).toHaveLength(2);
+		// The first occurrence keeps its identity object and id.
+		expect(commands[0]).toBe(first);
+		// The second survives whole - only its id moved.
+		expect(commands[1]).toMatchObject({ name: "Wait", type: "Wait", time: 200 });
+		expect(commands[1].id).not.toBe("dup");
+		expect(commands[1].id).not.toBe(commands[0].id);
+	});
+
+	it("mints an id for a command that has none, rather than hiding it", () => {
+		const idless = { name: "Readwise sync", type: "UserScript", path: "s.js" };
+		const { commands } = normalizeCommandList([idless]);
+
+		expect(commands).toHaveLength(1);
+		expect(commands[0]).toMatchObject({ name: "Readwise sync", path: "s.js" });
+		expect(typeof commands[0].id).toBe("string");
+		expect(commands[0].id).not.toBe("");
+	});
+
+	it.each([
+		["empty string", ""],
+		["number", 3],
+		["null", null],
+	])("mints an id for a command whose id is a %s", (_label, id) => {
+		const { commands } = normalizeCommandList([{ name: "X", type: "Wait", id }]);
+		expect(commands).toHaveLength(1);
+		expect(typeof commands[0].id).toBe("string");
+		expect(commands[0].id).not.toBe("");
+	});
+
+	it("gives two id-less commands DIFFERENT ids (the each_key_duplicate case)", () => {
+		const { commands } = normalizeCommandList([
+			{ name: "A", type: "Wait" },
+			{ name: "B", type: "Wait" },
+		]);
+		expect(commands[0].id).not.toBe(commands[1].id);
+	});
+
+	it("drops holes, which carry nothing, and keeps everything around them", () => {
+		const a = wait("a");
+		const b = wait("b");
+		const { commands, changed } = normalizeCommandList([a, null, "stray", b, 7]);
+
+		expect(changed).toBe(true);
+		expect(commands).toEqual([a, b]);
+	});
+
+	it("never mutates its input", () => {
+		const input = [wait("dup"), wait("dup"), null];
+		const before = JSON.stringify(input);
+		normalizeCommandList(input);
+		expect(JSON.stringify(input)).toBe(before);
+	});
+
+	it.each(MALFORMED_COMMAND_SHAPES)("reads $key as an empty list", ({ value }) => {
+		expect(normalizeCommandList(value).commands).toEqual([]);
+	});
+
+	it("is idempotent", () => {
+		const first = normalizeCommandList([wait("dup"), wait("dup"), null]);
+		const second = normalizeCommandList(first.commands);
+		expect(second.changed).toBe(false);
+		expect(second.commands).toBe(first.commands);
+	});
+});
+
+describe("regenerateIds", () => {
+	it("re-ids the macro and every command", () => {
+		const macro: IMacro = {
+			id: "m",
+			name: "M",
+			commands: [wait("a"), wait("b")],
+		};
+		regenerateIds(macro);
+
+		expect(macro.id).not.toBe("m");
+		expect(macro.commands[0].id).not.toBe("a");
+		expect(macro.commands[1].id).not.toBe("b");
+	});
+
+	it.each(MALFORMED_COMMAND_SHAPES)(
+		"leaves a $key commands value exactly as it found it",
+		({ key, value }) => {
+			const macro = { id: "m", name: "M" } as Record<string, unknown>;
+			if (value !== undefined) macro.commands = value;
+			const before = JSON.stringify(macro.commands ?? null);
+
+			expect(() => regenerateIds(macro as unknown as IMacro)).not.toThrow();
+
+			expect(JSON.stringify(macro.commands ?? null)).toBe(before);
+			expect("commands" in macro).toBe(key !== "missing");
+			// The macro id is still refreshed: only the unreadable value is preserved.
+			expect(macro.id).not.toBe("m");
+		},
+	);
+
+	it("steps over a hole in the list instead of dereferencing it", () => {
+		const macro = {
+			id: "m",
+			name: "M",
+			commands: [wait("a"), null, wait("b")],
+		} as unknown as IMacro;
+
+		expect(() => regenerateIds(macro)).not.toThrow();
+		expect(macro.commands[0].id).not.toBe("a");
+		expect(macro.commands[1]).toBeNull();
+		expect(macro.commands[2].id).not.toBe("b");
+	});
+
+	it("tolerates a macro that is not an object at all", () => {
+		expect(() => regenerateIds(null as unknown as IMacro)).not.toThrow();
+		expect(() => regenerateIds("x" as unknown as IMacro)).not.toThrow();
+	});
+});
diff --git a/src/utils/macroUtils.ts b/src/utils/macroUtils.ts
index d35b54ea5..ee3f5ddac 100644
--- a/src/utils/macroUtils.ts
+++ b/src/utils/macroUtils.ts
@@ -1,12 +1,162 @@
 import { v4 as uuidv4 } from "uuid";
 import type { IMacro } from "../types/macros/IMacro";
+import type { ICommand } from "../types/macros/ICommand";
 
 /**
- * Regenerates all IDs in a macro to prevent collisions after duplication
+ * Whether `value` is shaped enough like a command to be walked: a non-null
+ * object. `data.json` is untrusted, so a command list can hold a `null` (a
+ * truncated write), a string, or a number.
+ *
+ * The sibling of `isChoiceLike` in choiceUtils.ts, and the same argument applies.
+ */
+export function isCommandLike(value: unknown): value is ICommand {
+	return typeof value === "object" && value !== null;
+}
+
+/**
+ * A command list that is always safe to iterate, map or spread.
+ *
+ * `IMacro.commands` is declared `ICommand[]` and nothing enforces it. This is
+ * the same untrusted-input problem `childChoicesOf` solves one type over, with
+ * the same two guards that look right and are not:
+ *
+ *   `macro.commands ?? []`    passes `{}` straight through (not nullish)
+ *   `if (macro.commands)`     passes `{}` AND `"not a list"` through (truthy)
+ *
+ * Both shapes are real: `{"0": {...}, "1": {...}}` is the classic
+ * array-turned-object JSON artefact, and a string reaches `for..of` intact
+ * because strings are iterable - which is how a malformed macro used to "run"
+ * successfully while doing nothing at all (#1593).
+ *
+ * Takes `unknown` rather than `IMacro` on purpose (mirroring `rootChoicesOf`):
+ * `choice.macro` is untrusted too, so the callers that need this most cannot
+ * produce an `IMacro` to pass. Read `commandListOf(choice.macro?.commands)`.
+ *
+ * This is a READ view, never a repair: nothing here persists the `[]` it hands
+ * back for a malformed value. WRITE paths must leave a malformed `commands`
+ * exactly as they found it - guard them with `hasCommandList` - so the original
+ * survives on disk to be recovered by hand.
+ */
+export function commandListOf(value: unknown): ICommand[] {
+	return Array.isArray(value) ? value : [];
+}
+
+/**
+ * Whether `value` is a real command array, i.e. whether a WRITE path may rebuild
+ * it. Guards the `{ ...macro, commands: ... }` rebuilds so a malformed list is
+ * passed through untouched instead of being silently rewritten to the `[]` that
+ * `commandListOf` reads it as.
+ */
+export function hasCommandList(value: unknown): boolean {
+	return Array.isArray(value);
+}
+
+/**
+ * Whether a command list value holds something we cannot read, as opposed to
+ * nothing at all. True only for the malformed shapes that can still CARRY
+ * commands: a non-empty object where an array belongs, or a non-empty primitive
+ * (a double-encoded `"[{\"id\":...}]"` is a routine scripted-edit artefact, and
+ * in general we cannot prove that an unreadable non-empty value is empty).
+ *
+ * `undefined`, `null`, `{}`, `""`, `0` and `false` carry nothing, so for those
+ * the macro really has no commands and an empty editor is the honest thing to
+ * show.
+ *
+ * This is the line between "degrade quietly" and "tell the user": a macro whose
+ * commands we cannot read must not claim to be empty, and must offer no
+ * affordance that would overwrite the value. The editor's card and its
+ * suppressed controls both read this one predicate so they cannot disagree.
+ */
+export function isUnreadableCommandList(value: unknown): boolean {
+	if (Array.isArray(value)) return false;
+	if (value === undefined || value === null) return false;
+	if (typeof value === "object") return Object.keys(value).length > 0;
+	// A non-empty primitive was never empty; an empty one carries nothing.
+	return Boolean(value);
+}
+
+export interface NormalizedCommandList {
+	commands: ICommand[];
+	/** False when `commands` is the input array itself, unchanged. */
+	changed: boolean;
+}
+
+/**
+ * The command list an EDITOR should work over: every entry an object with an id
+ * that is unique within the list.
+ *
+ * Two things go wrong in a list that is otherwise a perfectly good array, and
+ * both used to cost the user the whole macro editor (#1593). Svelte's keyed
+ * `{#each ... (command.id)}` throws `each_key_duplicate` on a repeated key and
+ * `Cannot read properties of null` on a hole, and svelte-dnd-action reads `.id`
+ * on every item it is handed. Either throw aborts the mount, which since #1584
+ * means an honest error card - and a macro that can only be repaired by hand.
+ *
+ * The two cases are NOT the same and are deliberately not treated the same:
+ *
+ *   - A duplicate or missing id is a REAL command that merely cannot be keyed.
+ *     It is given a fresh uuid and kept. Never dropped: dropping it would hide a
+ *     working command from the one screen that could delete it, and the editor
+ *     persists the list it rendered, so the next reorder would erase it from
+ *     disk with no prompt and no undo.
+ *   - A `null` or a stray primitive carries nothing: there is no command to
+ *     re-id, the engine already steps over it, and it is dropped.
+ *
+ * Re-iding is safe for stored user-script secrets: a secret lives in the
+ * command's own `settings` as a `{secretRef}` object and travels with it
+ * (userScriptSecrets.ts). Only the stable-id RE-ADOPTION path
+ * (`buildUserScriptSecretId`) keys on `command.id`, and it only runs for a
+ * setting that has no ref yet.
+ *
+ * Applied once at the editor seam (CommandSequenceEditor's constructor), never
+ * at load: `loadSettings` runs before the migrations and never saves, so a heal
+ * there would mint fresh uuids on every launch until an unrelated save landed.
+ * Here it is idempotent per session and is persisted by the user's first
+ * ordinary edit.
+ *
+ * Returns the input array itself when there was nothing to change, so a healthy
+ * macro is provably untouched.
+ */
+export function normalizeCommandList(value: unknown): NormalizedCommandList {
+	const input = commandListOf(value);
+	const seen = new Set<string>();
+	let changed = false;
+
+	const commands: ICommand[] = [];
+	for (const entry of input) {
+		if (!isCommandLike(entry)) {
+			changed = true;
+			continue;
+		}
+		const id = entry.id;
+		if (typeof id === "string" && id !== "" && !seen.has(id)) {
+			seen.add(id);
+			commands.push(entry);
+			continue;
+		}
+		const replacement = { ...entry, id: uuidv4() };
+		seen.add(replacement.id);
+		commands.push(replacement);
+		changed = true;
+	}
+
+	return changed ? { commands, changed } : { commands: input, changed: false };
+}
+
+/**
+ * Regenerates all IDs in a macro to prevent collisions after duplication.
+ *
+ * Total over a malformed macro: a missing or non-array `commands` is left
+ * exactly as it is (this is a WRITE path - see `hasCommandList`), and a hole in
+ * the list is stepped over rather than dereferenced. Reached from
+ * `duplicateChoice`, which runs over whatever the user's data.json holds.
  */
 export function regenerateIds(macro: IMacro): void {
+	if (!isCommandLike(macro)) return;
 	macro.id = uuidv4();
-	macro.commands.forEach(command => {
+	if (!hasCommandList(macro.commands)) return;
+	macro.commands.forEach((command) => {
+		if (!isCommandLike(command)) return;
 		command.id = uuidv4();
 	});
 }

From 095339caa80e50d093aff9cb9f0d9a0d302411f2 Mon Sep 17 00:00:00 2001
From: Christian Bager Bach Houmann <christian@bagerbach.com>
Date: Mon, 27 Jul 2026 17:29:59 +0200
Subject: [PATCH 2/8] fix(macros): a malformed command list no longer costs the
 user the macro editor

Before this, every malformed `macro.commands` landed in the same place: the
error card #1584 introduced, with no controls - a macro repairable only by
hand-editing data.json. Verified live against seven shapes; four of them were
dead ends that did not need to be.

The editor now has three states, decided by whether the value could still be
CARRYING commands:

- a readable array -> fully editable. A duplicate-id or id-less command is kept
  under a fresh uuid (normalizeCommandList) rather than dropped, so the user can
  see and delete it; a `null` hole, which carries nothing, is stepped over.
- a value that carries nothing (null, {}, absent) -> an empty but fully usable
  editor. `macro: null` previously made "Configure" do nothing at all: display()
  runs from MacroBuilder's constructor, so the throw took the modal with it.
- a value we cannot read that could be carrying commands ({"0":...}, "str", 7)
  -> a card that says so, with every writing affordance withheld, so the `[]` we
  read it as can never reach disk. render() reports this to its host, because
  ConditionalBranchEditorModal's Save button lives outside the editor and would
  otherwise overwrite an unreadable branch in one click.

The repair is never persisted by opening a macro - only by the user's first
ordinary edit - so open-and-close leaves data.json byte-identical.

Refs #1593
---
 src/gui/MacroGUIs/CommandList.svelte          |  38 +++-
 src/gui/MacroGUIs/CommandSequenceEditor.ts    |  75 +++++--
 ...CommandSequenceEditor.unrenderable.test.ts | 197 +++++++++++++-----
 .../MacroGUIs/ConditionalBranchEditorModal.ts |  33 ++-
 src/gui/MacroGUIs/MacroBuilder.ts             |  46 +++-
 src/gui/svelte/DataUnreadable.svelte          |  72 +++++++
 6 files changed, 379 insertions(+), 82 deletions(-)
 create mode 100644 src/gui/svelte/DataUnreadable.svelte

diff --git a/src/gui/MacroGUIs/CommandList.svelte b/src/gui/MacroGUIs/CommandList.svelte
index 00298ae85..44395a71b 100644
--- a/src/gui/MacroGUIs/CommandList.svelte
+++ b/src/gui/MacroGUIs/CommandList.svelte
@@ -43,6 +43,32 @@ let {
 	onEditElseBranch,
 }: CommandListProps = $props();
 
+// Everything rendered, handed to the dnd zone, or reordered is filtered to
+// entries that can actually be keyed: an object with a unique, non-empty string
+// id. svelte-dnd-action reads `.id` on every item and the keyed {#each} throws
+// `each_key_duplicate` on a repeat (#1451, #1593) — and on a POST-mount update
+// that throw escapes mountComponent's try entirely (see its doc comment), so
+// this has to be a $derived, not a one-off check at setup.
+//
+// CommandSequenceEditor normalizes the list before it ever gets here, so in
+// practice this filter is a no-op: an id-less or duplicate-id command has
+// already been given a fresh uuid and KEPT. That matters, because the persist
+// path below writes back the list this zone was seeded with — a filter that
+// silently dropped a real command would delete it from data.json on the first
+// reorder. The only thing it can still drop is a `null`/primitive hole, which
+// carries nothing.
+const renderable = $derived.by(() => {
+	const seen = new Set<string>();
+	if (!Array.isArray(commands)) return [];
+	return commands.filter((command) => {
+		if (typeof command !== "object" || command === null) return false;
+		const id = command.id;
+		if (typeof id !== "string" || id === "" || seen.has(id)) return false;
+		seen.add(id);
+		return true;
+	});
+});
+
 const isMobile = Platform.isMobile;
 // Desktop: drag is armed by grabbing the handle (shared with the choices list; see
 // createDragArming for the click-swallow failsafe). Mobile: no handle — the whole row
@@ -94,7 +120,9 @@ let startDrag = () => {
 // Keyboard reorder (ArrowUp/ArrowDown on a row's drag handle): move the command one
 // step and persist via the same snapshot path as a pointer drag's finalize.
 function moveCommand(id: string, direction: -1 | 1) {
-	const list = stripShadow(commands);
+	// `renderable`, not `commands`: stripShadow reads `item.id`, so the raw list
+	// would throw on the very hole the render filter exists to hide.
+	const list = stripShadow(renderable);
 	const index = list.findIndex((c) => c.id === id);
 	if (index === -1) return;
 	const target = index + direction;
@@ -112,7 +140,9 @@ function moveCommand(id: string, direction: -1 | 1) {
 }
 
 function updateCommand(command: ICommand) {
-	commands = replaceById(commands, command);
+	// `renderable` for the same reason as moveCommand: replaceById maps over
+	// `item.id`.
+	commands = replaceById(renderable, command);
 	persist();
 }
 
@@ -198,7 +228,7 @@ async function configureOpenFile(command: IOpenFileCommand) {
 <ol
 	class="quickAddCommandList"
 	use:dndzone={baseDndOptions({
-		items: commands,
+		items: renderable,
 		dragDisabled,
 		type: "command",
 		// A command's `.name` differs from its rendered label for Choice/Conditional
@@ -209,7 +239,7 @@ async function configureOpenFile(command: IOpenFileCommand) {
 	onconsider={handleConsider}
 	onfinalize={handleSort}
 >
-	{#each stripShadow(commands) as command (command.id)}
+	{#each stripShadow(renderable) as command (command.id)}
 		{#if command.type === CommandType.Wait}
 			<WaitCommand
 				command={asWait(command)}
diff --git a/src/gui/MacroGUIs/CommandSequenceEditor.ts b/src/gui/MacroGUIs/CommandSequenceEditor.ts
index d9ea045b5..605e2becf 100644
--- a/src/gui/MacroGUIs/CommandSequenceEditor.ts
+++ b/src/gui/MacroGUIs/CommandSequenceEditor.ts
@@ -53,6 +53,11 @@ import type { IConditionalCommand } from "../../types/macros/Conditional/ICondit
 import { getUserScriptMemberAccess } from "../../utilityObsidian";
 import { ConditionalCommand } from "../../types/macros/Conditional/ConditionalCommand";
 import { clearUserScriptSecretsFromCommand } from "../../utils/userScriptSecrets";
+import {
+	isUnreadableCommandList,
+	normalizeCommandList,
+} from "../../utils/macroUtils";
+import DataUnreadable from "../svelte/DataUnreadable.svelte";
 
 type ConditionalHandler = (command: IConditionalCommand) => Promise<boolean>;
 
@@ -65,7 +70,11 @@ export interface CommandSequenceEditorConditionalHandlers {
 export interface CommandSequenceEditorOptions {
 	app: App;
 	plugin: QuickAdd;
-	commands: ICommand[];
+	/**
+	 * Typed `unknown` because it is not: this is `macro.commands` straight out of
+	 * `data.json` (see commandListOf). The editor normalizes it at construction.
+	 */
+	commands: unknown;
 	choices: IChoice[];
 	onCommandsChange?: (commands: ICommand[]) => void;
 	conditionalHandlers?: CommandSequenceEditorConditionalHandlers;
@@ -79,16 +88,32 @@ export class CommandSequenceEditor {
 	private readonly conditionalHandlers?: CommandSequenceEditorConditionalHandlers;
 
 	private commandsRef: ICommand[];
+	/**
+	 * True when the value handed to us was not a list we could read AND could
+	 * still be carrying commands (see isUnreadableCommandList). The editor is
+	 * read-only in that state: `commandsRef` is an empty array that must never
+	 * reach disk over the real value.
+	 */
+	private readonly unreadable: boolean;
 	private obsidianCommands: IObsidianCommand[] = [];
 	private scriptCandidates: ScriptCandidate[] = [];
 	private commandListHandle: MountHandle | null = null;
 	private commandListProps: CommandListProps | null = null;
 	private containerEl: HTMLElement | null = null;
+	private unreadableCardHandle: MountHandle | null = null;
 
 	constructor(options: CommandSequenceEditorOptions) {
 		this.app = options.app;
 		this.plugin = options.plugin;
-		this.commandsRef = options.commands;
+		// data.json is untrusted, so the value arrives raw and is made editable
+		// here — the one seam every host that shows a command list goes through
+		// (MacroBuilder, ConditionalBranchEditorModal). Normalizing keeps a
+		// duplicate-id or id-less command under a fresh uuid instead of letting
+		// the keyed {#each} throw and cost the user the whole editor (#1593).
+		// Nothing is persisted by this: the repair reaches disk only with the
+		// user's first ordinary edit, so opening and closing changes nothing.
+		this.unreadable = isUnreadableCommandList(options.commands);
+		this.commandsRef = normalizeCommandList(options.commands).commands;
 		this.choices = options.choices;
 		this.onCommandsChange = options.onCommandsChange;
 		this.conditionalHandlers = options.conditionalHandlers;
@@ -97,34 +122,58 @@ export class CommandSequenceEditor {
 		this.loadScriptCandidates();
 	}
 
-	public render(containerEl: HTMLElement) {
+	/**
+	 * @returns whether the editor is fully usable. False means the command list is
+	 * showing a card instead, and the host must not commit `commandsRef` anywhere
+	 * (see ConditionalBranchEditorModal's Save button).
+	 */
+	public render(containerEl: HTMLElement): boolean {
 		this.destroy();
 		this.containerEl = containerEl;
 		containerEl.empty();
 		containerEl.addClass("quickAddCommandEditor");
 
-		// A list we could not draw must not be edited blind. Every control below
-		// appends to `commandsRef` and persists through onCommandsChange, so with the
-		// list invisible the user would be adding commands they cannot see, reorder or
-		// delete — and with a `null`/non-iterable `commands` the same controls throw
-		// silently instead. Before #1584 this was unreachable (the throw escaped and
-		// the modal never opened at all); now that the editor survives, it has to stop
-		// offering the affordances the failed view was the only way to review.
-		// The card explains what happened; the macro's name, "run on startup" and icon
-		// stay editable around it.
-		if (!this.renderCommandList(containerEl)) return;
+		// A list we could not read must not be edited blind. Every control below
+		// appends to `commandsRef` and persists through onCommandsChange, so an
+		// editor offered over a value we could not read would overwrite it with the
+		// `[]` we read it as — destroying the only copy the user has. Say so
+		// instead, and offer nothing that writes. The macro's name, "run on
+		// startup" and icon stay editable around it.
+		if (this.unreadable) {
+			this.renderUnreadableCard(containerEl);
+			return false;
+		}
+
+		// Belt and braces: the list is a readable array and every entry has been
+		// given a unique id, so a mount failure here is a genuine bug rather than
+		// bad data. Same reasoning as above though — with the list invisible the
+		// user would be adding commands they cannot see, reorder or delete.
+		if (!this.renderCommandList(containerEl)) return false;
 
 		this.renderCommandBar(containerEl);
 		this.renderAddObsidianCommandSetting(containerEl);
 		this.renderAddEditorCommandSetting(containerEl);
 		this.renderAddUserScriptSetting(containerEl);
 		this.renderAddChoiceSetting(containerEl);
+		return true;
+	}
+
+	private renderUnreadableCard(parent: HTMLElement) {
+		const cardEl = parent.createDiv("commandList");
+		this.unreadableCardHandle = mountComponent(
+			cardEl,
+			DataUnreadable,
+			{ what: "this macro's commands" },
+			{ what: "this macro's commands" },
+		);
 	}
 
 	public destroy() {
 		this.commandListHandle?.destroy();
 		this.commandListHandle = null;
 		this.commandListProps = null;
+		this.unreadableCardHandle?.destroy();
+		this.unreadableCardHandle = null;
 	}
 
 	private loadObsidianCommands(): void {
diff --git a/src/gui/MacroGUIs/CommandSequenceEditor.unrenderable.test.ts b/src/gui/MacroGUIs/CommandSequenceEditor.unrenderable.test.ts
index 05abf6663..8c28cdbff 100644
--- a/src/gui/MacroGUIs/CommandSequenceEditor.unrenderable.test.ts
+++ b/src/gui/MacroGUIs/CommandSequenceEditor.unrenderable.test.ts
@@ -4,7 +4,6 @@ vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() }));
 
 import { App } from "obsidian";
 import type QuickAdd from "../../main";
-import type { ICommand } from "../../types/macros/ICommand";
 import { CommandSequenceEditor } from "./CommandSequenceEditor";
 import { log } from "../../logger/logManager";
 
@@ -24,76 +23,164 @@ function renderEditor(commands: unknown, onCommandsChange = vi.fn()) {
 	const editor = new CommandSequenceEditor({
 		app: testApp(),
 		plugin: {} as unknown as QuickAdd,
-		commands: commands as ICommand[],
+		commands,
 		choices: [],
 		onCommandsChange,
 	});
-	editor.render(container);
-	return { container, editor, onCommandsChange };
+	const editable = editor.render(container);
+	return { container, editor, onCommandsChange, editable };
 }
 
+const addControlCount = (container: HTMLElement) =>
+	container.querySelectorAll("button").length;
+const rowCount = (container: HTMLElement) =>
+	container.querySelectorAll(".quickAddCommandList > li").length;
+
+const wait = (id: string, time = 100) => ({ id, name: "Wait", type: "Wait", time });
+
 /**
- * #1584 follow-through. Once mountComponent stopped letting a broken CommandList
- * take the macro builder down with it, the builder started OPENING over a list it
- * could not draw - and every add control was still live. Two ways that goes wrong:
- *
- *   - `commands: null` (the issue's own live repro): every add control throws
- *     `[...null]` inside its click handler. Dead buttons, silently.
- *   - a valid array with duplicate ids: `{#each ... (command.id)}` throws
- *     `each_key_duplicate`, so the list is invisible - but the add controls WORK,
- *     appending commands the user cannot see, review or delete, and persisting
- *     them on close.
+ * The macro editor's three states over `macro.commands` from data.json (#1593).
  *
- * A list we could not draw must not be edited blind.
+ * #1584 stopped a broken command list from taking the macro builder down with
+ * it, but every malformed shape then landed in the same place: an error card
+ * with no controls, i.e. a macro that could only be repaired by hand-editing
+ * data.json. That is honest for a value we genuinely cannot read; it is far too
+ * blunt for the two shapes that are a perfectly good array with one bad entry,
+ * and for the shapes that carry nothing at all.
  */
-describe("CommandSequenceEditor over an unrenderable command list (#1584)", () => {
-	const addControlCount = (container: HTMLElement) =>
-		container.querySelectorAll("button").length;
-
-	it("shows the error card instead of the list", () => {
-		vi.spyOn(log, "logError").mockImplementation(() => {});
-		const { container } = renderEditor(null);
-
-		const card = container.querySelector(".qaMountFailed");
-		expect(card?.textContent).toContain(
-			"QuickAdd couldn't display this macro's commands",
-		);
-		vi.restoreAllMocks();
+describe("CommandSequenceEditor over a malformed command list (#1593)", () => {
+	describe("a readable array with a bad entry stays fully editable", () => {
+		it("renders both commands when two share an id, keeping the second", () => {
+			const { container, editable } = renderEditor([
+				wait("dup", 100),
+				wait("dup", 200),
+			]);
+
+			expect(editable).toBe(true);
+			expect(container.querySelector(".qaMountFailed")).toBeNull();
+			expect(rowCount(container)).toBe(2);
+			expect(addControlCount(container)).toBeGreaterThan(0);
+		});
+
+		it("renders a command that has no id at all", () => {
+			const { container, editable } = renderEditor([
+				{ name: "Readwise sync", type: "UserScript", path: "s.js" },
+				wait("ok"),
+			]);
+
+			expect(editable).toBe(true);
+			expect(rowCount(container)).toBe(2);
+		});
+
+		it("renders the commands either side of a null hole", () => {
+			const { container, editable } = renderEditor([wait("a"), null, wait("b")]);
+
+			expect(editable).toBe(true);
+			expect(rowCount(container)).toBe(2);
+		});
+
+		it("does not persist the repair until the user actually edits something", () => {
+			const { onCommandsChange } = renderEditor([wait("dup"), wait("dup")]);
+
+			// Opening a macro must not write to data.json. The re-id rides along with
+			// the user's first ordinary edit instead.
+			expect(onCommandsChange).not.toHaveBeenCalled();
+		});
+	});
+
+	describe("a value that carries nothing opens an empty, usable editor", () => {
+		it.each([
+			["null", null],
+			["undefined", undefined],
+			["an empty object", {}],
+			["an empty string", ""],
+			["an empty array", []],
+		])("offers the full editor for %s", (_label, commands) => {
+			const { container, editable } = renderEditor(commands);
+
+			expect(editable).toBe(true);
+			expect(container.querySelector(".qaDataUnreadable")).toBeNull();
+			expect(container.querySelector(".quickCommandContainer")).not.toBeNull();
+			expect(addControlCount(container)).toBeGreaterThan(0);
+			expect(rowCount(container)).toBe(0);
+		});
 	});
 
-	it("offers no control that would edit the list it could not draw", () => {
-		vi.spyOn(log, "logError").mockImplementation(() => {});
-		const { container } = renderEditor(null);
+	describe("a value that could be carrying commands is read-only", () => {
+		it.each([
+			["an array-turned-object", { "0": wait("hidden") }],
+			["a string", "not a list"],
+			["a JSON string", '[{"id":"x","type":"Wait","name":"Wait"}]'],
+			["a number", 7],
+		])("shows the unreadable card and no edit controls for %s", (_label, commands) => {
+			const { container, editable } = renderEditor(commands);
 
-		// The quick-command bar and the four "Add …" rows are all gone.
-		expect(addControlCount(container)).toBe(0);
-		expect(container.querySelector(".quickCommandContainer")).toBeNull();
-		expect(container.textContent).not.toContain("Obsidian command");
-		expect(container.textContent).not.toContain("User scripts");
-		vi.restoreAllMocks();
+			expect(editable).toBe(false);
+			expect(container.querySelector(".qaDataUnreadable")?.textContent).toContain(
+				"QuickAdd couldn't read this macro's commands",
+			);
+			// Nothing that could write the `[]` we read the value as.
+			expect(addControlCount(container)).toBe(0);
+			expect(container.querySelector(".quickCommandContainer")).toBeNull();
+			expect(container.textContent).not.toContain("Obsidian command");
+			expect(container.textContent).not.toContain("User scripts");
+		});
+
+		it("says the value has not been touched, and does not ask for a bug report", () => {
+			const { container } = renderEditor("not a list");
+			const text = (container.textContent ?? "").replace(/\s+/g, " ");
+
+			expect(text).toContain("will not overwrite it");
+			expect(text).toContain("data.json");
+			// This is the user's data, not a QuickAdd bug: MountFailed's copy would
+			// send them to file an issue with no error message to put in it.
+			expect(text).not.toContain("report this");
+		});
+
+		it("never emits a change for a value it could not read", () => {
+			const { onCommandsChange } = renderEditor({ "0": wait("hidden") });
+			expect(onCommandsChange).not.toHaveBeenCalled();
+		});
 	});
 
-	it("cannot append to a list the user cannot see", () => {
-		vi.spyOn(log, "logError").mockImplementation(() => {});
-		// A valid array, so nothing throws on append - only the RENDER fails. This is
-		// the shape where an ungated editor silently persists invisible commands.
-		const duplicateIds = [
-			{ id: "dup", name: "One", type: "Wait" },
-			{ id: "dup", name: "Two", type: "Wait" },
-		];
-		const { container, onCommandsChange } = renderEditor(duplicateIds);
-
-		expect(container.querySelector(".qaMountFailed")).not.toBeNull();
-		expect(addControlCount(container)).toBe(0);
-		expect(onCommandsChange).not.toHaveBeenCalled();
-		vi.restoreAllMocks();
+	describe("a genuine mount failure still withholds the controls (#1584)", () => {
+		it("shows the error card instead of the list", () => {
+			vi.spyOn(log, "logError").mockImplementation(() => {});
+			// A component that throws on mount is the only way left to reach this arm
+			// now that data shapes are handled above; simulate it by breaking the
+			// props the list needs.
+			const container = document.createElement("div");
+			document.body.appendChild(container);
+			const editor = new CommandSequenceEditor({
+				app: testApp(),
+				plugin: {} as unknown as QuickAdd,
+				// A frozen array whose entries are getters that throw reproduces a
+				// render-time explosion without depending on Svelte internals.
+				commands: [
+					Object.defineProperty({ id: "boom" }, "type", {
+						get() {
+							throw new Error("boom");
+						},
+						enumerable: true,
+					}),
+				],
+				choices: [],
+			});
+			const editable = editor.render(container);
+
+			expect(editable).toBe(false);
+			expect(container.querySelector(".qaMountFailed")?.textContent).toContain(
+				"QuickAdd couldn't display this macro's commands",
+			);
+			expect(addControlCount(container)).toBe(0);
+			vi.restoreAllMocks();
+		});
 	});
 
-	it("still builds the whole editor when the list renders", () => {
-		const { container } = renderEditor([
-			{ id: "wait-1", name: "Wait", type: "Wait", time: 100 },
-		]);
+	it("still builds the whole editor when the list is healthy", () => {
+		const { container, editable } = renderEditor([wait("wait-1")]);
 
+		expect(editable).toBe(true);
 		expect(container.querySelector(".qaMountFailed")).toBeNull();
 		expect(container.querySelector(".quickCommandContainer")).not.toBeNull();
 		expect(addControlCount(container)).toBeGreaterThan(0);
diff --git a/src/gui/MacroGUIs/ConditionalBranchEditorModal.ts b/src/gui/MacroGUIs/ConditionalBranchEditorModal.ts
index 29f8cd883..f55ef7dbb 100644
--- a/src/gui/MacroGUIs/ConditionalBranchEditorModal.ts
+++ b/src/gui/MacroGUIs/ConditionalBranchEditorModal.ts
@@ -4,6 +4,7 @@ import type QuickAdd from "../../main";
 import type IChoice from "../../types/choices/IChoice";
 import type { ICommand } from "../../types/macros/ICommand";
 import { deepClone } from "../../utils/deepClone";
+import { commandListOf } from "../../utils/macroUtils";
 import {
 	CommandSequenceEditor,
 	type CommandSequenceEditorConditionalHandlers,
@@ -14,7 +15,8 @@ interface ConditionalBranchEditorModalOptions {
 	plugin: QuickAdd;
 	choices: IChoice[];
 	title: string;
-	commands: ICommand[];
+	/** Raw `thenCommands`/`elseCommands` out of data.json — see commandListOf. */
+	commands: unknown;
 	conditionalHandlers: CommandSequenceEditorConditionalHandlers;
 }
 
@@ -22,7 +24,7 @@ export class ConditionalBranchEditorModal extends Modal {
 	public waitForClose: Promise<ICommand[] | null>;
 	private resolvePromise!: (commands: ICommand[] | null) => void;
 	private commandEditor: CommandSequenceEditor | null = null;
-	private workingCommands: ICommand[];
+	private workingCommands: unknown;
 	private readonly plugin: QuickAdd;
 	private readonly choices: IChoice[];
 	private readonly conditionalHandlers: CommandSequenceEditorConditionalHandlers;
@@ -75,16 +77,35 @@ export class ConditionalBranchEditorModal extends Modal {
 			},
 			conditionalHandlers: this.conditionalHandlers,
 		});
-		this.commandEditor.render(editorContainer);
+		const editable = this.commandEditor.render(editorContainer);
 
-		this.renderButtonBar();
+		this.renderButtonBar(editable);
 	}
 
-	private renderButtonBar() {
+	/**
+	 * The Save button lives OUTSIDE the command editor, so suppressing the
+	 * editor's own controls is not enough: Save resolves `workingCommands`, which
+	 * MacroBuilder writes onto the conditional's `thenCommands`/`elseCommands`. If
+	 * the branch held a value we could not read, that one click would replace it
+	 * with the empty list we read it as. There is nothing to save in that state,
+	 * so the button bar offers only a way out (#1593).
+	 */
+	private renderButtonBar(editable: boolean) {
 		const buttonContainer = this.contentEl.createDiv({
 			cls: "qa-command-button-row",
 		});
 
+		if (!editable) {
+			new ButtonComponent(buttonContainer)
+				.setCta()
+				.setButtonText("Close")
+				.onClick(() => {
+					this.resolve(null);
+					this.close();
+				});
+			return;
+		}
+
 		new ButtonComponent(buttonContainer)
 			.setButtonText("Cancel")
 			.onClick(() => {
@@ -96,7 +117,7 @@ export class ConditionalBranchEditorModal extends Modal {
 			.setCta()
 			.setButtonText("Save")
 			.onClick(() => {
-				this.resolve(this.workingCommands);
+				this.resolve(commandListOf(this.workingCommands));
 				this.close();
 			});
 	}
diff --git a/src/gui/MacroGUIs/MacroBuilder.ts b/src/gui/MacroGUIs/MacroBuilder.ts
index df82e6fa7..b9cc10489 100644
--- a/src/gui/MacroGUIs/MacroBuilder.ts
+++ b/src/gui/MacroGUIs/MacroBuilder.ts
@@ -19,6 +19,12 @@ import {
 	isChoiceLike,
 	rootChoicesOf,
 } from "../../utils/choiceUtils";
+import {
+	isCommandLike,
+	isUnreadableCommandList,
+} from "../../utils/macroUtils";
+import type { ICommand } from "../../types/macros/ICommand";
+import { v4 as uuidv4 } from "uuid";
 
 /** Exported for the malformed-tree sweep (src/utils/malformedChoices.entrypoints.test.ts). */
 export function getChoicesAsList(nestedChoices: IChoice[]): IChoice[] {
@@ -108,9 +114,11 @@ export class MacroBuilder extends Modal {
 					);
 					if (!newName) return;
 
-					// Keep choice name and macro name in sync
+					// Keep choice name and macro name in sync. The macro object can
+					// be missing from a hand-edited data.json; renaming the choice
+					// still has to work, so only sync a macro that is there.
 					this.choice.name = newName;
-					this.macro.name = newName;
+					if (isCommandLike(this.macro)) this.macro.name = newName;
 					this.reload();
 				} catch {
 					// Prompt cancelled (Esc/Cancel) — keep the current name.
@@ -143,15 +151,45 @@ export class MacroBuilder extends Modal {
 		this.display();
 	}
 
+	/**
+	 * The value to show as this macro's command list.
+	 *
+	 * `choice.macro` is untrusted too, and a Macro choice whose `macro` key is
+	 * missing entirely used to make "Configure" do nothing at all: `display()`
+	 * runs from the constructor, before `open()`, so the throw took the modal with
+	 * it. When the value carries nothing there is nothing to lose and the editor
+	 * opens empty; when it is something we cannot read, it is passed straight
+	 * through so the editor shows its unreadable card rather than offering to
+	 * overwrite it.
+	 */
+	private macroCommandsValue(): unknown {
+		if (isCommandLike(this.macro)) return this.macro.commands;
+		return isUnreadableCommandList(this.macro) ? this.macro : undefined;
+	}
+
+	/**
+	 * Commit an edit back onto the choice, materializing the macro object if it
+	 * was missing. Only reachable when the editor is usable, which
+	 * `macroCommandsValue` guarantees means nothing readable is being replaced.
+	 */
+	private setMacroCommands(commands: ICommand[]) {
+		if (!isCommandLike(this.macro)) {
+			this.macro = { id: uuidv4(), name: this.choice.name, commands };
+			this.choice.macro = this.macro;
+			return;
+		}
+		this.macro.commands = commands;
+	}
+
 	private addCommandEditor() {
 		const editorContainer = this.contentEl.createDiv("macroBuilder__editor");
 		this.commandEditor = new CommandSequenceEditor({
 			app: this.app,
 			plugin: this.plugin,
-			commands: this.macro.commands,
+			commands: this.macroCommandsValue(),
 			choices: this.choices,
 			onCommandsChange: (commands) => {
-				this.macro.commands = commands;
+				this.setMacroCommands(commands);
 			},
 			conditionalHandlers: this.buildConditionalHandlers(),
 		});
diff --git a/src/gui/svelte/DataUnreadable.svelte b/src/gui/svelte/DataUnreadable.svelte
new file mode 100644
index 000000000..62e609235
--- /dev/null
+++ b/src/gui/svelte/DataUnreadable.svelte
@@ -0,0 +1,72 @@
+<script lang="ts">
+	import ObsidianIcon from "../components/ObsidianIcon.svelte";
+
+	let {
+		what,
+	}: {
+		/** Noun phrase naming what could not be read ("this macro's commands"). */
+		what: string;
+	} = $props();
+</script>
+
+<!--
+  Shown when a value in data.json could not be READ - not when something threw.
+  The distinction is the whole point of this card existing next to MountFailed:
+  nothing is broken in QuickAdd, so "include the message below if you report
+  this" would send the user to file a bug about their own file, and there is no
+  error message to give them anyway.
+
+  What they need instead is the three things below: that we could not read it,
+  that we have not touched it and will not, and where it lives. Same promise the
+  choice list makes for an unreadable folder (ChoicesUnavailable.svelte) - kept
+  as a separate component rather than a shared one because that card's copy is
+  specifically about the choice tree, and a card that says the wrong noun is
+  worse than one more small component.
+
+  Deliberately offers no button. Anything that could rewrite data.json would
+  destroy the very value the user needs in order to recover.
+-->
+<div class="qaDataUnreadable">
+	<div class="qaDataUnreadableHead">
+		<ObsidianIcon iconId="alert-triangle" size={16} />
+		<span class="qaDataUnreadableTitle">QuickAdd couldn't read {what}</span>
+	</div>
+	<p class="qaDataUnreadableBody">
+		The saved value isn't in a shape QuickAdd understands, so it can't be shown
+		or edited here. Nothing has been changed or deleted, and QuickAdd will not
+		overwrite it.
+	</p>
+	<p class="qaDataUnreadableBody">
+		It's stored in <code>.obsidian/plugins/quickadd/data.json</code> inside this
+		vault. Make a copy of that file before editing it.
+	</p>
+</div>
+
+<style>
+	.qaDataUnreadable {
+		border: 1px solid var(--background-modifier-border);
+		border-radius: var(--radius-m);
+		padding: 12px 14px;
+		margin: 4px 0 8px;
+		background: var(--background-secondary);
+	}
+
+	.qaDataUnreadableHead {
+		display: flex;
+		align-items: center;
+		gap: 8px;
+		color: var(--text-warning, var(--text-normal));
+	}
+
+	.qaDataUnreadableTitle {
+		font-weight: var(--font-semibold);
+		color: var(--text-normal);
+	}
+
+	.qaDataUnreadableBody {
+		margin: 8px 0 0;
+		color: var(--text-muted);
+		font-size: var(--font-ui-small, 13px);
+		max-width: 68ch;
+	}
+</style>

From 3c27d1e13943f390b27024efc8184fcfdd43a2ee Mon Sep 17 00:00:00 2001
From: Christian Bager Bach Houmann <christian@bagerbach.com>
Date: Mon, 27 Jul 2026 17:34:58 +0200
Subject: [PATCH 3/8] fix(macros): a macro that can't be read fails loudly
 instead of throwing or lying

Two of the malformed shapes were broken outside the UI too, both because
`if (!this.macro.commands)` is a truthiness test: an array-turned-object reached
`for..of` and surfaced a bare "i is not iterable" to the user, and a string
reached it INTACT - strings are iterable - so the macro reported success having
walked its own characters and run nothing. A conditional's branch had the same
hole one level down, skipped as silently as a branch the user left empty.

Both now say what happened, name the macro, and point at data.json.

Also routes the remaining command-list walkers through the accessor, guarding
them at their own entry point so the recursion into then/else branches is
covered too: packageTraversal, packageImportService, packagePreview,
collectChoiceRequirements, SingleMacroEngine.

Deliberately NOT swept: removeMacroIndirection's `macro.commands || []`. That
is truthiness, so it already passes an unreadable value through VERBATIM;
substituting the accessor would replace it with [] at the one migration that
then deletes settings.macros and is flagged complete forever.

Refs #1593
---
 src/engine/MacroChoiceEngine.ts            | 28 ++++++++++++++++++++--
 src/engine/SingleMacroEngine.ts            | 15 +++++++++---
 src/preflight/collectChoiceRequirements.ts |  4 +++-
 src/services/packageImportService.ts       | 23 +++++++++++-------
 src/services/packagePreview.ts             | 10 ++++----
 src/utils/packageTraversal.ts              | 27 +++++++++++----------
 6 files changed, 76 insertions(+), 31 deletions(-)

diff --git a/src/engine/MacroChoiceEngine.ts b/src/engine/MacroChoiceEngine.ts
index e9984ba96..1d8e51998 100644
--- a/src/engine/MacroChoiceEngine.ts
+++ b/src/engine/MacroChoiceEngine.ts
@@ -59,6 +59,7 @@ import { evaluateCondition } from "./helpers/conditionalEvaluator";
 import { handleMacroAbort } from "../utils/macroAbortHandler";
 import { buildOpenFileOptions } from "./helpers/openFileOptions";
 import { createVariablesProxy } from "../utils/variablesProxy";
+import { commandListOf, isUnreadableCommandList } from "../utils/macroUtils";
 
 type ConditionalScriptRunner = () => Promise<unknown>;
 type UserScriptFunction = (
@@ -263,14 +264,27 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 	}
 
 	async run(): Promise<void> {
-		if (!this.macro || !this.macro.commands) {
+		// `!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.
+		if (isUnreadableCommandList(this.macro?.commands)) {
+			log.logError(
+				`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.`
+			);
+			return;
+		}
+
+		const commands = commandListOf(this.macro?.commands);
+		if (commands.length === 0) {
 			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 +769,16 @@ 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).
+		if (isUnreadableCommandList(branch)) {
+			log.logError(
+				`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.`
+			);
+			return;
+		}
+
 		if (!Array.isArray(branch) || branch.length === 0) {
 			return;
 		}
diff --git a/src/engine/SingleMacroEngine.ts b/src/engine/SingleMacroEngine.ts
index 973c5178e..36a2f5455 100644
--- a/src/engine/SingleMacroEngine.ts
+++ b/src/engine/SingleMacroEngine.ts
@@ -15,6 +15,7 @@ import {
 } from "../utils/userScriptSecrets";
 import { MacroChoiceEngine } from "./MacroChoiceEngine";
 import { handleMacroAbort } from "../utils/macroAbortHandler";
+import { commandListOf, hasCommandList } from "../utils/macroUtils";
 import { MacroAbortError } from "../errors/MacroAbortError";
 
 // Member names that QuickAdd itself treats as conventions/metadata rather than entrypoints:
@@ -206,8 +207,10 @@ export class SingleMacroEngine {
 		macroChoice: IMacroChoice,
 		memberAccess: string[],
 	): Promise<{ executed: boolean; result?: unknown }> {
-		const originalCommands = macroChoice.macro?.commands;
-		if (!originalCommands?.length) {
+		// commandListOf, not `?.length`: a string `commands` reports a length and
+		// then throws on `.map` below (#1593).
+		const originalCommands = commandListOf(macroChoice.macro?.commands);
+		if (!originalCommands.length) {
 			return { executed: false };
 		}
 
@@ -236,7 +239,13 @@ export class SingleMacroEngine {
 				this.ensureNotAborted();
 			}
 
-			const updatedCommands = macroChoice.macro?.commands ?? originalCommands;
+			// Fall back to the list we started from if a pre-command replaced
+			// `commands` with something unreadable, rather than carrying that value
+			// into the index arithmetic below.
+			const refreshed = macroChoice.macro?.commands;
+			const updatedCommands = hasCommandList(refreshed)
+				? (refreshed as typeof originalCommands)
+				: originalCommands;
 			// Pre-commands may have mutated the commands array, so re-resolve the selected
 			// command by its stable id. Both the candidate and the post-command slice are
 			// derived from this refreshed index so they cannot drift apart. The original
diff --git a/src/preflight/collectChoiceRequirements.ts b/src/preflight/collectChoiceRequirements.ts
index 7d72b9f59..ea4ecf60e 100644
--- a/src/preflight/collectChoiceRequirements.ts
+++ b/src/preflight/collectChoiceRequirements.ts
@@ -13,6 +13,7 @@ import type IChoice from "src/types/choices/IChoice";
 import type IMacroChoice from "src/types/choices/IMacroChoice";
 import type ITemplateChoice from "src/types/choices/ITemplateChoice";
 import { CommandType } from "src/types/macros/CommandType";
+import { commandListOf } from "src/utils/macroUtils";
 import type { ICommand } from "src/types/macros/ICommand";
 import type { IUserScript } from "src/types/macros/IUserScript";
 import {
@@ -506,7 +507,8 @@ async function collectMacroScriptRequirements(
 	preloadedUserScripts?: Map<string, unknown>,
 ): Promise<FieldRequirement[]> {
 	const requirements: FieldRequirement[] = [];
-	const commands: ICommand[] = choice?.macro?.commands ?? [];
+	// `?? []` would pass an array-turned-object straight into the loop below.
+	const commands: ICommand[] = commandListOf(choice?.macro?.commands);
 
 	for (const command of commands) {
 		if (command?.type !== CommandType.UserScript) continue;
diff --git a/src/services/packageImportService.ts b/src/services/packageImportService.ts
index 99a434686..dc6cc5308 100644
--- a/src/services/packageImportService.ts
+++ b/src/services/packageImportService.ts
@@ -17,6 +17,7 @@ import {
 	hasUnreadableChildren,
 	isChoiceLike,
 } from "../utils/choiceUtils";
+import { commandListOf, isCommandLike } from "../utils/macroUtils";
 import type { ICommand } from "../types/macros/ICommand";
 import type { IChoiceCommand } from "../types/macros/IChoiceCommand";
 import type { IConditionalCommand } from "../types/macros/Conditional/IConditionalCommand";
@@ -903,11 +904,12 @@ function remapChoiceTree(
 
 	if (choice.type === "Macro") {
 		const macroChoice = choice as IMacroChoice;
-		if (isDuplicated) {
+		// `macro` is untrusted too: an imported package can omit it entirely.
+		if (isDuplicated && macroChoice.macro) {
 			macroChoice.macro.id = uuidv4();
 		}
 		remapCommands(
-			macroChoice.macro.commands,
+			macroChoice.macro?.commands,
 			idMap,
 			importableChoiceIds,
 			isDuplicated,
@@ -935,14 +937,17 @@ function remapChoiceTree(
 }
 
 function remapCommands(
-	commands: ICommand[],
+	// `unknown`: raw `macro.commands` / branch values out of an imported package.
+	commands: unknown,
 	idMap: Map<string, string>,
 	importableChoiceIds: Set<string>,
 	shouldRegenerateIds: boolean,
 	secretSanitizerOptions: UserScriptSecretSanitizerOptions,
 ): void {
-	for (const command of commands) {
-		if (!command) continue;
+	// Mutates each command in place, so a value we cannot read is simply left
+	// alone rather than replaced with the [] we read it as.
+	for (const command of commandListOf(commands)) {
+		if (!isCommandLike(command)) continue;
 		stripUserScriptSecretRefsFromCommand(command, secretSanitizerOptions);
 
 		if (shouldRegenerateIds) {
@@ -1076,7 +1081,7 @@ function applyAssetPathOverrides(
 	switch (choice.type) {
 		case "Macro": {
 			const macroChoice = choice as IMacroChoice;
-			applyOverridesToCommands(macroChoice.macro.commands, pathOverrides);
+			applyOverridesToCommands(macroChoice.macro?.commands, pathOverrides);
 			break;
 		}
 		case "Template": {
@@ -1114,11 +1119,11 @@ function applyAssetPathOverrides(
 }
 
 function applyOverridesToCommands(
-	commands: ICommand[],
+	commands: unknown,
 	pathOverrides: Map<string, string>,
 ): void {
-	for (const command of commands) {
-		if (!command) continue;
+	for (const command of commandListOf(commands)) {
+		if (!isCommandLike(command)) continue;
 
 		switch (command.type) {
 			case CommandType.UserScript: {
diff --git a/src/services/packagePreview.ts b/src/services/packagePreview.ts
index 310d285ff..c53f0bbf3 100644
--- a/src/services/packagePreview.ts
+++ b/src/services/packagePreview.ts
@@ -15,6 +15,7 @@ import type {
 	QuickAddPackageAssetKind,
 } from "../types/packages/QuickAddPackage";
 import { flattenChoices } from "../utils/choiceUtils";
+import { commandListOf, isCommandLike } from "../utils/macroUtils";
 import { decodeFromBase64 } from "../utils/base64";
 import { extractScriptFromMarkdown } from "../utils/extractScriptFromMarkdown";
 import { MARKDOWN_FILE_EXTENSION_REGEX } from "../constants";
@@ -468,7 +469,7 @@ function collectChoice(
 				detail: joinCrumb(crumbs),
 			});
 		}
-		collectCommands(choice.macro?.commands ?? [], walk, crumbs, entryIds, depthLevel);
+		collectCommands(choice.macro?.commands, walk, crumbs, entryIds, depthLevel);
 	}
 
 	if (isTemplateChoice(choice)) {
@@ -515,14 +516,15 @@ function collectChoice(
 }
 
 function collectCommands(
-	commands: ICommand[],
+	// `unknown`: raw `macro.commands` / branch values, straight from data.json.
+	commands: unknown,
 	walk: ChoiceWalk,
 	crumbs: string[],
 	entryIds: ReadonlySet<string>,
 	depth: number,
 ): void {
-	for (const command of commands) {
-		if (!command) continue;
+	for (const command of commandListOf(commands)) {
+		if (!isCommandLike(command)) continue;
 		const label = commandLabel(command);
 		const commandCrumbs = [...crumbs, label];
 
diff --git a/src/utils/packageTraversal.ts b/src/utils/packageTraversal.ts
index 005564cc5..b81df8d15 100644
--- a/src/utils/packageTraversal.ts
+++ b/src/utils/packageTraversal.ts
@@ -9,6 +9,7 @@ import type { IUserScript } from "../types/macros/IUserScript";
 import type { IConditionalCommand } from "../types/macros/Conditional/IConditionalCommand";
 import type { INestedChoiceCommand } from "../types/macros/QuickCommands/INestedChoiceCommand";
 import { childChoicesOf, isChoiceLike, rootChoicesOf } from "./choiceUtils";
+import { commandListOf, isCommandLike } from "./macroUtils";
 import { CommandType } from "../types/macros/CommandType";
 
 export interface ChoiceCatalogEntry {
@@ -147,18 +148,20 @@ function collectChoiceDependencies(choice: IChoice): Set<string> {
 	}
 
 	if (isMacroChoice(choice)) {
-		collectDependenciesFromCommands(choice.macro.commands, dependencies);
+		collectDependenciesFromCommands(choice.macro?.commands, dependencies);
 	}
 
 	return dependencies;
 }
 
 function collectDependenciesFromCommands(
-	commands: ICommand[],
+	// `unknown`: this is `macro.commands` / a conditional's branch out of
+	// data.json, and the recursion below feeds itself (see commandListOf).
+	commands: unknown,
 	accumulator: Set<string>,
 ): void {
-	for (const command of commands) {
-		if (!command) continue;
+	for (const command of commandListOf(commands)) {
+		if (!isCommandLike(command)) continue;
 
 		switch (command.type) {
 			case CommandType.Choice: {
@@ -277,13 +280,13 @@ export function collectScriptDependencies(
 		}
 
 		if (isMacroChoice(choice)) {
-			visitCommands(choice.macro.commands);
+			visitCommands(choice.macro?.commands);
 		}
 	};
 
-	const visitCommands = (commands: ICommand[]) => {
-		for (const command of commands) {
-			if (!command) continue;
+	const visitCommands = (commands: unknown) => {
+		for (const command of commandListOf(commands)) {
+			if (!isCommandLike(command)) continue;
 			if (
 				visitReferencedChoiceFromCommand(
 					command,
@@ -364,13 +367,13 @@ export function collectFileDependencies(
 		}
 
 		if (isMacroChoice(choice)) {
-			visitCommands(choice.macro.commands);
+			visitCommands(choice.macro?.commands);
 		}
 	};
 
-	const visitCommands = (commands: ICommand[]) => {
-		for (const command of commands) {
-			if (!command) continue;
+	const visitCommands = (commands: unknown) => {
+		for (const command of commandListOf(commands)) {
+			if (!isCommandLike(command)) continue;
 			if (
 				visitReferencedChoiceFromCommand(
 					command,

From 06687b580f8a1f447ec8162aa4a388b39c390414 Mon Sep 17 00:00:00 2001
From: Christian Bager Bach Houmann <christian@bagerbach.com>
Date: Mon, 27 Jul 2026 17:39:45 +0200
Subject: [PATCH 4/8] test(macros): extend the malformed-tree ratchet to macros
 and command lists

The #1566 fixture had no Macro node at all, so two rows that already swept
macro-reaching walkers - duplicateChoice (regenerateIds) and
collectChoiceClosure (packageTraversal) - were dead coverage for them. Grows the
one shared fixture rather than forking a second one that would drift: every
malformed `commands` shape, a missing `macro` object, an unkeyable-but-readable
array (duplicate id / no id / hole), and corruption inside a conditional's
branches, all repeated at every depth.

malformedSnapshot now pins command values too, and pins ARITY for a readable
list: a walker may re-key an entry (that is the #1593 repair) but must never
lose one.

The new buildPackagePreview row found a pre-existing hole of the same class in
collectChoice, which walked a packaged folder's children and dereferenced a null
hole. Fixed alongside.

Refs #1593
---
 src/services/packagePreview.ts                |   5 +-
 .../malformedChoices.entrypoints.test.ts      |  81 +++++++++
 src/utils/malformedChoices.fixture.ts         | 169 +++++++++++++++++-
 3 files changed, 250 insertions(+), 5 deletions(-)

diff --git a/src/services/packagePreview.ts b/src/services/packagePreview.ts
index c53f0bbf3..8e5f575c2 100644
--- a/src/services/packagePreview.ts
+++ b/src/services/packagePreview.ts
@@ -14,7 +14,7 @@ import type {
 	QuickAddPackage,
 	QuickAddPackageAssetKind,
 } from "../types/packages/QuickAddPackage";
-import { flattenChoices } from "../utils/choiceUtils";
+import { flattenChoices, isChoiceLike } from "../utils/choiceUtils";
 import { commandListOf, isCommandLike } from "../utils/macroUtils";
 import { decodeFromBase64 } from "../utils/base64";
 import { extractScriptFromMarkdown } from "../utils/extractScriptFromMarkdown";
@@ -507,6 +507,9 @@ function collectChoice(
 
 	if (isMultiChoice(choice) && Array.isArray(choice.choices)) {
 		for (const child of choice.choices) {
+			// A packaged folder's list can hold a `null` hole like any other
+			// (#1566); it carries nothing, so step over it rather than deref it.
+			if (!isChoiceLike(child)) continue;
 			// Skip children that have their own top-level row (avoids double
 			// counting); recurse inline-only children so they can't hide.
 			if (entryIds.has(child.id)) continue;
diff --git a/src/utils/malformedChoices.entrypoints.test.ts b/src/utils/malformedChoices.entrypoints.test.ts
index 3f7bf083a..b7240d30d 100644
--- a/src/utils/malformedChoices.entrypoints.test.ts
+++ b/src/utils/malformedChoices.entrypoints.test.ts
@@ -39,6 +39,16 @@ import {
 	isEmptyFolderChoice,
 } from "../gui/suggesters/choiceSuggester";
 import { getChoicesAsList as macroBuilderChoiceList } from "../gui/MacroGUIs/MacroBuilder";
+import {
+	commandListOf,
+	hasCommandList,
+	isUnreadableCommandList,
+	normalizeCommandList,
+	regenerateIds,
+} from "./macroUtils";
+import type { IMacro } from "../types/macros/IMacro";
+import { buildPackagePreview } from "../services/packagePreview";
+import type { QuickAddPackage } from "../types/packages/QuickAddPackage";
 import {
 	HEALTHY_IDS,
 	leaf,
@@ -135,8 +145,79 @@ const sweeps: Sweep[] = [
 
 	// --- macro builder
 	["getChoicesAsList", (t) => macroBuilderChoiceList(t)],
+
+	// --- macro command lists (#1593). Same argument as the folder walkers above,
+	// one type over: `macro.commands` is declared ICommand[] and read raw.
+	[
+		"commandListOf (every macro)",
+		(t) => macrosIn(t).map((m) => commandListOf(m?.commands)),
+	],
+	[
+		"hasCommandList (every macro)",
+		(t) => macrosIn(t).map((m) => hasCommandList(m?.commands)),
+	],
+	[
+		"isUnreadableCommandList (every macro)",
+		(t) => macrosIn(t).map((m) => isUnreadableCommandList(m?.commands)),
+	],
+	[
+		"normalizeCommandList (every macro)",
+		(t) => macrosIn(t).map((m) => normalizeCommandList(m?.commands)),
+	],
+	[
+		"regenerateIds (every macro)",
+		// A WRITE path (duplicateChoice). Runs over a CLONE so the shared fixture
+		// tree keeps its ids; the point is that it neither throws nor rewrites an
+		// unreadable value.
+		(t) => macrosIn(structuredCloneTree(t)).map((m) => regenerateIds(m as unknown as IMacro)),
+	],
+	[
+		"buildPackagePreview (whole tree as a package)",
+		(t) => buildPackagePreview([], packageOf(t), new Set()),
+	],
 ];
 
+/** The malformed tree wrapped as an importable package, for the preview walker. */
+function packageOf(tree: IChoice[]): QuickAddPackage {
+	return {
+		schemaVersion: 1,
+		quickAddVersion: "1.18.0",
+		createdAt: "2026-06-01T00:00:00.000Z",
+		entryChoiceIds: tree.filter(isChoiceLike).map((c) => c.id),
+		choices: tree
+			.filter(isChoiceLike)
+			.map((choice) => ({ choice, pathHint: [choice.name], parentChoiceId: null })),
+		assets: [],
+	} as unknown as QuickAddPackage;
+}
+
+/** Every `macro` object in the tree, including the unreadable ones. */
+function macrosIn(tree: IChoice[]): (Record<string, unknown> | null)[] {
+	const out: (Record<string, unknown> | null)[] = [];
+	const walk = (list: unknown) => {
+		if (!Array.isArray(list)) return;
+		for (const entry of list) {
+			if (typeof entry !== "object" || entry === null) continue;
+			const node = entry as Record<string, unknown>;
+			if (node.type === "Macro") {
+				out.push(
+					typeof node.macro === "object"
+						? (node.macro as Record<string, unknown> | null)
+						: null,
+				);
+			}
+			if (node.type === "Multi") walk(node.choices);
+		}
+	};
+	walk(tree);
+	return out;
+}
+
+function structuredCloneTree(tree: IChoice[]): IChoice[] {
+	return JSON.parse(JSON.stringify(tree)) as IChoice[];
+}
+
+
 describe("every choice-tree entry point over a malformed tree (#1566)", () => {
 	it.each(sweeps)("%s does not throw", (_name, run) => {
 		expect(() => run(malformedTree())).not.toThrow();
diff --git a/src/utils/malformedChoices.fixture.ts b/src/utils/malformedChoices.fixture.ts
index 52544b273..2910e0a43 100644
--- a/src/utils/malformedChoices.fixture.ts
+++ b/src/utils/malformedChoices.fixture.ts
@@ -57,6 +57,82 @@ export const MALFORMED_CHILDREN_SHAPES: {
 	{ key: "number", value: 7, lossy: true },
 ];
 
+/**
+ * The same question for `macro.commands`, which has exactly the same problem
+ * (#1593): declared `ICommand[]`, read straight out of an untrusted data.json.
+ * Kept as its own list because a command list has two failure modes a choice
+ * list does not - it is also reachable as a conditional's `thenCommands` /
+ * `elseCommands`, and a perfectly good ARRAY can still be unrenderable.
+ */
+export const MALFORMED_COMMANDS_SHAPES: {
+	key: string;
+	value: unknown;
+	/** Whether the value could still be holding commands we cannot read. */
+	lossy: boolean;
+}[] = [
+	{ key: "missing", value: undefined, lossy: false },
+	{ key: "null", value: null, lossy: false },
+	{ key: "emptyObject", value: {}, lossy: false },
+	{ key: "arrayLikeObject", value: { "0": waitCommand("hidden-cmd") }, lossy: true },
+	{ key: "string", value: "not a list", lossy: true },
+	{ key: "number", value: 7, lossy: true },
+];
+
+export function waitCommand(id: string, name = "Wait"): unknown {
+	return { id, name, type: "Wait", time: 100 };
+}
+
+/**
+ * A Macro choice whose `macro.commands` is whatever `commands` is, however wrong
+ * that is. `macro` itself is untrusted too, so passing `macro: null` builds a
+ * choice with no macro object at all - the shape that used to make "Configure"
+ * do nothing whatsoever.
+ */
+export function macroChoice(
+	name: string,
+	id: string,
+	commands: unknown,
+	options: { noMacro?: boolean } = {},
+): IChoice {
+	const node: Record<string, unknown> = {
+		id,
+		name,
+		type: "Macro",
+		command: false,
+		runOnStartup: false,
+	};
+	if (options.noMacro) {
+		node.macro = null;
+		return node as unknown as IChoice;
+	}
+	const macro: Record<string, unknown> = { id: `${id}-macro`, name };
+	// `undefined` must leave the key ABSENT (same reasoning as malformedFolder).
+	if (commands !== undefined) macro.commands = commands;
+	node.macro = macro;
+	return node as unknown as IChoice;
+}
+
+/** A Conditional command whose branches are whatever they are handed. */
+export function conditionalCommand(
+	id: string,
+	thenCommands: unknown,
+	elseCommands: unknown,
+): unknown {
+	return {
+		id,
+		name: "If",
+		type: "Conditional",
+		condition: {
+			mode: "variable",
+			variableName: "x",
+			operator: "isTruthy",
+			valueType: "string",
+		},
+		thenCommands,
+		elseCommands,
+	};
+}
+
 /**
  * One tree carrying every malformed shape plus healthy neighbours on both sides,
  * so a walker that dies partway through is caught by a missing TAIL choice and
@@ -77,6 +153,47 @@ export function malformedTree(): IChoice[] {
 			),
 		);
 
+	// The same, for macros. Every shape appears at every depth for the same
+	// reason the folder shapes do: a walker that guards `macro.commands` at its
+	// entry point but not inside a conditional's branches passes the top-level
+	// cases happily.
+	const brokenMacros = (suffix = "") => [
+		...MALFORMED_COMMANDS_SHAPES.map((shape) =>
+			macroChoice(
+				`Broken macro ${shape.key}${suffix}`,
+				`broken-macro-${shape.key}${suffix}`,
+				shape.value,
+			),
+		),
+		// A macro object that is missing entirely.
+		macroChoice(`No macro${suffix}`, `no-macro${suffix}`, undefined, {
+			noMacro: true,
+		}),
+		// A readable ARRAY that is still unrenderable: a hole, an id-less command
+		// and two commands sharing an id. Nothing here may be dropped or rewritten
+		// by a walker that is only passing through.
+		macroChoice(`Unkeyable macro${suffix}`, `unkeyable-macro${suffix}`, [
+			waitCommand("dup-cmd", "First"),
+			waitCommand("dup-cmd", "Second"),
+			{ name: "No id", type: "Wait", time: 1 },
+			null,
+			"stray",
+		]),
+		// Corruption inside a conditional's branches - one level below every guard
+		// that only looks at `macro.commands`.
+		macroChoice(`Conditional macro${suffix}`, `conditional-macro${suffix}`, [
+			conditionalCommand(
+				`cond-both${suffix}`,
+				{ "0": waitCommand(`hidden-then${suffix}`) },
+				"not a list",
+			),
+			conditionalCommand(`cond-holes${suffix}`, [null, waitCommand(`t${suffix}`)], [
+				waitCommand(`dup-branch${suffix}`),
+				waitCommand(`dup-branch${suffix}`),
+			]),
+		]),
+	];
+
 	return [
 		leaf("Head template", "head"),
 		folder("Healthy folder", "healthy", [leaf("Child note", "child")]),
@@ -85,13 +202,18 @@ export function malformedTree(): IChoice[] {
 		folder("Nesting folder", "nesting", [
 			leaf("Nested note", "nested-note"),
 			...broken("-nested"),
+			...brokenMacros("-nested"),
 			null as unknown as IChoice,
 			folder("Deep folder", "deep", [
 				leaf("Deep note", "deep-note"),
 				...broken("-deep"),
+				...brokenMacros("-deep"),
 			]),
 		]),
 		...broken(),
+		...brokenMacros(),
+		// A healthy macro, so a walker that skips the type entirely is caught too.
+		macroChoice("Healthy macro", "healthy-macro", [waitCommand("healthy-cmd")]),
 		// A hole in the list itself: `null` survives a truncated write, and it used
 		// to throw inside dedupeChoicesById during loadSettings.
 		null as unknown as IChoice,
@@ -109,25 +231,64 @@ export const HEALTHY_IDS = [
 	"nested-note",
 	"deep",
 	"deep-note",
+	"healthy-macro",
 	"tail",
 ];
 
 /**
- * Every unreadable `choices` value still in the tree, keyed by folder id, as a
- * comparable string. This is what the preservation assertions compare: an edit
- * elsewhere in the tree must not rewrite, drop, or "repair" any of them.
+ * Every unreadable `choices` / `macro.commands` / branch value still in the
+ * tree, keyed by the node that owns it, as a comparable string. This is what the
+ * preservation assertions compare: an edit elsewhere in the tree must not
+ * rewrite, drop, or "repair" any of them.
  *
  * List holes (`null`, a stray primitive) are deliberately NOT tracked. They hold
  * nothing recoverable, so a walker is free to step over one or drop it; only a
- * folder's children value can be hiding real choices.
+ * container's value can be hiding real choices or commands.
+ *
+ * Command IDS are not tracked either, for the same reason at one remove: the
+ * macro EDITOR is allowed to re-key an unkeyable command (see
+ * normalizeCommandList), and pinning ids here would forbid the very repair
+ * #1593 added. What is pinned is that no walker LOSES a command.
  */
 export function malformedSnapshot(choices: IChoice[]): string {
 	const seen: unknown[] = [];
+
+	const walkCommands = (owner: string, list: unknown) => {
+		if (!Array.isArray(list)) {
+			seen.push([owner, "commands", list]);
+			return;
+		}
+		// Arity is the invariant for a readable list: a walker may re-key an entry
+		// but must never drop one.
+		seen.push([owner, "commandCount", list.length]);
+		list.forEach((command, index) => {
+			if (typeof command !== "object" || command === null) return;
+			const node = command as Record<string, unknown>;
+			if (node.type !== "Conditional") return;
+			walkCommands(`${owner}/${index}/then`, node.thenCommands);
+			walkCommands(`${owner}/${index}/else`, node.elseCommands);
+		});
+	};
+
 	const walk = (list: unknown) => {
 		if (!Array.isArray(list)) return;
 		for (const entry of list) {
 			if (typeof entry !== "object" || entry === null) continue;
 			const node = entry as Record<string, unknown>;
+			if (node.type === "Macro") {
+				const macro = node.macro;
+				if (typeof macro !== "object" || macro === null) {
+					seen.push([node.id, "macro", macro]);
+					continue;
+				}
+				const macroNode = macro as Record<string, unknown>;
+				if ("commands" in macroNode) {
+					walkCommands(String(node.id), macroNode.commands);
+				} else {
+					seen.push([node.id, "commandsAbsent"]);
+				}
+				continue;
+			}
 			if (node.type !== "Multi") continue;
 			if (Array.isArray(node.choices)) {
 				walk(node.choices);

From b10a8ad0a93a215fae6718fbc445b4765d6081f7 Mon Sep 17 00:00:00 2001
From: Christian Bager Bach Houmann <christian@bagerbach.com>
Date: Mon, 27 Jul 2026 18:18:27 +0200
Subject: [PATCH 5/8] fix(macros): close the holes an adversarial review found
 in the fix itself

Five defects, three of them regressions this branch introduced. All reproduced
before fixing, all now covered by tests that fail without the fix.

- `macro: []` / `macro: [cmd]`: `isCommandLike` accepts arrays (typeof [] is
  "object"), so the builder opened a fully editable editor and then wrote
  `macro.commands = [...]` onto an Array - a non-index property that
  JSON.stringify drops. The user added commands, saw them, and every save
  silently discarded them. New `isMacroObject` rejects arrays; the array's
  entries now render as the commands they probably are and the first edit
  materializes a real macro object around them.

- `commands: []` is the HEALTHY default (QuickAddMacro's constructor), and the
  new length check turned it into a 15-second red notice on every run - and on
  every launch for an unpopulated run-on-startup macro. Only a MISSING list
  says anything now, matching the base behaviour and the sibling branch guard.

- `[ok, null, ok]` still threw on the `{{MACRO:Name::member}}` path:
  commandListOf converts the non-array shapes, but a hole INSIDE a real array
  sails through .map/.filter. This was the one asymmetric entrypoint - the same
  macro ran fine without member access.

- A `null` hole in a PACKAGED folder aborted the whole import. Guarding the
  preview walker earlier in this branch is what made it reachable: the package
  now previews cleanly, so the user gets to click Import and fail there. Fixed
  on both the import and export sides.

- A conditional's branch rendered "Then: 10" for a branch saved as "not a
  list" (its character count) and "Then: 0" for an array-turned-object.

Refs #1593
---
 .../MacroChoiceEngine.malformed.test.ts       | 156 +++++++++++++++++
 src/engine/MacroChoiceEngine.ts               |  19 ++-
 .../SingleMacroEngine.member-access.test.ts   |  83 +++++++++
 src/engine/SingleMacroEngine.ts               |  15 +-
 .../Components/ConditionalCommand.svelte      |   9 +-
 .../MacroGUIs/MacroBuilder.malformed.test.ts  | 161 ++++++++++++++++++
 src/gui/MacroGUIs/MacroBuilder.ts             |  30 ++--
 src/services/packageExportService.ts          |   5 +-
 src/services/packageImportService.ts          |  11 +-
 src/utils/macroUtils.test.ts                  |  37 ++++
 src/utils/macroUtils.ts                       |  27 +++
 11 files changed, 530 insertions(+), 23 deletions(-)
 create mode 100644 src/engine/MacroChoiceEngine.malformed.test.ts
 create mode 100644 src/gui/MacroGUIs/MacroBuilder.malformed.test.ts

diff --git a/src/engine/MacroChoiceEngine.malformed.test.ts b/src/engine/MacroChoiceEngine.malformed.test.ts
new file mode 100644
index 000000000..f0a8422ad
--- /dev/null
+++ b/src/engine/MacroChoiceEngine.malformed.test.ts
@@ -0,0 +1,156 @@
+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],
+	])("says it could not read %s, instead of throwing or lying", 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.
+		await expect(
+			runMacro({ id: "m", name: "M", commands }),
+		).resolves.toBeUndefined();
+
+		expect(errors).toHaveLength(1);
+		expect(errors[0]).toContain("Could not read the commands for macro");
+		expect(errors[0]).toContain("Test choice");
+		expect(errors[0]).toContain("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("says it could not read the branch instead of skipping it silently", async () => {
+		await runMacro({
+			id: "m",
+			name: "M",
+			commands: [conditional([], { "0": wait("hidden") })],
+		});
+
+		expect(errors).toHaveLength(1);
+		expect(errors[0]).toContain("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([]);
+	});
+});
diff --git a/src/engine/MacroChoiceEngine.ts b/src/engine/MacroChoiceEngine.ts
index 1d8e51998..a2496d31b 100644
--- a/src/engine/MacroChoiceEngine.ts
+++ b/src/engine/MacroChoiceEngine.ts
@@ -59,7 +59,11 @@ import { evaluateCondition } from "./helpers/conditionalEvaluator";
 import { handleMacroAbort } from "../utils/macroAbortHandler";
 import { buildOpenFileOptions } from "./helpers/openFileOptions";
 import { createVariablesProxy } from "../utils/variablesProxy";
-import { commandListOf, isUnreadableCommandList } from "../utils/macroUtils";
+import {
+	commandListOf,
+	hasCommandList,
+	isUnreadableCommandList,
+} from "../utils/macroUtils";
 
 type ConditionalScriptRunner = () => Promise<unknown>;
 type UserScriptFunction = (
@@ -278,9 +282,16 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 
 		const commands = commandListOf(this.macro?.commands);
 		if (commands.length === 0) {
-			log.logError(
-				`No commands in the macro for choice '${this.choice.name}'`
-			);
+			// `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;
 		}
 
diff --git a/src/engine/SingleMacroEngine.member-access.test.ts b/src/engine/SingleMacroEngine.member-access.test.ts
index cd0669258..c8ded6369 100644
--- a/src/engine/SingleMacroEngine.member-access.test.ts
+++ b/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]);
diff --git a/src/engine/SingleMacroEngine.ts b/src/engine/SingleMacroEngine.ts
index 36a2f5455..a8c408f1a 100644
--- a/src/engine/SingleMacroEngine.ts
+++ b/src/engine/SingleMacroEngine.ts
@@ -15,7 +15,11 @@ import {
 } from "../utils/userScriptSecrets";
 import { MacroChoiceEngine } from "./MacroChoiceEngine";
 import { handleMacroAbort } from "../utils/macroAbortHandler";
-import { commandListOf, hasCommandList } from "../utils/macroUtils";
+import {
+	commandListOf,
+	hasCommandList,
+	isCommandLike,
+} from "../utils/macroUtils";
 import { MacroAbortError } from "../errors/MacroAbortError";
 
 // Member names that QuickAdd itself treats as conventions/metadata rather than entrypoints:
@@ -218,6 +222,12 @@ export class SingleMacroEngine {
 			.map((command, index) => ({ command, index }))
 			.filter(
 				(entry): entry is { command: IUserScript; index: number } =>
+					// isCommandLike, not just the type check: commandListOf converts the
+					// NON-ARRAY shapes, but a `null` hole INSIDE a real array sails
+					// straight through .map/.filter. Without this, `[ok, null, ok]` threw
+					// here on the `{{MACRO:Name::member}}` path while the same macro ran
+					// fine without member access (#1593).
+					isCommandLike(entry.command) &&
 					entry.command.type === CommandType.UserScript,
 			);
 
@@ -257,6 +267,9 @@ export class SingleMacroEngine {
 			if (candidateId !== undefined) {
 				refreshedIndex = updatedCommands.findIndex(
 					(command) =>
+						// A pre-command can rewrite `commands` to include a hole; the
+						// hasCommandList guard above only rejects a non-array refresh.
+						isCommandLike(command) &&
 						command.id === candidateId &&
 						command.type === CommandType.UserScript,
 				);
diff --git a/src/gui/MacroGUIs/Components/ConditionalCommand.svelte b/src/gui/MacroGUIs/Components/ConditionalCommand.svelte
index 0cb89f5f0..b11a029eb 100644
--- a/src/gui/MacroGUIs/Components/ConditionalCommand.svelte
+++ b/src/gui/MacroGUIs/Components/ConditionalCommand.svelte
@@ -3,6 +3,7 @@
 	import DragHandle from "../../components/DragHandle.svelte";
 	import type { IConditionalCommand } from "../../../types/macros/Conditional/IConditionalCommand";
 	import { getConditionSummary } from "../../../utils/conditionalHelpers";
+	import { commandListOf } from "../../../utils/macroUtils";
 
 	let {
 		command,
@@ -27,8 +28,12 @@
 	} = $props();
 
 	const summary = $derived(getConditionSummary(command.condition));
-	const thenCount = $derived(command.thenCommands?.length ?? 0);
-	const elseCount = $derived(command.elseCommands?.length ?? 0);
+	// `?.length ?? 0` reads a LENGTH off whatever is there: a branch saved as
+	// "not a list" rendered "Then: 10" (its character count) and an
+	// array-turned-object rendered "Then: 0" as if the branch were empty. Count
+	// only what is actually a list of commands (#1593).
+	const thenCount = $derived(commandListOf(command.thenCommands).length);
+	const elseCount = $derived(commandListOf(command.elseCommands).length);
 </script>
 
 <li class="quickAddCommandListItem conditionalCommand">
diff --git a/src/gui/MacroGUIs/MacroBuilder.malformed.test.ts b/src/gui/MacroGUIs/MacroBuilder.malformed.test.ts
new file mode 100644
index 000000000..783158f37
--- /dev/null
+++ b/src/gui/MacroGUIs/MacroBuilder.malformed.test.ts
@@ -0,0 +1,161 @@
+import { afterEach, describe, expect, it, vi } from "vitest";
+
+vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() }));
+
+import { App } from "obsidian";
+import type QuickAdd from "../../main";
+import type IMacroChoice from "../../types/choices/IMacroChoice";
+import { MacroBuilder } from "./MacroBuilder";
+
+function testApp(): App {
+	const app = new App() as App & {
+		dom: { appContainerEl: HTMLElement };
+		keymap: { pushScope: () => void; popScope: () => void };
+	};
+	app.dom = { appContainerEl: document.body };
+	app.keymap = { pushScope: vi.fn(), popScope: vi.fn() };
+	return app;
+}
+
+/** The REAL CommandSequenceEditor, unlike MacroBuilder.test.ts which mocks it. */
+function openBuilder(macro: unknown) {
+	const choice = {
+		id: "c1",
+		name: "Macro under test",
+		type: "Macro",
+		command: false,
+		runOnStartup: false,
+		macro,
+	} as unknown as IMacroChoice;
+
+	const modal = new MacroBuilder(
+		testApp(),
+		{ settings: { choices: [] } } as unknown as QuickAdd,
+		choice,
+		[],
+	);
+	return { modal, choice, el: modal.contentEl };
+}
+
+const addControls = (el: HTMLElement) => el.querySelectorAll("button").length;
+const rows = (el: HTMLElement) =>
+	el.querySelectorAll(".quickAddCommandList > li").length;
+
+/**
+ * `choice.macro` is as untrusted as `macro.commands` (#1593). The shape that
+ * mattered most was `macro: null`: `display()` runs from the CONSTRUCTOR, before
+ * `open()`, so a throw there took the modal with it and clicking "Configure" did
+ * nothing whatsoever - no card, no notice, nothing.
+ */
+describe("MacroBuilder over a malformed macro object (#1593)", () => {
+	afterEach(() => {
+		document.body.replaceChildren();
+	});
+
+	it.each([
+		["null", null],
+		["undefined", undefined],
+		["an empty object", {}],
+	])("opens a usable, empty editor when macro is %s", (_label, macro) => {
+		const { el } = openBuilder(macro);
+
+		expect(el.querySelector(".qaDataUnreadable")).toBeNull();
+		expect(el.querySelector(".qaMountFailed")).toBeNull();
+		expect(addControls(el)).toBeGreaterThan(0);
+		expect(rows(el)).toBe(0);
+		// The rest of the modal is there too.
+		expect(el.textContent).toContain("Run on startup");
+	});
+
+	it("materializes a real macro object on the first edit, and it survives JSON", () => {
+		const { modal, choice } = openBuilder(null);
+
+		// What an "Add …" click does.
+		(modal as unknown as { setMacroCommands(c: unknown[]): void }).setMacroCommands([
+			{ id: "new-1", name: "Wait", type: "Wait", time: 100 },
+		]);
+
+		const persisted = JSON.parse(JSON.stringify(choice)) as IMacroChoice;
+		expect(persisted.macro.commands).toHaveLength(1);
+		expect(persisted.macro.commands[0].id).toBe("new-1");
+		expect(persisted.macro.name).toBe("Macro under test");
+		expect(typeof persisted.macro.id).toBe("string");
+	});
+
+	// An ARRAY passes `typeof === "object"`, so a looser guard let the builder
+	// treat it as a macro object and write `macro.commands = [...]` onto it - a
+	// non-index property that JSON.stringify drops. The user saw their commands
+	// and every save silently discarded them.
+	describe("an array-valued macro", () => {
+		it("renders the array's entries as the commands they probably are", () => {
+			const { el } = openBuilder([
+				{ id: "a", name: "Alpha", type: "Wait", time: 1 },
+				{ id: "b", name: "Beta", type: "Wait", time: 2 },
+			]);
+
+			expect(rows(el)).toBe(2);
+			expect(addControls(el)).toBeGreaterThan(0);
+		});
+
+		it("survives the JSON round-trip after an edit, losing nothing", () => {
+			const { modal, choice } = openBuilder([
+				{ id: "a", name: "Alpha", type: "Wait", time: 1 },
+			]);
+
+			(modal as unknown as { setMacroCommands(c: unknown[]): void }).setMacroCommands([
+				{ id: "a", name: "Alpha", type: "Wait", time: 1 },
+				{ id: "b", name: "Beta", type: "Wait", time: 2 },
+			]);
+
+			const persisted = JSON.parse(JSON.stringify(choice)) as IMacroChoice;
+			expect(persisted.macro.commands.map((c) => c.id)).toEqual(["a", "b"]);
+		});
+
+		it("treats an empty array as carrying nothing", () => {
+			const { modal, choice, el } = openBuilder([]);
+
+			expect(rows(el)).toBe(0);
+			expect(addControls(el)).toBeGreaterThan(0);
+
+			(modal as unknown as { setMacroCommands(c: unknown[]): void }).setMacroCommands([
+				{ id: "x", name: "Wait", type: "Wait", time: 1 },
+			]);
+			const persisted = JSON.parse(JSON.stringify(choice)) as IMacroChoice;
+			expect(persisted.macro.commands).toHaveLength(1);
+		});
+	});
+
+	it.each([
+		["a string", "not a macro"],
+		["a number", 7],
+	])("shows the unreadable card and no edit controls for %s", (_label, macro) => {
+		const { el } = openBuilder(macro);
+
+		expect(el.querySelector(".qaDataUnreadable")?.textContent).toContain(
+			"QuickAdd couldn't read this macro's commands",
+		);
+		expect(rows(el)).toBe(0);
+		// Only the rename button in the header; nothing that writes commands.
+		expect(el.querySelector(".quickCommandContainer")).toBeNull();
+	});
+
+	it("leaves an unreadable macro byte-identical after open and close", () => {
+		const macro = "not a macro";
+		const { modal, choice } = openBuilder(macro);
+		const before = JSON.stringify(choice);
+
+		modal.onClose();
+
+		expect(JSON.stringify(choice)).toBe(before);
+	});
+
+	it("still renames the choice when the macro object is missing", () => {
+		const { modal, choice } = openBuilder(null);
+		const button = modal.contentEl.querySelector<HTMLButtonElement>(
+			".qa-rename-title-button",
+		);
+
+		expect(button).not.toBeNull();
+		expect(choice.name).toBe("Macro under test");
+	});
+});
diff --git a/src/gui/MacroGUIs/MacroBuilder.ts b/src/gui/MacroGUIs/MacroBuilder.ts
index b9cc10489..fdf7b5b6c 100644
--- a/src/gui/MacroGUIs/MacroBuilder.ts
+++ b/src/gui/MacroGUIs/MacroBuilder.ts
@@ -19,10 +19,7 @@ import {
 	isChoiceLike,
 	rootChoicesOf,
 } from "../../utils/choiceUtils";
-import {
-	isCommandLike,
-	isUnreadableCommandList,
-} from "../../utils/macroUtils";
+import { isMacroObject, isUnreadableMacro } from "../../utils/macroUtils";
 import type { ICommand } from "../../types/macros/ICommand";
 import { v4 as uuidv4 } from "uuid";
 
@@ -118,7 +115,7 @@ export class MacroBuilder extends Modal {
 					// be missing from a hand-edited data.json; renaming the choice
 					// still has to work, so only sync a macro that is there.
 					this.choice.name = newName;
-					if (isCommandLike(this.macro)) this.macro.name = newName;
+					if (isMacroObject(this.macro)) this.macro.name = newName;
 					this.reload();
 				} catch {
 					// Prompt cancelled (Esc/Cancel) — keep the current name.
@@ -157,14 +154,23 @@ export class MacroBuilder extends Modal {
 	 * `choice.macro` is untrusted too, and a Macro choice whose `macro` key is
 	 * missing entirely used to make "Configure" do nothing at all: `display()`
 	 * runs from the constructor, before `open()`, so the throw took the modal with
-	 * it. When the value carries nothing there is nothing to lose and the editor
-	 * opens empty; when it is something we cannot read, it is passed straight
-	 * through so the editor shows its unreadable card rather than offering to
-	 * overwrite it.
+	 * it.
+	 *
+	 * Three cases, and `macro` being an ARRAY is the one worth naming: `[]` and
+	 * `[{...}]` are both objects, but writing `macro.commands` onto an Array sets
+	 * a non-index property that `JSON.stringify` drops, so the user's edits would
+	 * vanish on every save while the editor happily showed them. Handing the array
+	 * itself over as the command list instead means its entries render as the
+	 * commands they probably are, and `setMacroCommands` materializes a real macro
+	 * object around them on the first edit - nothing lost either way.
 	 */
 	private macroCommandsValue(): unknown {
-		if (isCommandLike(this.macro)) return this.macro.commands;
-		return isUnreadableCommandList(this.macro) ? this.macro : undefined;
+		if (isMacroObject(this.macro)) return this.macro.commands;
+		// Not a macro object. Hand the value itself over when it could be carrying
+		// something (an array renders as commands; a string gets the unreadable
+		// card); otherwise nothing, so the editor opens empty and the first edit
+		// materializes a real macro below.
+		return isUnreadableMacro(this.macro) ? this.macro : undefined;
 	}
 
 	/**
@@ -173,7 +179,7 @@ export class MacroBuilder extends Modal {
 	 * `macroCommandsValue` guarantees means nothing readable is being replaced.
 	 */
 	private setMacroCommands(commands: ICommand[]) {
-		if (!isCommandLike(this.macro)) {
+		if (!isMacroObject(this.macro)) {
 			this.macro = { id: uuidv4(), name: this.choice.name, commands };
 			this.choice.macro = this.macro;
 			return;
diff --git a/src/services/packageExportService.ts b/src/services/packageExportService.ts
index 341a62cab..26c6fd6a6 100644
--- a/src/services/packageExportService.ts
+++ b/src/services/packageExportService.ts
@@ -18,6 +18,7 @@ import { log } from "../logger/logManager";
 import GenericYesNoPrompt from "../gui/GenericYesNoPrompt/GenericYesNoPrompt";
 import { decodeFromBase64, encodeToBase64 } from "../utils/base64";
 import { deepClone } from "../utils/deepClone";
+import { isChoiceLike } from "../utils/choiceUtils";
 import { ensureParentFolders } from "../utils/ensureParentFolders";
 import {
 	detectUserScriptSecretOptions,
@@ -232,7 +233,9 @@ function pruneChoiceTree(
 	}
 
 	multi.choices = multi.choices
-		.filter((child) => includedIds.has(child.id))
+		// Same hole as the import side: a `null` in the folder's list is not a
+		// choice and must not be dereferenced (#1566).
+		.filter((child) => isChoiceLike(child) && includedIds.has(child.id))
 		.map((child) => {
 			pruneChoiceTree(child, includedIds);
 			return child;
diff --git a/src/services/packageImportService.ts b/src/services/packageImportService.ts
index dc6cc5308..ffbe02885 100644
--- a/src/services/packageImportService.ts
+++ b/src/services/packageImportService.ts
@@ -904,8 +904,10 @@ function remapChoiceTree(
 
 	if (choice.type === "Macro") {
 		const macroChoice = choice as IMacroChoice;
-		// `macro` is untrusted too: an imported package can omit it entirely.
-		if (isDuplicated && macroChoice.macro) {
+		// `macro` is untrusted too: an imported package can omit it entirely, or
+		// carry a primitive where the object belongs (truthiness would let `"x"`
+		// through and then throw on the assignment).
+		if (isDuplicated && isCommandLike(macroChoice.macro)) {
 			macroChoice.macro.id = uuidv4();
 		}
 		remapCommands(
@@ -921,7 +923,10 @@ function remapChoiceTree(
 		const multi = choice as IMultiChoice;
 		if (Array.isArray(multi.choices)) {
 			multi.choices = multi.choices
-				.filter((child) => importableChoiceIds.has(child.id))
+				// A packaged folder's list can hold a `null` hole like any other
+				// (#1566); dereferencing it here aborted the whole import, taking
+				// healthy siblings with it.
+				.filter((child) => isChoiceLike(child) && importableChoiceIds.has(child.id))
 				.map((child) =>
 					remapChoiceTree(
 						child,
diff --git a/src/utils/macroUtils.test.ts b/src/utils/macroUtils.test.ts
index a41bef40b..e9d1ff51b 100644
--- a/src/utils/macroUtils.test.ts
+++ b/src/utils/macroUtils.test.ts
@@ -6,7 +6,9 @@ import {
 	commandListOf,
 	hasCommandList,
 	isCommandLike,
+	isMacroObject,
 	isUnreadableCommandList,
+	isUnreadableMacro,
 	normalizeCommandList,
 	regenerateIds,
 } from "./macroUtils";
@@ -210,3 +212,38 @@ describe("regenerateIds", () => {
 		expect(() => regenerateIds("x" as unknown as IMacro)).not.toThrow();
 	});
 });
+
+describe("isMacroObject / isUnreadableMacro", () => {
+	// An ARRAY is `typeof === "object"`, so the looser isCommandLike accepts one.
+	// That mattered: writing `macro.commands = [...]` onto an Array sets a
+	// non-index property, which JSON.stringify (i.e. saveData) silently drops -
+	// so the editor showed the user's new commands and every save discarded them.
+	it("rejects arrays, which cannot carry a macro's fields through JSON", () => {
+		expect(isMacroObject({})).toBe(true);
+		expect(isMacroObject({ id: "m", name: "M", commands: [] })).toBe(true);
+		expect(isMacroObject([])).toBe(false);
+		expect(isMacroObject([wait("a")])).toBe(false);
+		expect(isMacroObject(null)).toBe(false);
+		expect(isMacroObject("x")).toBe(false);
+	});
+
+	it("proves the JSON hazard the predicate exists for", () => {
+		const asArray: unknown = [];
+		(asArray as Record<string, unknown>).commands = [wait("new")];
+		expect(JSON.parse(JSON.stringify(asArray))).toEqual([]);
+	});
+
+	it.each([
+		["a macro object", {}, false],
+		["a populated macro object", { id: "m", commands: [] }, false],
+		["undefined", undefined, false],
+		["null", null, false],
+		["an empty array", [], false],
+		["an empty string", "", false],
+		["a populated array", [wait("a")], true],
+		["a string", "not a macro", true],
+		["a number", 7, true],
+	])("says whether %s could be carrying a macro", (_label, value, expected) => {
+		expect(isUnreadableMacro(value)).toBe(expected);
+	});
+});
diff --git a/src/utils/macroUtils.ts b/src/utils/macroUtils.ts
index ee3f5ddac..226bb71e1 100644
--- a/src/utils/macroUtils.ts
+++ b/src/utils/macroUtils.ts
@@ -13,6 +13,33 @@ export function isCommandLike(value: unknown): value is ICommand {
 	return typeof value === "object" && value !== null;
 }
 
+/**
+ * Whether `value` is a macro OBJECT we can read and write through.
+ *
+ * `isCommandLike` is not enough here, because `typeof [] === "object"`: an
+ * array-valued `macro` would pass it, and `macro.commands = [...]` on an Array
+ * writes a non-index property that `JSON.stringify` (i.e. `saveData`) and
+ * `$state.snapshot` both discard - so the user's edits would vanish on every
+ * save while the editor showed them happily.
+ */
+export function isMacroObject(
+	value: unknown,
+): value is Record<string, unknown> {
+	return isCommandLike(value) && !Array.isArray(value);
+}
+
+/**
+ * Whether a `macro` value is something we cannot read that could still be
+ * carrying a macro. Same "degrade quietly vs tell the user" line as
+ * {@link isUnreadableCommandList}, one level up: an empty array carries nothing
+ * (so the editor may replace it), a non-empty one might.
+ */
+export function isUnreadableMacro(value: unknown): boolean {
+	if (isMacroObject(value)) return false;
+	if (Array.isArray(value)) return value.length > 0;
+	return isUnreadableCommandList(value);
+}
+
 /**
  * A command list that is always safe to iterate, map or spread.
  *

From cc78a2c93d1e190beee991b3040f29642ba0eefb Mon Sep 17 00:00:00 2001
From: Christian Bager Bach Houmann <christian@bagerbach.com>
Date: Mon, 27 Jul 2026 18:33:27 +0200
Subject: [PATCH 6/8] fix(macros): an unreadable macro fails the run instead of
 reporting success

Review follow-up on #1616.

- run() and executeConditional now THROW for a command list they could not
  read, rather than logging and returning. Returning let `quickadd:run` answer
  ok:true for a macro that ran nothing, so automation carried on as if it had
  worked - and in the conditional case the outer loop went on to run every
  command AFTER the branch, including file writes that were only ever meant to
  follow a branch that never ran.

- packageImportService used the looser isCommandLike for the `macro` object, so
  `macro.id = uuidv4()` on an array-valued macro was a silent no-op (JSON drops
  non-index properties) and the duplicate kept the original's id.

- remapCommands and applyOverridesToCommands read `macro?.commands`, which is
  undefined for an array-valued macro - so the choice references and secret
  refs its entries hold were never remapped, and a duplicated choice could
  still invoke the pre-existing local one. Both now go through the new shared
  macroCommandsValueOf, which is also what MacroBuilder's recovery path reads,
  so the two cannot disagree about where a macro's commands live.

Refs #1593
---
 .../MacroChoiceEngine.malformed.test.ts       | 45 +++++++++++++------
 src/engine/MacroChoiceEngine.ts               | 20 ++++++---
 src/gui/MacroGUIs/MacroBuilder.ts             | 12 +++--
 src/services/packageImportService.ts          | 26 ++++++++---
 src/utils/macroUtils.ts                       | 19 ++++++++
 5 files changed, 90 insertions(+), 32 deletions(-)

diff --git a/src/engine/MacroChoiceEngine.malformed.test.ts b/src/engine/MacroChoiceEngine.malformed.test.ts
index f0a8422ad..23c06e305 100644
--- a/src/engine/MacroChoiceEngine.malformed.test.ts
+++ b/src/engine/MacroChoiceEngine.malformed.test.ts
@@ -62,18 +62,22 @@ describe("MacroChoiceEngine.run over a malformed command list (#1593)", () => {
 		["an array-turned-object", { "0": wait("hidden") }],
 		["a string", "not a list"],
 		["a number", 7],
-	])("says it could not read %s, instead of throwing or lying", async (_l, commands) => {
+	])("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 }),
-		).resolves.toBeUndefined();
+		).rejects.toThrow(/Could not read the commands for macro 'Test choice'/);
+	});
 
-		expect(errors).toHaveLength(1);
-		expect(errors[0]).toContain("Could not read the commands for macro");
-		expect(errors[0]).toContain("Test choice");
-		expect(errors[0]).toContain("data.json");
+	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
@@ -129,15 +133,28 @@ describe("MacroChoiceEngine conditional branches over a malformed value (#1593)"
 		elseCommands,
 	});
 
-	it("says it could not read the branch instead of skipping it silently", async () => {
-		await runMacro({
-			id: "m",
-			name: "M",
-			commands: [conditional([], { "0": wait("hidden") })],
-		});
+	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/);
+	});
 
-		expect(errors).toHaveLength(1);
-		expect(errors[0]).toContain("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.
diff --git a/src/engine/MacroChoiceEngine.ts b/src/engine/MacroChoiceEngine.ts
index a2496d31b..177bf5c43 100644
--- a/src/engine/MacroChoiceEngine.ts
+++ b/src/engine/MacroChoiceEngine.ts
@@ -273,11 +273,16 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 		// 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)) {
-			log.logError(
+			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.`
 			);
-			return;
 		}
 
 		const commands = commandListOf(this.macro?.commands);
@@ -783,11 +788,16 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 		// 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)) {
-			log.logError(
-				`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.`
+			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.`
 			);
-			return;
 		}
 
 		if (!Array.isArray(branch) || branch.length === 0) {
diff --git a/src/gui/MacroGUIs/MacroBuilder.ts b/src/gui/MacroGUIs/MacroBuilder.ts
index fdf7b5b6c..5d236251e 100644
--- a/src/gui/MacroGUIs/MacroBuilder.ts
+++ b/src/gui/MacroGUIs/MacroBuilder.ts
@@ -19,7 +19,10 @@ import {
 	isChoiceLike,
 	rootChoicesOf,
 } from "../../utils/choiceUtils";
-import { isMacroObject, isUnreadableMacro } from "../../utils/macroUtils";
+import {
+	isMacroObject,
+	macroCommandsValueOf,
+} from "../../utils/macroUtils";
 import type { ICommand } from "../../types/macros/ICommand";
 import { v4 as uuidv4 } from "uuid";
 
@@ -165,12 +168,7 @@ export class MacroBuilder extends Modal {
 	 * object around them on the first edit - nothing lost either way.
 	 */
 	private macroCommandsValue(): unknown {
-		if (isMacroObject(this.macro)) return this.macro.commands;
-		// Not a macro object. Hand the value itself over when it could be carrying
-		// something (an array renders as commands; a string gets the unreadable
-		// card); otherwise nothing, so the editor opens empty and the first edit
-		// materializes a real macro below.
-		return isUnreadableMacro(this.macro) ? this.macro : undefined;
+		return macroCommandsValueOf(this.macro);
 	}
 
 	/**
diff --git a/src/services/packageImportService.ts b/src/services/packageImportService.ts
index ffbe02885..9c201ae85 100644
--- a/src/services/packageImportService.ts
+++ b/src/services/packageImportService.ts
@@ -17,7 +17,12 @@ import {
 	hasUnreadableChildren,
 	isChoiceLike,
 } from "../utils/choiceUtils";
-import { commandListOf, isCommandLike } from "../utils/macroUtils";
+import {
+	commandListOf,
+	isCommandLike,
+	isMacroObject,
+	macroCommandsValueOf,
+} from "../utils/macroUtils";
 import type { ICommand } from "../types/macros/ICommand";
 import type { IChoiceCommand } from "../types/macros/IChoiceCommand";
 import type { IConditionalCommand } from "../types/macros/Conditional/IConditionalCommand";
@@ -905,13 +910,19 @@ function remapChoiceTree(
 	if (choice.type === "Macro") {
 		const macroChoice = choice as IMacroChoice;
 		// `macro` is untrusted too: an imported package can omit it entirely, or
-		// carry a primitive where the object belongs (truthiness would let `"x"`
-		// through and then throw on the assignment).
-		if (isDuplicated && isCommandLike(macroChoice.macro)) {
+		// carry a primitive where the object belongs. isMacroObject, not the
+		// looser isCommandLike: an Array passes `typeof === "object"`, and
+		// `macro.id = ...` on one sets a non-index property that JSON.stringify
+		// drops - a silent no-op that would leave the duplicate sharing an id.
+		if (isDuplicated && isMacroObject(macroChoice.macro)) {
 			macroChoice.macro.id = uuidv4();
 		}
+		// macroCommandsValueOf, not `macro?.commands`: an array-valued `macro` IS
+		// the command list (see MacroBuilder's recovery path), and reading
+		// `.commands` off it would silently skip remapping every choice reference
+		// and secret ref it holds.
 		remapCommands(
-			macroChoice.macro?.commands,
+			macroCommandsValueOf(macroChoice.macro),
 			idMap,
 			importableChoiceIds,
 			isDuplicated,
@@ -1086,7 +1097,10 @@ function applyAssetPathOverrides(
 	switch (choice.type) {
 		case "Macro": {
 			const macroChoice = choice as IMacroChoice;
-			applyOverridesToCommands(macroChoice.macro?.commands, pathOverrides);
+			applyOverridesToCommands(
+				macroCommandsValueOf(macroChoice.macro),
+				pathOverrides,
+			);
 			break;
 		}
 		case "Template": {
diff --git a/src/utils/macroUtils.ts b/src/utils/macroUtils.ts
index 226bb71e1..81cd5cc46 100644
--- a/src/utils/macroUtils.ts
+++ b/src/utils/macroUtils.ts
@@ -40,6 +40,25 @@ export function isUnreadableMacro(value: unknown): boolean {
 	return isUnreadableCommandList(value);
 }
 
+/**
+ * Where a macro's command list actually lives, given an untrusted `macro`.
+ *
+ * Normally `macro.commands`. But an ARRAY-valued `macro` is treated as the
+ * command list itself: writing `macro.commands` onto an Array is dropped by
+ * `JSON.stringify`, so the recoverable reading is that the array IS the
+ * commands (someone saved `macro: commands`). Every consumer has to agree on
+ * that, or the macro builder repairs one list while the package importer
+ * remaps another - hence one function rather than a convention.
+ *
+ * Returns `undefined` for a value that carries nothing, so the caller can tell
+ * "no commands" from "commands we could not read" via
+ * {@link isUnreadableCommandList} on the result.
+ */
+export function macroCommandsValueOf(macro: unknown): unknown {
+	if (isMacroObject(macro)) return macro.commands;
+	return isUnreadableMacro(macro) ? macro : undefined;
+}
+
 /**
  * A command list that is always safe to iterate, map or spread.
  *

From 62cca544e587b668411c9e2f07b79553098e60cf Mon Sep 17 00:00:00 2001
From: Christian Bager Bach Houmann <christian@bagerbach.com>
Date: Mon, 27 Jul 2026 18:39:27 +0200
Subject: [PATCH 7/8] test(macros): type the package fixture properly instead
 of casting past it

`packageOf` set `entryChoiceIds` where QuickAddPackage declares
`rootChoiceIds`, and an `as unknown as` hid the mismatch. It happens not to
matter today because walkPackage derives its entry ids from `pkg.choices`, but
a fixture that does not actually satisfy the shape the walker is handed in
production is exactly the drift this sweep exists to catch. Structurally typed
now, with no cast.

Refs #1593
---
 .../malformedChoices.entrypoints.test.ts      | 27 +++++++++++++------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/src/utils/malformedChoices.entrypoints.test.ts b/src/utils/malformedChoices.entrypoints.test.ts
index b7240d30d..cec171198 100644
--- a/src/utils/malformedChoices.entrypoints.test.ts
+++ b/src/utils/malformedChoices.entrypoints.test.ts
@@ -48,7 +48,10 @@ import {
 } from "./macroUtils";
 import type { IMacro } from "../types/macros/IMacro";
 import { buildPackagePreview } from "../services/packagePreview";
-import type { QuickAddPackage } from "../types/packages/QuickAddPackage";
+import {
+	QUICKADD_PACKAGE_SCHEMA_VERSION,
+	type QuickAddPackage,
+} from "../types/packages/QuickAddPackage";
 import {
 	HEALTHY_IDS,
 	leaf,
@@ -177,18 +180,26 @@ const sweeps: Sweep[] = [
 	],
 ];
 
-/** The malformed tree wrapped as an importable package, for the preview walker. */
+/**
+ * The malformed tree wrapped as an importable package, for the preview walker.
+ * Structurally typed, with NO cast: a cast here would let the fixture drift out
+ * of the shape the walker is handed in production, which is the one thing this
+ * sweep is for.
+ */
 function packageOf(tree: IChoice[]): QuickAddPackage {
+	const roots = tree.filter(isChoiceLike);
 	return {
-		schemaVersion: 1,
+		schemaVersion: QUICKADD_PACKAGE_SCHEMA_VERSION,
 		quickAddVersion: "1.18.0",
 		createdAt: "2026-06-01T00:00:00.000Z",
-		entryChoiceIds: tree.filter(isChoiceLike).map((c) => c.id),
-		choices: tree
-			.filter(isChoiceLike)
-			.map((choice) => ({ choice, pathHint: [choice.name], parentChoiceId: null })),
+		rootChoiceIds: roots.map((c) => c.id),
+		choices: roots.map((choice) => ({
+			choice,
+			pathHint: [choice.name],
+			parentChoiceId: null,
+		})),
 		assets: [],
-	} as unknown as QuickAddPackage;
+	};
 }
 
 /** Every `macro` object in the tree, including the unreadable ones. */

From 3c50e3229f1aaa0f439dec8c7aa6aca336016572 Mon Sep 17 00:00:00 2001
From: Christian Bager Bach Houmann <christian@bagerbach.com>
Date: Mon, 27 Jul 2026 18:46:37 +0200
Subject: [PATCH 8/8] test(packages): cover the macro and folder recovery paths
 through a real import

Drives applyPackageImport end-to-end over every malformed `macro` shape, and
pins the two behaviours the last round changed:

- an array-valued macro has its choice references remapped, so a duplicated
  choice's reference follows the imported copy instead of resolving to the
  pre-existing local choice
- a `null` hole in a packaged folder's children no longer aborts the whole
  import

Both fail without the fix (verified by reverting each guard in turn).

Refs #1593
---
 .../packageImportService.malformed.test.ts    | 155 ++++++++++++++++++
 1 file changed, 155 insertions(+)

diff --git a/src/services/packageImportService.malformed.test.ts b/src/services/packageImportService.malformed.test.ts
index d26206242..2f8be67cf 100644
--- a/src/services/packageImportService.malformed.test.ts
+++ b/src/services/packageImportService.malformed.test.ts
@@ -111,3 +111,158 @@ describe("package import over a malformed tree (#1566)", () => {
 		expect(find(result.updatedChoices, "imported-1")).toBeDefined();
 	});
 });
+
+/**
+ * #1593. The importer's macro writers dereferenced `macro` / `macro.commands`
+ * raw, and a PACKAGED folder's children list was dereferenced raw too. The
+ * second one only became reachable when this change guarded the preview walker:
+ * before that the package died at preview, so the Import button never lit.
+ */
+const importPackaged = async (
+	choices: IChoice[],
+	mode: "import" | "duplicate" = "import",
+	existing: IChoice[] = [],
+) =>
+	applyPackageImport({
+		app,
+		existingChoices: existing,
+		pkg: {
+			schemaVersion: QUICKADD_PACKAGE_SCHEMA_VERSION,
+			name: "Fixture",
+			description: "",
+			choices: choices.map((choice) => ({
+				choice,
+				parentChoiceId: null,
+				pathHint: [choice.name],
+			})),
+			assets: [],
+		} as never,
+		choiceDecisions: choices.map((c) => ({ choiceId: c.id, mode })),
+		assetDecisions: [],
+	});
+
+const macroChoice = (id: string, macro: unknown): IChoice =>
+	({
+		id,
+		name: `Macro ${id}`,
+		type: "Macro",
+		command: false,
+		runOnStartup: false,
+		macro,
+	}) as unknown as IChoice;
+
+const choiceCommand = (id: string, choiceId: string) => ({
+	id,
+	name: "Run choice",
+	type: "Choice",
+	choiceId,
+});
+
+describe("package import over a malformed macro (#1593)", () => {
+	it.each([
+		["null", null],
+		["a string", "not a macro"],
+		["an array-turned-object", { "0": {} }],
+		["commands as a string", { id: "m", name: "M", commands: "not a list" }],
+		["commands with a hole", { id: "m", name: "M", commands: [null] }],
+	])("imports a macro whose macro value is %s without throwing", async (_l, macro) => {
+		const result = await importPackaged([macroChoice("m1", macro)]);
+		expect(find(result.updatedChoices, "m1")).toBeDefined();
+	});
+
+	it.each([
+		["null", null],
+		["a string", "not a macro"],
+	])("leaves a %s macro value exactly as it found it", async (_l, macro) => {
+		const result = await importPackaged([macroChoice("m1", macro)]);
+		const imported = find(result.updatedChoices, "m1") as unknown as {
+			macro: unknown;
+		};
+		expect(imported.macro).toEqual(macro);
+	});
+
+	// An array-valued `macro` IS the command list (MacroBuilder's recovery path),
+	// so its entries have to be remapped like any other - otherwise a duplicated
+	// choice keeps the ORIGINAL id and invokes the pre-existing local choice
+	// instead of the imported copy.
+	it("remaps choice references inside an array-valued macro", async () => {
+		const target = leaf("Target", "target-1");
+		const macro = macroChoice("m1", [choiceCommand("cmd-1", "target-1")]);
+
+		const result = await importPackaged([target, macro], "duplicate", [
+			leaf("Target", "target-1"),
+		]);
+
+		const imported = result.updatedChoices.find(
+			(c) => c.type === "Macro",
+		) as unknown as { macro: { choiceId: string }[] };
+		const importedTarget = result.updatedChoices.find(
+			(c) => c.type === "Template" && c.id !== "target-1",
+		);
+
+		expect(importedTarget).toBeDefined();
+		// The reference follows the duplicate, not the pre-existing local choice.
+		expect(imported.macro[0].choiceId).toBe(importedTarget?.id);
+		expect(imported.macro[0].choiceId).not.toBe("target-1");
+	});
+
+	it("does not silently no-op the macro id when duplicating an array-valued macro", async () => {
+		const macro = macroChoice("m1", [choiceCommand("cmd-1", "nope")]);
+		const result = await importPackaged([macro], "duplicate", [
+			macroChoice("m1", [choiceCommand("cmd-1", "nope")]),
+		]);
+
+		// The array survives the JSON round-trip whole; nothing was written onto it
+		// as a non-index property (which JSON.stringify would have dropped).
+		const persisted = JSON.parse(JSON.stringify(result.updatedChoices));
+		const imported = persisted.filter(
+			(c: IChoice) => c.type === "Macro",
+		) as unknown as { macro: unknown[] }[];
+		expect(imported).toHaveLength(2);
+		for (const m of imported) {
+			expect(Array.isArray(m.macro)).toBe(true);
+			expect(m.macro).toHaveLength(1);
+		}
+	});
+
+	// Before this change the deref here threw and aborted the WHOLE import, and
+	// guarding the preview walker is what made it reachable: the package used to
+	// die at preview, so the Import button never lit.
+	it("imports a packaged folder whose children list holds a hole", async () => {
+		const kid = leaf("Kid", "kid-1");
+		const packagedFolder = {
+			id: "f1",
+			name: "Packaged folder",
+			type: "Multi",
+			command: false,
+			collapsed: false,
+			choices: [null, kid],
+		} as unknown as IChoice;
+
+		const result = await applyPackageImport({
+			app,
+			existingChoices: [],
+			pkg: {
+				schemaVersion: QUICKADD_PACKAGE_SCHEMA_VERSION,
+				name: "Fixture",
+				description: "",
+				choices: [
+					{ choice: packagedFolder, parentChoiceId: null, pathHint: ["F"] },
+					{ choice: kid, parentChoiceId: "f1", pathHint: ["F", "Kid"] },
+				],
+				assets: [],
+			} as never,
+			choiceDecisions: [
+				{ choiceId: "f1", mode: "import" },
+				{ choiceId: "kid-1", mode: "import" },
+			],
+			assetDecisions: [],
+		});
+
+		const imported = find(result.updatedChoices, "f1") as IMultiChoice;
+		expect(imported).toBeDefined();
+		// The hole carried nothing and is gone; the real child survived, and the
+		// import completed rather than aborting on the deref.
+		expect(imported.choices?.map((c) => c.id)).toEqual(["kid-1"]);
+	});
+});