diff --git a/src/engine/MacroChoiceEngine.ts b/src/engine/MacroChoiceEngine.ts index 5bf7944e..77e83914 100644 --- a/src/engine/MacroChoiceEngine.ts +++ b/src/engine/MacroChoiceEngine.ts @@ -71,54 +71,6 @@ type UserScriptFunction = ( settings: Record ) => Promise; -function hasCommandType( - command: unknown, - type: CommandType -): command is ICommand { - return isRecord(command) && command.type === type; -} - -function isObsidianCommand(command: unknown): command is IObsidianCommand { - return hasCommandType(command, CommandType.Obsidian); -} - -function isUserScriptCommand(command: unknown): command is IUserScript { - return hasCommandType(command, CommandType.UserScript); -} - -function isChoiceCommand(command: unknown): command is IChoiceCommand { - return hasCommandType(command, CommandType.Choice); -} - -function isWaitCommand(command: unknown): command is IWaitCommand { - return hasCommandType(command, CommandType.Wait); -} - -function isNestedChoiceCommand( - command: unknown -): command is INestedChoiceCommand { - return hasCommandType(command, CommandType.NestedChoice); -} - -function isEditorCommand(command: unknown): command is IEditorCommand { - return hasCommandType(command, CommandType.EditorCommand); -} - -function isAIAssistantCommand( - command: unknown -): command is IAIAssistantCommand { - return hasCommandType(command, CommandType.AIAssistant); -} - -function isOpenFileCommand(command: unknown): command is IOpenFileCommand { - return hasCommandType(command, CommandType.OpenFile); -} - -function isConditionalCommand( - command: unknown -): command is IConditionalCommand { - return hasCommandType(command, CommandType.Conditional); -} type UserScriptObjectExport = Record & { entry?: UserScriptFunction; settings?: Record; @@ -145,6 +97,26 @@ function getUserScriptSettings( return isRecord(settings) ? settings : undefined; } +/** + * Why a command could not be run, phrased for the person reading the notice. + * + * Two shapes reach here, and telling them apart is the difference between + * actionable and baffling. A real-but-unknown type name is worth quoting: it is + * what the user greps for. An entry with no usable type has nothing to quote - + * printing `'undefined'` or `'[object Object]'` (a hand-authored package can put + * any JSON here) would send them looking for a command type that never existed. + */ +function describeUnknownType( + type: unknown, + kind: "command" | "editor command" = "command", +): string { + if (typeof type !== "string" || !type.trim()) { + return `the saved entry has no ${kind} type, so QuickAdd cannot tell what it was meant to do. It is in .obsidian/plugins/quickadd/data.json.`; + } + + return `QuickAdd does not recognise the ${kind} type '${type}'. It can come from a newer version of QuickAdd, an imported package, or a hand-edited .obsidian/plugins/quickadd/data.json.`; +} + function getConditionalScriptCacheKey(condition: ScriptCondition): string { return `${condition.scriptPath}::${condition.exportName ?? "default"}`; } @@ -310,29 +282,72 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine { protected async executeCommands(commands: ICommand[]) { try { for (const command of commands) { - if (isObsidianCommand(command)) - this.executeObsidianCommand(command); - if (isUserScriptCommand(command)) - await this.executeUserScript(command); - if (isChoiceCommand(command)) - await this.executeChoice(command); - if (isWaitCommand(command)) { - await waitFor(command.time); - } - if (isNestedChoiceCommand(command)) { - await this.executeNestedChoice(command); - } - if (isEditorCommand(command)) { - await this.executeEditorCommand(command); - } - if (isAIAssistantCommand(command)) { - await this.executeAIAssistant(command); - } - if (isOpenFileCommand(command)) { - await this.executeOpenFile(command); - } - if (isConditionalCommand(command)) { - await this.executeConditional(command); + // A null/undefined entry is corruption, not a command, and the old + // `isRecord` inside every type guard was the only thing keeping it + // from throwing here. It stays SILENT: the same shape package import + // skips without comment (packageImportService), and a red notice per + // hole would bury the #1583 resilience work under noise. + if (!command) continue; + + // A switch, not the old flat `if (isX(command))` chain: that chain had + // no else, so a type it did not know was dropped on the floor with no + // error, no notice and no log, and the macro still reported success + // (#1571). The `never` assignment in the default branch makes adding a + // CommandType without a handler a tsc error, so the hole cannot come + // back. `type` is all the old guards ever checked, so the casts are + // exactly as strict as what they replaced. + switch (command.type) { + case CommandType.Obsidian: + this.executeObsidianCommand(command as IObsidianCommand); + break; + case CommandType.UserScript: + await this.executeUserScript(command as IUserScript); + break; + case CommandType.Choice: + await this.executeChoice(command as IChoiceCommand); + break; + case CommandType.Wait: + await waitFor((command as IWaitCommand).time); + break; + case CommandType.NestedChoice: + await this.executeNestedChoice(command as INestedChoiceCommand); + break; + case CommandType.EditorCommand: + await this.executeEditorCommand(command as IEditorCommand); + break; + case CommandType.AIAssistant: + await this.executeAIAssistant(command as IAIAssistantCommand); + break; + case CommandType.InfiniteAIAssistant: + // Never creatable, never executable: it shipped in 1.2.0 with no + // builder entry and no engine branch, so the only way to hold one + // is a hand-edited data.json or a hand-authored package. Naming it + // here is what the `never` check costs, and it turns the silent + // drop into a visible one until the type is removed outright. + this.reportUnrunnableCommand( + command, + 'no released QuickAdd has ever been able to run an "Infinite AI Assistant" command - it cannot be created or configured in the macro builder either. Delete the step from the macro.', + ); + break; + case CommandType.OpenFile: + await this.executeOpenFile(command as IOpenFileCommand); + break; + case CommandType.Conditional: + await this.executeConditional(command as IConditionalCommand); + break; + default: { + // Compile-time exhaustiveness (the idiom executeEditorCommand + // already uses) AND a runtime shout: `type` is only typed at the + // edges, so a newer QuickAdd's data.json read by an older one - + // which our own downgrade recipe produces - really does arrive + // here with a string TypeScript says is impossible. + const exhaustiveCheck: never = command.type; + this.reportUnrunnableCommand( + command, + describeUnknownType(exhaustiveCheck), + ); + break; + } } } } catch (error) { @@ -350,6 +365,35 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine { } } + /** + * Say out loud that a command was skipped, then carry on with the rest of + * the macro - the same shape `executeObsidianCommand` uses for a command + * whose plugin is gone. Skipping is the safer half of the choice: the step + * did nothing either way, and aborting a macro on load-order noise would + * take the file-writing steps that DID work with it. The shout is the half + * that was missing (#1571) - without it the macro reports success and the + * user's next step inherits the hole, e.g. an unset output variable that + * turns into a mid-macro prompt asking them to type the missing value. + * + * One notice per skipped command, not per type: the same cadence as every + * other skip in this file, and a macro that holds several is exactly the + * case where seeing each one matters. + */ + private reportUnrunnableCommand(command: ICommand, reason: string): void { + const name = + typeof command?.name === "string" && command.name.trim() + ? `'${command.name}'` + : "an unnamed command"; + + // "QuickAdd did not stop the macro" rather than "the rest of the macro + // ran": this fires BEFORE the rest runs, and when the skipped step is the + // last one there is no rest. What the user needs from the tail is that + // the macro was not aborted, so the result they get is a partial one. + log.logError( + `Skipped ${name} in the macro for '${this.choice.name}': ${reason} QuickAdd did not stop the macro, so its result may be incomplete.` + ); + } + // Slightly modified from Templater's user script engine: // https://github.com/SilentVoid13/Templater/blob/master/src/UserTemplates/UserTemplateParser.ts protected async executeUserScript(command: IUserScript) { @@ -650,8 +694,17 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine { MoveCursorToLineEndCommand.run(this.app); break; default: { + // Skip and shout, like the outer loop - this used to throw, which + // aborted the whole macro. Same threat model, so it deserves the same + // answer: a newer QuickAdd that adds an EditorCommandType writes a + // data.json an older one still has to read, and one unknown editor + // step is no reason to take the file-writing steps around it down + // with it. const exhaustiveCheck: never = command.editorCommandType; - throw new Error(`Unhandled editor command type: ${exhaustiveCheck}`); + this.reportUnrunnableCommand( + command, + describeUnknownType(exhaustiveCheck, "editor command"), + ); } } } diff --git a/src/engine/MacroChoiceEngine.unknownCommand.test.ts b/src/engine/MacroChoiceEngine.unknownCommand.test.ts new file mode 100644 index 00000000..fd13fc58 --- /dev/null +++ b/src/engine/MacroChoiceEngine.unknownCommand.test.ts @@ -0,0 +1,264 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("../quickAddApi", () => ({ QuickAddApi: { GetApi: () => ({}) } })); +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 type { ICommand } from "../types/macros/ICommand"; +import { CommandType } from "../types/macros/CommandType"; +import { MacroChoiceEngine } from "./MacroChoiceEngine"; +import { log } from "../logger/logManager"; +import { settingsStore } from "../settingsStore"; + +const OBSIDIAN_COMMAND_ID = "app:go-back"; + +/** + * An Obsidian command is the cheapest "did the macro carry on?" probe: it is + * synchronous, needs no vault, and lands on a spy. + */ +function obsidianCommand(id: string, name = "Marker"): ICommand { + return { + id, + name, + type: CommandType.Obsidian, + commandId: OBSIDIAN_COMMAND_ID, + } as unknown as ICommand; +} + +function buildEngine(commands: unknown[], variables: Record = {}) { + const executeCommandById = vi.fn(); + const app = { + commands: { + executeCommandById, + commands: { [OBSIDIAN_COMMAND_ID]: { id: OBSIDIAN_COMMAND_ID } }, + }, + } as unknown as App; + + const choice = { + id: "choice-id", + name: "Test choice", + type: "Macro", + command: false, + runOnStartup: false, + macro: { id: "macro-id", name: "Test macro", commands }, + } as unknown as IMacroChoice; + + const choiceExecutor: IChoiceExecutor = { + execute: vi.fn(), + variables: new Map(), + }; + + const engine = new MacroChoiceEngine( + app, + { settings: settingsStore.getState() } as never, + choice, + choiceExecutor, + new Map(Object.entries(variables)), + ); + + return { engine, executeCommandById }; +} + +function runMacro(commands: unknown[], variables: Record = {}) { + const { engine, executeCommandById } = buildEngine(commands, variables); + return { run: engine.run(), executeCommandById }; +} + +/** + * The dispatch loop used to be a flat `if (isX(command))` chain with no else, + * so a command type it did not know was dropped with no error, no notice and + * no log, and the macro still reported success (#1571). The step after it then + * inherited the hole - an unset output variable turns into a mid-macro prompt + * asking the user to type the value the skipped step was supposed to produce. + */ +describe("MacroChoiceEngine over a command type it cannot run (#1571)", () => { + let errors: string[]; + + beforeEach(() => { + errors = []; + vi.spyOn(log, "logError").mockImplementation((msg: string | Error) => { + errors.push(msg instanceof Error ? msg.message : msg); + }); + }); + + it("says so, instead of dropping the step silently", async () => { + const { run } = runMacro([ + { id: "c1", name: "From a newer QuickAdd", type: "SomeFutureThing" }, + ]); + await run; + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain("From a newer QuickAdd"); + expect(errors[0]).toContain("SomeFutureThing"); + expect(errors[0]).toContain("Test choice"); + }); + + it("points at the three ways an unrunnable command actually arrives", async () => { + // A hand-edited data.json, an imported package, and - the one our own + // downgrade recipe produces - a data.json written by a NEWER QuickAdd. + const { run } = runMacro([ + { id: "c1", name: "Mystery", type: "SomeFutureThing" }, + ]); + await run; + + expect(errors[0]).toMatch(/newer version of QuickAdd/i); + expect(errors[0]).toMatch(/imported package/i); + expect(errors[0]).toMatch(/data\.json/); + }); + + it("carries on with the rest of the macro rather than aborting it", async () => { + // Skipping is the safer half: the step did nothing either way, and + // aborting would take the file-writing steps that DID work with it. + const { run, executeCommandById } = runMacro([ + { id: "c1", name: "Mystery", type: "SomeFutureThing" }, + obsidianCommand("c2"), + ]); + await run; + + expect(executeCommandById).toHaveBeenCalledWith(OBSIDIAN_COMMAND_ID); + expect(errors).toHaveLength(1); + }); + + it("shouts about the Infinite AI Assistant command, which never ran anywhere", async () => { + const { run, executeCommandById } = runMacro([ + { + id: "c1", + name: "Summarise", + type: CommandType.InfiniteAIAssistant, + model: "gpt-4", + outputVariableName: "out", + }, + obsidianCommand("c2"), + ]); + await run; + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain("Summarise"); + expect(errors[0]).toMatch(/Infinite AI Assistant/i); + expect(executeCommandById).toHaveBeenCalledWith(OBSIDIAN_COMMAND_ID); + }); + + it("shouts from inside a conditional branch too", async () => { + // Branches recurse through the same loop, so the hole was there as well. + const { run } = runMacro( + [ + { + id: "cond", + name: "If condition", + type: CommandType.Conditional, + condition: { + mode: "variable", + variableName: "go", + operator: "isTruthy", + valueType: "string", + }, + thenCommands: [ + { id: "c1", name: "Mystery", type: "SomeFutureThing" }, + ], + elseCommands: [], + }, + ], + { go: "yes" }, + ); + await run; + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain("Mystery"); + }); + + it("never quotes a type it does not have", async () => { + // A truncated write or a hand-authored package step can leave an entry with + // no type at all, a non-string one, or no object at all. Quoting + // `'undefined'` (or `'[object Object]'`) would send the user hunting for a + // command type that never existed - but staying silent would be the very + // hole this change closes, so each of these still gets said out loud. + const { run } = runMacro([ + { id: "c1", name: "Half a command" }, + { id: "c2", name: "Weird", type: { nested: true } }, + 5, + "Wait", + ]); + await run; + + expect(errors).toHaveLength(4); + for (const message of errors) { + expect(message).toMatch(/has no command type/); + expect(message).not.toMatch(/undefined|\[object Object\]/); + } + }); + + it("does not quote a name it does not have either", async () => { + // A hand-authored package step can omit `name`. `'undefined'` in the + // notice is the same class of lie as quoting a type that is not there. + const { run } = runMacro([{ id: "c1", type: "SomeFutureThing" }]); + await run; + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain("an unnamed command"); + expect(errors[0]).not.toMatch(/undefined/); + }); + + it("skips an unknown editor command instead of aborting the macro", async () => { + // Editor commands dispatch through their own switch, which used to THROW + // on an unknown type and take the whole macro down. Same threat model as + // the outer loop - a newer QuickAdd adding an EditorCommandType - so it + // gets the same answer. + const { run, executeCommandById } = runMacro([ + { + id: "c1", + name: "Fold everything", + type: CommandType.EditorCommand, + editorCommandType: "FoldEverything", + }, + obsidianCommand("c2"), + ]); + await run; + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain("FoldEverything"); + expect(errors[0]).toContain("editor command type"); + expect(executeCommandById).toHaveBeenCalledWith(OBSIDIAN_COMMAND_ID); + }); + + it("skips a null entry without a notice - corruption is not a command", async () => { + // `packageImportService` guards for exactly this shape, and a red notice + // per hole would bury the settings-resilience work (#1583) under noise. + const { run, executeCommandById } = runMacro([ + null, + undefined, + obsidianCommand("c2"), + ]); + await run; + + expect(errors).toEqual([]); + expect(executeCommandById).toHaveBeenCalledWith(OBSIDIAN_COMMAND_ID); + }); + + it("guards runSubset too, which is a public entry of its own", async () => { + const { engine } = buildEngine([]); + await engine.runSubset([ + { id: "c1", name: "Mystery", type: "SomeFutureThing" }, + ] as unknown as ICommand[]); + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain("SomeFutureThing"); + }); + + it("still runs the types it does know, exactly once each", async () => { + const { run, executeCommandById } = runMacro([ + obsidianCommand("c1", "First"), + { id: "c2", name: "Wait", type: CommandType.Wait, time: 1 }, + obsidianCommand("c3", "Second"), + ]); + await run; + + expect(executeCommandById).toHaveBeenCalledTimes(2); + expect(errors).toEqual([]); + }); +});