fix: preserve macro user script variable updates (#999)

* fix: sync macro user script variables

* fix: avoid overwriting executor map updates

* refactor: proxy macro variables map

* test: strengthen variables proxy coverage

* chore: include value in proxy property descriptor

* fix: preserve executor vars when custom map provided

* fix: expose hasOwnProperty shim on variables proxy

* fix: clarify variable map merge and set AI outputs into map

* chore: tidy proxy test expectation

* docs: add macro variables proxy guide

* refactor: extract macro variable wiring helpers

* chore: remove macro variables proxy dev doc

* test: add proxy edge case coverage

Author: chhoumann

src/engine/MacroChoiceEngine.entry.test.ts

@@ -1,4 +1,4 @@
-import { beforeEach, describe, expect, it, vi } from "vitest";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import { MacroChoiceEngine } from "./MacroChoiceEngine";
 import type QuickAdd from "../main";
 import type { IChoiceExecutor } from "../IChoiceExecutor";
@@ -211,6 +211,122 @@ describe("MacroChoiceEngine user script entry handling", () => {
 	});
 });
 
+describe("MacroChoiceEngine user script variable propagation", () => {
+	const app = {} as App;
+	const plugin = {} as unknown as QuickAdd;
+
+	let choiceExecutor: IChoiceExecutor;
+	let variables: Map<string, unknown>;
+	let macroChoice: IMacroChoice;
+	let logs: Array<number | undefined>;
+	let getApiSpy: ReturnType<typeof vi.spyOn>;
+
+	beforeEach(() => {
+		vi.clearAllMocks();
+		mockGetUserScript.mockReset();
+		mockInitializeUserScriptSettings.mockReset();
+		mockSuggest.mockReset();
+
+		getApiSpy = vi
+			.spyOn(QuickAddApi, "GetApi")
+			.mockReturnValue({} as unknown as ReturnType<typeof QuickAddApi.GetApi>);
+
+		logs = [];
+		variables = new Map<string, unknown>();
+		choiceExecutor = {
+			execute: vi.fn(),
+			variables,
+		};
+
+		const makeScript = (index: number): IUserScript => ({
+			id: `script-${index}`,
+			name: `Script ${index}`,
+			type: CommandType.UserScript,
+			path: `script-${index}.js`,
+			settings: {},
+		});
+
+		macroChoice = {
+			id: "macro-sequence",
+			name: "Macro sequence",
+			type: "Macro",
+			command: false,
+			runOnStartup: false,
+			macro: {
+				id: "macro-sequence",
+				name: "Macro sequence",
+				commands: [
+					makeScript(1),
+					makeScript(2),
+					makeScript(3),
+					makeScript(4),
+				],
+			} as IMacro,
+		};
+
+		mockGetUserScript.mockImplementation((command: IUserScript) => {
+			const nextValueByPath: Record<string, number> = {
+				"script-1.js": 1,
+				"script-2.js": 2,
+				"script-3.js": 3,
+				"script-4.js": 3,
+			};
+
+			const nextValue = nextValueByPath[command.path ?? ""] ?? 0;
+
+			return Promise.resolve(async (params: { variables: Record<string, unknown> }) => {
+				logs.push(params.variables.target as number | undefined);
+				if (nextValue !== 0) {
+					params.variables.target = nextValue;
+				}
+			});
+		});
+	});
+
+	afterEach(() => {
+		getApiSpy.mockRestore();
+	});
+
+	it("propagates variable mutations across sequential user scripts", async () => {
+		const engine = new MacroChoiceEngine(
+			app,
+			plugin,
+			macroChoice,
+			choiceExecutor,
+			variables,
+		);
+
+		await engine.run();
+
+		expect(logs).toEqual([undefined, 1, 2, 3]);
+		expect(choiceExecutor.variables.get("target")).toBe(3);
+	});
+
+	it("merges existing executor variables into provided map without losing state", async () => {
+		const executorVariables = new Map<string, unknown>([
+			["keep", "executor"],
+		]);
+		const providedVariables = new Map<string, unknown>([
+			["override", 1],
+		]);
+
+		const engine = new MacroChoiceEngine(
+			app,
+			plugin,
+			macroChoice,
+			{
+				...choiceExecutor,
+				variables: executorVariables,
+			},
+			providedVariables,
+		);
+
+		expect(engine["params"].variables.keep).toBe("executor");
+		expect(engine["params"].variables.override).toBe(1);
+		expect(engine["choiceExecutor"].variables).toBe(providedVariables);
+	});
+});
+
 describe("MacroChoiceEngine choice command cancellation", () => {
 	const app = {} as App;
 	let plugin: QuickAdd & { getChoiceById: ReturnType<typeof vi.fn> };

src/engine/MacroChoiceEngine.ts

@@ -47,6 +47,7 @@ import type { ScriptCondition } from "../types/macros/Conditional/types";
 import { evaluateCondition } from "./helpers/conditionalEvaluator";
 import { handleMacroAbort } from "../utils/macroAbortHandler";
 import { buildOpenFileOptions } from "./helpers/openFileOptions";
+import { createVariablesProxy } from "../utils/variablesProxy";
 
 type ConditionalScriptRunner = () => Promise<unknown>;
 
@@ -78,6 +79,42 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 	private userScriptCommand: IUserScript | null;
 	private conditionalScriptCache = new Map<string, ConditionalScriptRunner>();
 	private readonly preloadedUserScripts: Map<string, unknown>;
+	private buildParams(
+		app: App,
+		plugin: QuickAdd,
+		choiceExecutor: IChoiceExecutor,
+		sharedVariables: Map<string, unknown>
+	) {
+		return {
+			app,
+			quickAddApi: QuickAddApi.GetApi(app, plugin, choiceExecutor),
+			variables: createVariablesProxy(sharedVariables),
+			obsidian,
+			abort: (message?: string) => {
+				throw new MacroAbortError(message);
+			},
+		};
+	}
+
+	private initSharedVariables(
+		choiceExecutor: IChoiceExecutor,
+		providedVariables?: Map<string, unknown>
+	): Map<string, unknown> {
+		const existingVariables = choiceExecutor.variables;
+
+		if (providedVariables) {
+			if (existingVariables && providedVariables !== existingVariables) {
+				existingVariables.forEach((value, key) => {
+					if (!providedVariables.has(key)) {
+						providedVariables.set(key, value);
+					}
+				});
+			}
+			return providedVariables;
+		}
+
+		return existingVariables ?? new Map<string, unknown>();
+	}
 
 	constructor(
 		app: App,
@@ -93,19 +130,12 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 		this.macro = choice?.macro;
 		this.choiceExecutor = choiceExecutor;
 		this.preloadedUserScripts = preloadedUserScripts ?? new Map();
-		this.params = {
-			app: this.app,
-			quickAddApi: QuickAddApi.GetApi(app, plugin, choiceExecutor),
-			variables: {},
-			obsidian,
-			abort: (message?: string) => {
-				throw new MacroAbortError(message);
-			},
-		};
-
-		variables?.forEach((value, key) => {
-			this.params.variables[key] = value;
-		});
+		const sharedVariables = this.initSharedVariables(
+			choiceExecutor,
+			variables
+		);
+		this.choiceExecutor.variables = sharedVariables;
+		this.params = this.buildParams(app, plugin, choiceExecutor, sharedVariables);
 	}
 
 	async run(): Promise<void> {
@@ -151,14 +181,6 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 				if (command?.type === CommandType.Conditional) {
 					await this.executeConditional(command as IConditionalCommand);
 				}
-
-				this.pullExecutorVariablesIntoParams();
-				Object.keys(this.params.variables).forEach((key) => {
-					this.choiceExecutor.variables.set(
-						key,
-						this.params.variables[key]
-					);
-				});
 			}
 		} catch (error) {
 			if (
@@ -462,7 +484,6 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 	}
 
 	private async executeConditional(command: IConditionalCommand) {
-		this.pullExecutorVariablesIntoParams();
 		const shouldRunThenBranch = await evaluateCondition(command.condition, {
 			variables: this.params.variables,
 			evaluateScriptCondition: async (condition: ScriptCondition) =>
@@ -489,12 +510,6 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine {
 		this.output = value;
 	}
 
-	private pullExecutorVariablesIntoParams() {
-		this.choiceExecutor.variables.forEach((value, key) => {
-			this.params.variables[key] = value;
-		});
-	}
-
 	private async evaluateScriptCondition(
 		condition: ScriptCondition
 	): Promise<boolean> {

src/engine/SingleMacroEngine.ts

@@ -288,11 +288,6 @@ export class SingleMacroEngine {
 		engine: MacroChoiceEngine,
 		settings: Record<string, unknown>,
 	): Promise<unknown> {
-		// Ensure params reflect latest shared variables before executing
-		this.choiceExecutor.variables.forEach((value, key) => {
-			engine.params.variables[key] = value;
-		});
-
 		if (typeof member === "function") {
 			return await (
 				member as (

src/quickAddApi.ts

@@ -273,8 +273,10 @@ export class QuickAddApi {
 					}
 
 					if (settings?.shouldAssignVariables) {
-						// Copy over `output` and `output-quoted` to the variables (if 'outout' is variable name)
-						Object.assign(choiceExecutor.variables, assistantRes);
+						// Copy over `output` and `output-quoted` to the variables (if 'output' is variable name)
+						Object.entries(assistantRes).forEach(([key, value]) => {
+							choiceExecutor.variables.set(key, value);
+						});
 					}
 
 					return assistantRes;
@@ -372,8 +374,10 @@ export class QuickAddApi {
 					}
 
 					if (settings?.shouldAssignVariables) {
-						// Copy over `output` and `output-quoted` to the variables (if 'outout' is variable name)
-						Object.assign(choiceExecutor.variables, assistantRes);
+						// Copy over `output` and `output-quoted` to the variables (if 'output' is variable name)
+						Object.entries(assistantRes).forEach(([key, value]) => {
+							choiceExecutor.variables.set(key, value);
+						});
 					}
 
 					return assistantRes;