From f692836e9c929c37e6ad16c0536239578d2fe186 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 10:58:19 +0200 Subject: [PATCH 1/8] fix(prompts): make a dismissed prompt a typed cancellation, not an English sentence A dismissed QuickAdd prompt used to reject with a bare English string, and `isCancellationError` decided "did the user cancel?" by comparing the caught value against three literals - two of which were case variants of the same sentence. Rewording any of them, or adding a prompt with a slightly different one, silently reclassified "the user backed out" as "an error occurred", with no compiler or test signal (#1577). All nine reject sites now throw `promptCancelled()`, a `UserCancelError`. That is deliberately the SAME class the ~40 consumers already converted the string into, rather than a new one, for two reasons: it is what the public docs already promise (`MacroAbortError("Input cancelled by user")` - `UserCancelError` inherits that name), and because `UserCancelError extends MacroAbortError` a dismissal now arrives already classified, so a consumer that forgets to convert it still aborts quietly instead of reporting a failure. Two consumers needed real changes rather than none: - `choiceRename` gated its error logging on `instanceof Error`, which was a fine proxy for "not a cancellation" while cancellations were strings. It would now log every cancelled rename as an error, so it asks the contract instead. - `runOnePagePreflight` compared against the literal `"cancelled"` alongside its `instanceof MacroAbortError` check; the instanceof check alone now covers both the native modal and a remote provider. The legacy sentinel list stays in `isCancellationError`, narrowed to its real purpose: a *user script* can still throw one, and MacroChoiceEngine has honoured that for as long as the sentinels existed. `cancellationContract.test.ts` ratchets it - no source file may reject with a bare string literal or re-introduce a sentinel - so the fragility cannot come back by copy-paste. Verified in an isolated vault (Obsidian 1.13.0): dismissing the script picker in the macro editor now floats `Input cancelled by user` with a real stack instead of the bare string `no input given.` (which had no stack at all, and so could not be attributed or classified by anything downstream), and dismissing a `{{VALUE:}}` prompt still aborts the template run quietly with no note created and no error notice. Refs #1577 --- src/errors/UserCancelError.ts | 24 ++++ src/formatters/previewDiagnostics.ts | 7 +- ...icCheckboxPrompt.audit-api-prompts.test.ts | 5 +- .../genericCheckboxPrompt.ts | 3 +- .../GenericInputPrompt/GenericInputPrompt.ts | 3 +- src/gui/GenericSuggester/genericSuggester.ts | 3 +- .../GenericWideInputPrompt.test.ts | 3 +- .../GenericWideInputPrompt.ts | 3 +- src/gui/InputSuggester/inputSuggester.ts | 3 +- src/gui/MathModal.ts | 3 +- src/gui/MultiChoiceSettingsModal.ts | 3 +- src/gui/MultiSuggester/multiSuggester.ts | 3 +- src/gui/choiceRename.ts | 10 +- src/preflight/OnePageInputModal.test.ts | 5 +- src/preflight/OnePageInputModal.ts | 5 +- .../runOnePagePreflight.fallback.test.ts | 8 +- src/preflight/runOnePagePreflight.ts | 8 +- src/utils/cancellationContract.test.ts | 109 ++++++++++++++++++ src/utils/errorUtils.ts | 53 +++++---- 19 files changed, 211 insertions(+), 50 deletions(-) create mode 100644 src/utils/cancellationContract.test.ts diff --git a/src/errors/UserCancelError.ts b/src/errors/UserCancelError.ts index 6947c1e96..2780ece03 100644 --- a/src/errors/UserCancelError.ts +++ b/src/errors/UserCancelError.ts @@ -14,3 +14,27 @@ import { MacroAbortError } from "./MacroAbortError"; * error name continue to recognise it. */ export class UserCancelError extends MacroAbortError {} + +/** + * The one message every prompt uses when the user dismisses it. Matches the text the + * public docs promise (`MacroAbortError("Input cancelled by user")`), so a script that + * surfaces `error.message` reads the same whether the prompt was dismissed in the app, + * dismissed remotely, or converted from a legacy sentinel on the way up. + */ +export const PROMPT_CANCELLED_MESSAGE = "Input cancelled by user"; + +/** + * The cancellation a prompt throws when the user dismisses it (Escape / Cancel / close). + * + * Prompts used to reject with a bare English sentence, recognised by exact string + * equality (see the legacy sentinel list in `errorUtils`) — so rewording one, or + * adding a prompt with a slightly different + * sentence, silently turned "the user cancelled" into "an error occurred" with no compiler + * or test signal (#1577). This is the typed replacement, and it is deliberately the SAME + * class the ~40 consumers already converted that string into, rather than a new one: a + * dismissal now arrives already classified, so a consumer that forgets to convert it still + * behaves correctly. + */ +export function promptCancelled(): UserCancelError { + return new UserCancelError(PROMPT_CANCELLED_MESSAGE); +} diff --git a/src/formatters/previewDiagnostics.ts b/src/formatters/previewDiagnostics.ts index da605ebd2..ad11fda32 100644 --- a/src/formatters/previewDiagnostics.ts +++ b/src/formatters/previewDiagnostics.ts @@ -74,10 +74,9 @@ function stripBrandPrefix(message: string): string { export function describePreviewFailure(error: unknown): string | null { // A cancelled prompt is a user action, not an authoring mistake. The preview // formatters never prompt, but a nested resolver still can. Check the RAW - // value first: QuickAdd's modals reject with a bare string - // (`rejectPromise("No input given.")`), and `isCancellationError` only ever - // matches strings - so an `instanceof Error` gate ahead of it would let every - // cancellation through as "Preview unavailable". + // value first, before the `instanceof Error` gate below: a dismissal is a + // typed UserCancelError (#1577), and letting it reach that gate would report + // every cancellation as "Preview unavailable". if (isCancellationError(error)) return null; if (!(error instanceof Error)) return PREVIEW_FAILED_MESSAGE; const message = error.message ?? ""; diff --git a/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.audit-api-prompts.test.ts b/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.audit-api-prompts.test.ts index 228705a96..ddb13e61f 100644 --- a/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.audit-api-prompts.test.ts +++ b/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.audit-api-prompts.test.ts @@ -1,5 +1,6 @@ import type { App } from "obsidian"; import { beforeAll, describe, expect, it, vi } from "vitest"; +import { UserCancelError } from "../../errors/UserCancelError"; // Mock obsidian with a local Modal/ButtonComponent/ToggleComponent so // super.onClose() resolves (the shared stub's Modal omits onClose on the @@ -161,7 +162,7 @@ describe("GenericCheckboxPrompt header + cancel (audit: prompts-gui-checkbox-pro expect(() => buttonByText(prompt, "Cancel")).not.toThrow(); }); - it("Cancel rejects the promise (no input given)", async () => { + it("Cancel rejects the promise with a typed cancellation", async () => { const prompt = new GenericCheckboxPrompt(app, ["a", "b"], ["a"]); const promise = prompt.promise; @@ -169,7 +170,7 @@ describe("GenericCheckboxPrompt header + cancel (audit: prompts-gui-checkbox-pro new Event("click", { bubbles: true }), ); - await expect(promise).rejects.toBe("no input given."); + await expect(promise).rejects.toBeInstanceOf(UserCancelError); }); it("Submit resolves the selected items", async () => { diff --git a/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.ts b/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.ts index d2e4d4bd2..5a8087b91 100644 --- a/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.ts +++ b/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.ts @@ -1,5 +1,6 @@ import type { App } from "obsidian"; import { ButtonComponent, Modal, ToggleComponent } from "obsidian"; +import { promptCancelled } from "../../errors/UserCancelError"; export default class GenericCheckboxPrompt extends Modal { private resolvePromise: (value: string[]) => void; @@ -54,7 +55,7 @@ export default class GenericCheckboxPrompt extends Modal { onClose() { super.onClose(); - if (!this.resolved) this.rejectPromise("no input given."); + if (!this.resolved) this.rejectPromise(promptCancelled()); } private addCheckboxRows() { diff --git a/src/gui/GenericInputPrompt/GenericInputPrompt.ts b/src/gui/GenericInputPrompt/GenericInputPrompt.ts index 876b99136..5d653c90b 100644 --- a/src/gui/GenericInputPrompt/GenericInputPrompt.ts +++ b/src/gui/GenericInputPrompt/GenericInputPrompt.ts @@ -8,6 +8,7 @@ import { positionInputPromptCursor } from "../inputPromptCursor"; import { renderPromptContextLine } from "../promptContextLine"; import type { ImagePasteHandle } from "../imagePasteHandler"; import { attachImagePasteHandler } from "../imagePasteHandler"; +import { promptCancelled } from "../../errors/UserCancelError"; /** * The keyboard gesture that skips an optional prompt: ctrl/cmd+shift+Enter. @@ -290,7 +291,7 @@ export default class GenericInputPrompt extends Modal { } private resolveInput() { - if (!this.didSubmit) this.rejectPromise("No input given."); + if (!this.didSubmit) this.rejectPromise(promptCancelled()); else this.resolvePromise(this.input); } diff --git a/src/gui/GenericSuggester/genericSuggester.ts b/src/gui/GenericSuggester/genericSuggester.ts index 7536f748a..b72662a12 100644 --- a/src/gui/GenericSuggester/genericSuggester.ts +++ b/src/gui/GenericSuggester/genericSuggester.ts @@ -6,6 +6,7 @@ import { normalizeDisplayItem, normalizeQuery, } from "../suggesters/utils"; +import { promptCancelled } from "../../errors/UserCancelError"; type SuggestRender = (value: T, el: HTMLElement) => void; @@ -160,7 +161,7 @@ export default class GenericSuggester extends FuzzySuggestModal { onClose() { super.onClose(); - if (!this.resolved) this.rejectPromise("no input given."); + if (!this.resolved) this.rejectPromise(promptCancelled()); } private warnIfEmptyDisplay(): void { diff --git a/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.test.ts b/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.test.ts index 627e1e037..a06aa9a4e 100644 --- a/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.test.ts +++ b/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.test.ts @@ -3,6 +3,7 @@ import { Modal } from "obsidian"; import type QuickAdd from "../../main"; import { setQuickAddInstance } from "../../quickAddInstance"; import GenericWideInputPrompt from "./GenericWideInputPrompt"; +import { UserCancelError } from "../../errors/UserCancelError"; // The obsidian-stub Modal does not implement onOpen/onClose; the prompt calls // super.onOpen()/super.onClose(). Provide no-ops so construction and close do not @@ -220,7 +221,7 @@ describe("image paste submit/cancel races (issue #1484)", () => { ).find((button) => button.textContent === "Cancel") as HTMLButtonElement; cancelButton.click(); - await expect(waitForClose).rejects.toBe("No input given."); + await expect(waitForClose).rejects.toBeInstanceOf(UserCancelError); // The save landing later must NOT resurrect the submit on the closed // modal (deferred submit is guarded by didClose). diff --git a/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.ts b/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.ts index 12d0e39da..38fc9f48a 100644 --- a/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.ts +++ b/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.ts @@ -10,6 +10,7 @@ import { attachTextareaIndent } from "../components/textareaIndent"; import { isSkipPromptShortcut } from "../GenericInputPrompt/GenericInputPrompt"; import type { ImagePasteHandle } from "../imagePasteHandler"; import { attachImagePasteHandler } from "../imagePasteHandler"; +import { promptCancelled } from "../../errors/UserCancelError"; export default class GenericWideInputPrompt extends Modal { public waitForClose: Promise; @@ -281,7 +282,7 @@ export default class GenericWideInputPrompt extends Modal { } private resolveInput() { - if (!this.didSubmit) this.rejectPromise("No input given."); + if (!this.didSubmit) this.rejectPromise(promptCancelled()); else this.resolvePromise(this.input); } diff --git a/src/gui/InputSuggester/inputSuggester.ts b/src/gui/InputSuggester/inputSuggester.ts index ee41f5001..a16bd4511 100644 --- a/src/gui/InputSuggester/inputSuggester.ts +++ b/src/gui/InputSuggester/inputSuggester.ts @@ -6,6 +6,7 @@ import { normalizeDisplayItem, normalizeQuery, } from "../suggesters/utils"; +import { promptCancelled } from "../../errors/UserCancelError"; type SuggestRender = (value: T, el: HTMLElement) => void; @@ -275,7 +276,7 @@ export default class InputSuggester extends FuzzySuggestModal { onClose() { super.onClose(); - if (!this.resolved) this.rejectPromise("no input given."); + if (!this.resolved) this.rejectPromise(promptCancelled()); } private warnIfEmptyDisplay(): void { diff --git a/src/gui/MathModal.ts b/src/gui/MathModal.ts index fc1599189..759110f85 100644 --- a/src/gui/MathModal.ts +++ b/src/gui/MathModal.ts @@ -10,6 +10,7 @@ import { import { getQuickAddInstance } from "../quickAddInstance"; import { LATEX_CURSOR_MOVE_HERE } from "../LaTeXSymbols"; import { LaTeXSuggester } from "./suggesters/LaTeXSuggester"; +import { promptCancelled } from "../errors/UserCancelError"; export class MathModal extends Modal { public waitForClose: Promise; @@ -154,7 +155,7 @@ export class MathModal extends Modal { const output = this.inputEl.value .replace(/\\n/g, `\\\\n`) .replace(new RegExp(LATEX_CURSOR_MOVE_HERE, "g"), ""); - if (!this.didSubmit) this.rejectPromise("No input given."); + if (!this.didSubmit) this.rejectPromise(promptCancelled()); else this.resolvePromise(output); } diff --git a/src/gui/MultiChoiceSettingsModal.ts b/src/gui/MultiChoiceSettingsModal.ts index ebf9e2693..6901809d2 100644 --- a/src/gui/MultiChoiceSettingsModal.ts +++ b/src/gui/MultiChoiceSettingsModal.ts @@ -2,6 +2,7 @@ import type { App } from "obsidian"; import { ButtonComponent, Modal, Setting } from "obsidian"; import type IMultiChoice from "../types/choices/IMultiChoice"; import { addChoiceIconSetting } from "./ChoiceBuilder/components/choiceIconSetting"; +import { promptCancelled } from "../errors/UserCancelError"; export class MultiChoiceSettingsModal extends Modal { public waitForClose: Promise; @@ -88,7 +89,7 @@ export class MultiChoiceSettingsModal extends Modal { onClose() { super.onClose(); if (!this.didSubmit) { - this.rejectPromise("No input given."); + this.rejectPromise(promptCancelled()); return; } diff --git a/src/gui/MultiSuggester/multiSuggester.ts b/src/gui/MultiSuggester/multiSuggester.ts index 771d80577..25e1a40f0 100644 --- a/src/gui/MultiSuggester/multiSuggester.ts +++ b/src/gui/MultiSuggester/multiSuggester.ts @@ -1,6 +1,7 @@ import type { App } from "obsidian"; import { Modal, Notice, Setting } from "obsidian"; import { normalizeDisplayItem } from "../suggesters/utils"; +import { promptCancelled } from "../../errors/UserCancelError"; export interface MultiSuggesterOptions { /** Modal title / prompt header. */ @@ -237,7 +238,7 @@ export default class MultiSuggester extends Modal { if (this.didSubmit) { this.resolvePromise(this.skipped ? [] : this.collectResult()); } else { - this.rejectPromise("no input given."); + this.rejectPromise(promptCancelled()); } } } diff --git a/src/gui/choiceRename.ts b/src/gui/choiceRename.ts index aaa952466..69a69f9dd 100644 --- a/src/gui/choiceRename.ts +++ b/src/gui/choiceRename.ts @@ -1,6 +1,7 @@ import type { App } from "obsidian"; import GenericInputPrompt from "./GenericInputPrompt/GenericInputPrompt"; import { log } from "src/logger/logManager"; +import { isCancellationError, toError } from "../utils/errorUtils"; import type { ChoiceType } from "../types/choices/choiceType"; import { choiceNounCapitalized } from "../utils/choiceNoun"; @@ -29,10 +30,11 @@ export async function promptRenameChoice( if (!trimmed || trimmed === currentName) return null; return trimmed; } catch (error) { - // GenericInputPrompt rejects with a string ("No input given.") when the - // user cancels (Esc/Cancel) — that is expected, not an error. Surface only - // genuine failures (Error instances) instead of swallowing them silently. - if (error instanceof Error) log.logError(error); + // A dismissal (Esc/Cancel) is expected, not an error. Ask the cancellation + // contract rather than gating on `instanceof Error`: since #1577 a dismissal + // IS an Error, so that gate would report every cancelled rename as a failure. + if (isCancellationError(error)) return null; + log.logError(toError(error, "Could not rename")); return null; } } diff --git a/src/preflight/OnePageInputModal.test.ts b/src/preflight/OnePageInputModal.test.ts index f3bd7c267..d4be308bd 100644 --- a/src/preflight/OnePageInputModal.test.ts +++ b/src/preflight/OnePageInputModal.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import type { App } from "obsidian"; import type { FieldRequirement } from "./RequirementCollector"; import { OnePageInputModal } from "./OnePageInputModal"; +import { UserCancelError } from "../errors/UserCancelError"; import { buildValueVariableKey } from "src/utils/valueSyntax"; const { attachImagePasteHandlerMock } = vi.hoisted(() => ({ @@ -745,13 +746,13 @@ describe("OnePageInputModal", () => { }); describe("Esc settles the modal promise (issue #1259 rider)", () => { - it("rejects with 'cancelled' when closed without submitting", async () => { + it("rejects with a typed cancellation when closed without submitting", async () => { const requirements: FieldRequirement[] = [ { id: "note", label: "note", type: "text" }, ]; const modal = new OnePageInputModal({} as App, requirements, new Map()); modal.onClose(); - await expect(modal.waitForClose).rejects.toBe("cancelled"); + await expect(modal.waitForClose).rejects.toBeInstanceOf(UserCancelError); }); it("does not double-settle after a submit", async () => { diff --git a/src/preflight/OnePageInputModal.ts b/src/preflight/OnePageInputModal.ts index 498a47d82..b363c03e3 100644 --- a/src/preflight/OnePageInputModal.ts +++ b/src/preflight/OnePageInputModal.ts @@ -33,6 +33,7 @@ import { normalizeNumericValue, normalizeSliderValue, } from "src/utils/valueSyntax"; +import { promptCancelled } from "../errors/UserCancelError"; type PreviewComputer = ( values: Record, @@ -685,7 +686,7 @@ export class OnePageInputModal extends Modal { private cancel() { this.settled = true; this.close(); - this.rejectPromise("cancelled"); + this.rejectPromise(promptCancelled()); } onClose() { @@ -695,7 +696,7 @@ export class OnePageInputModal extends Modal { // otherwise the choice execution hangs forever on waitForClose. if (!this.settled) { this.settled = true; - this.rejectPromise("cancelled"); + this.rejectPromise(promptCancelled()); } } diff --git a/src/preflight/runOnePagePreflight.fallback.test.ts b/src/preflight/runOnePagePreflight.fallback.test.ts index f29a6182a..19818879c 100644 --- a/src/preflight/runOnePagePreflight.fallback.test.ts +++ b/src/preflight/runOnePagePreflight.fallback.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import type { App } from "obsidian"; import { runOnePagePreflight } from "./runOnePagePreflight"; import { MacroAbortError } from "../errors/MacroAbortError"; +import { promptCancelled } from "../errors/UserCancelError"; import type ICaptureChoice from "../types/choices/ICaptureChoice"; import type { IChoiceExecutor } from "../IChoiceExecutor"; @@ -146,9 +147,10 @@ describe("runOnePagePreflight fallback warning", () => { expect(logWarningMock).not.toHaveBeenCalled(); }); - it("rethrows the modal's 'cancelled' rejection without warning", async () => { - modalOutcome = () => Promise.reject("cancelled"); - await expect(run()).rejects.toBe("cancelled"); + it("rethrows the modal's dismissal without warning", async () => { + const cancelled = promptCancelled(); + modalOutcome = () => Promise.reject(cancelled); + await expect(run()).rejects.toBe(cancelled); expect(logWarningMock).not.toHaveBeenCalled(); }); diff --git a/src/preflight/runOnePagePreflight.ts b/src/preflight/runOnePagePreflight.ts index 1973f3544..78dd7972f 100644 --- a/src/preflight/runOnePagePreflight.ts +++ b/src/preflight/runOnePagePreflight.ts @@ -244,10 +244,10 @@ export async function runOnePagePreflight( return true; } catch (error) { // Propagate an explicit cancellation/abort so the run stops instead of - // continuing with the inputs missing. The native modal throws the string - // "cancelled"; a remote provider rejects with UserCancelError (a - // MacroAbortError subclass). - if (error === "cancelled" || error instanceof MacroAbortError) { + // continuing with the inputs missing. Both the native modal and a remote + // provider signal a dismissal with UserCancelError, a MacroAbortError + // subclass (#1577), so one check covers both. + if (error instanceof MacroAbortError) { throw error; } // Any other error degrades to the sequential runtime prompts. That diff --git a/src/utils/cancellationContract.test.ts b/src/utils/cancellationContract.test.ts new file mode 100644 index 000000000..e2f6bac8f --- /dev/null +++ b/src/utils/cancellationContract.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, it } from "vitest"; +import { readdirSync, readFileSync, statSync } from "node:fs"; +import { join } from "node:path"; +import { isCancellationError } from "./errorUtils"; +import { + PROMPT_CANCELLED_MESSAGE, + UserCancelError, + promptCancelled, +} from "../errors/UserCancelError"; +import { MacroAbortError } from "../errors/MacroAbortError"; + +/** + * The cancellation contract (#1577): a dismissed prompt throws a typed + * `UserCancelError`, never a bare English sentence. The string list in + * `isCancellationError` exists only for user scripts that still throw a legacy + * sentinel - if QuickAdd's own code starts producing one again, the fragility the + * types removed is back, and the compiler will not say a word. Hence the ratchet. + */ + +const SRC = join(__dirname, ".."); + +function walk(dir: string, out: string[] = []): string[] { + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + walk(full, out); + continue; + } + if (!/\.(ts|svelte)$/.test(entry)) continue; + if (/\.test\.ts$/.test(entry)) continue; + out.push(full); + } + return out; +} + +/** + * A reject/throw whose argument is a bare string literal. Deliberately narrow: + * it catches the exact shape every prompt used before #1577 and would use again + * by copy-paste, without flagging `new Error("...")`. + */ +const BARE_STRING_REJECTION = + /\b(?:reject|rejectPromise|rejectPromise!)\s*\(\s*(["'`])/g; + +describe("cancellation contract (#1577)", () => { + it("no source file rejects a promise with a bare string literal", () => { + const offenders: string[] = []; + for (const file of walk(SRC)) { + const text = readFileSync(file, "utf8"); + for (const match of text.matchAll(BARE_STRING_REJECTION)) { + const line = text.slice(0, match.index).split("\n").length; + offenders.push( + `${file.slice(SRC.length + 1)}:${line} — ${match[0].trim()}…`, + ); + } + } + expect( + offenders, + "A dismissed prompt must reject with promptCancelled(), not a string. " + + "For a genuine failure, reject with an Error so it carries a stack.", + ).toEqual([]); + }); + + it("no source file re-introduces a legacy cancellation sentinel", () => { + const sentinels = ['"No input given."', '"no input given."']; + const offenders: string[] = []; + for (const file of walk(SRC)) { + // errorUtils owns the compatibility list by design. + if (file.endsWith(join("utils", "errorUtils.ts"))) continue; + const text = readFileSync(file, "utf8"); + for (const sentinel of sentinels) { + if (text.includes(sentinel)) { + offenders.push(`${file.slice(SRC.length + 1)} — ${sentinel}`); + } + } + } + expect(offenders).toEqual([]); + }); + + it("recognises a typed prompt dismissal", () => { + expect(isCancellationError(promptCancelled())).toBe(true); + expect(isCancellationError(new UserCancelError("anything"))).toBe(true); + }); + + it("still recognises the legacy sentinels a user script may throw", () => { + expect(isCancellationError("No input given.")).toBe(true); + expect(isCancellationError("no input given.")).toBe(true); + expect(isCancellationError("cancelled")).toBe(true); + }); + + it("does not mistake a real failure for a cancellation", () => { + expect(isCancellationError(new Error("ENOENT"))).toBe(false); + expect(isCancellationError(new MacroAbortError("target missing"))).toBe( + false, + ); + expect(isCancellationError("Cancelled")).toBe(false); + expect(isCancellationError(undefined)).toBe(false); + expect(isCancellationError(null)).toBe(false); + }); + + it("carries the message the public docs promise", () => { + const error = promptCancelled(); + // docs/.../QuickAddAPI.md tells scripts to expect + // MacroAbortError("Input cancelled by user"). + expect(error.message).toBe(PROMPT_CANCELLED_MESSAGE); + expect(error.message).toBe("Input cancelled by user"); + expect(error.name).toBe("MacroAbortError"); + expect(error).toBeInstanceOf(MacroAbortError); + }); +}); diff --git a/src/utils/errorUtils.ts b/src/utils/errorUtils.ts index 404186f91..bef2eb886 100644 --- a/src/utils/errorUtils.ts +++ b/src/utils/errorUtils.ts @@ -1,6 +1,7 @@ import { log } from "../logger/logManager"; import type { ErrorLevel } from "../logger/errorLevel"; import { ErrorLevel as ErrorLevelEnum } from "../logger/errorLevel"; +import { UserCancelError } from "../errors/UserCancelError"; /** * Maximum number of errors to keep in the error log @@ -54,39 +55,51 @@ export function toError(err: unknown, contextMessage?: string): Error { } /** - * Checks if an error indicates user cancellation rather than a real error. - * Used to distinguish between intentional user cancellations (Escape key, Cancel button) - * and actual errors (network failures, file system errors, etc.) - * + * Checks if a caught value means "the user backed out" rather than "something broke". + * + * Every QuickAdd prompt signals a dismissal by throwing {@link UserCancelError} + * (see `promptCancelled()`), so this is an `instanceof` check. Because + * `UserCancelError extends MacroAbortError`, a dismissal that nobody classifies still + * aborts the run quietly instead of being reported as a failure. + * * @param error - The error to check * @returns true if the error indicates user cancellation, false otherwise - * + * * @example * ```ts * try { * const result = await promptUser(); * } catch (error) { - * if (isCancellationError(error)) { - * throw new MacroAbortError("Input cancelled by user"); - * } - * throw error; // Re-throw actual errors + * if (isCancellationError(error)) return null; // the user backed out + * throw error; // a real failure * } * ``` */ export function isCancellationError(error: unknown): boolean { - if (typeof error !== "string") return false; - - const cancellationMessages = [ - "no input given.", // GenericSuggester, InputSuggester, GenericCheckboxPrompt - "No input given.", // GenericInputPrompt, MathModal - // GenericYesNoPrompt is deliberately absent: it resolves a dismissal - // instead of rejecting, so it never reaches this check (#1567). - "cancelled" // OnePagePreflight - ]; - - return cancellationMessages.includes(error); + // QuickAdd's own prompts throw a typed cancellation (#1577). This is the whole + // check for every in-plugin prompt; the sentinels below are compatibility only. + if (error instanceof UserCancelError) return true; + + // Legacy sentinels. QuickAdd's prompts used to reject with one of these bare + // English sentences and nothing else, so a *user script* may still throw one + // (or re-throw one it caught), and MacroChoiceEngine has honoured that for as + // long as the sentinels existed. Kept for that reason alone - no QuickAdd code + // produces them any more, which `errorUtils.cancellationContract.test.ts` + // pins. Do not add to this list: a new prompt throws `promptCancelled()`. + return typeof error === "string" && LEGACY_CANCELLATION_SENTINELS.has(error); } +/** + * Bare-string cancellations QuickAdd's prompts rejected with before #1577. + * Note the two case variants of one sentence - that inconsistency is exactly why + * string matching was the wrong signal. + */ +const LEGACY_CANCELLATION_SENTINELS: ReadonlySet = new Set([ + "no input given.", // GenericSuggester, InputSuggester, GenericCheckboxPrompt, MultiSuggester + "No input given.", // GenericInputPrompt, GenericWideInputPrompt, MathModal, MultiChoiceSettingsModal + "cancelled", // OnePageInputModal +]); + /** * Reports an error to the logging system with additional context * Converts any error type to a proper Error object and logs it with the appropriate level From 680688520f8e1bd7618ddfdee90c4fc611664363 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 11:03:29 +0200 Subject: [PATCH 2/8] fix(api): stop turning a broken prompt into "the user gave no input" Seven of the nine prompt wrappers on the script API surface did catch (error) { throwIfPromptCancelled(error); return undefined; } and `throwIfPromptCancelled` returned normally for anything that was not a MacroAbortError or a cancellation string. So every genuine failure - a throwing display callback, a bad argument, an Obsidian API error - was handed back to the script as `undefined`: nothing logged, nothing shown, and a value the script then acted on. `yesNoPrompt` reported a defect as "the user answered No"; `infoDialog` is declared `Promise`, so there it was not merely unlogged but unrepresentable (#1575). The helper is now `rethrowPromptError(error): never` with a trailing `throw error`, and all nine sites are identical - including the two in `requestInputs` that already rethrew and were the model for this. `never` is the load-bearing part: the swallow existed because a `void` helper let a wrapper forget the rethrow, and now that omission is a compile error. Deliberately NOT reported inside the wrapper. Every path that runs a script already reports a propagating error exactly once (MacroChoiceEngine for user scripts, TemplateChoiceEngine for inline scripts, main.ts / choiceSuggester at the top), and `reportError` raises a 15-second notice, so reporting here too would stack two or three notices for one failure. Nothing documented `undefined`: every signature in QuickAddAPI.md is non-optional, cancellation is documented as a MacroAbortError rejection, and UserScripts.md already tells scripts `throw error; // real errors should still bubble up` - which these seven methods made impossible. The one doc example that contradicted this ("if (!input) { // User cancelled") has been wrong since #976 and is rewritten to state the two-outcome contract. Verified in an isolated vault with a user script whose suggester display callback has a real TypeError. Before: QuickAdd swallowed it, the macro carried on, and wrote a note reading "the script carried on with: undefined" - no notice, nothing in the console. After: no note, and exactly one notice naming the script and the actual TypeError. Refs #1575 --- docs/src/content/docs/docs/QuickAddAPI.md | 43 +++++++----- src/engine/MacroChoiceEngine.entry.test.ts | 15 +++-- src/quickAddApi.test.ts | 76 ++++++++++++++++------ src/quickAddApi.ts | 56 +++++++++------- 4 files changed, 128 insertions(+), 62 deletions(-) diff --git a/docs/src/content/docs/docs/QuickAddAPI.md b/docs/src/content/docs/docs/QuickAddAPI.md index 1da98ebb1..3c6d8f843 100644 --- a/docs/src/content/docs/docs/QuickAddAPI.md +++ b/docs/src/content/docs/docs/QuickAddAPI.md @@ -936,29 +936,42 @@ module.exports = async (params) => { ## Error Handling Best Practices -Always wrap API calls in try-catch blocks: +A prompt has exactly two non-happy outcomes, and they are different things: + +- **The user dismissed it** (Escape, Cancel, closing the dialog). The promise rejects with `MacroAbortError("Input cancelled by user")`. Let it bubble and QuickAdd stops the run quietly - which is almost always what you want. +- **Something broke** (a bug in your script, a vault error). The promise rejects with that error. Let it bubble and QuickAdd reports it once, so you get a notice and a console entry to debug from. + +A prompt never resolves `undefined` to mean either of those. An empty string means the user submitted an empty answer, which is a real answer. ```javascript module.exports = async (params) => { const { quickAddApi } = params; - + + // The simplest correct script: no try/catch at all. A dismissal stops the + // macro; a real failure is reported. Only catch if you need to do something + // in between. + const input = await quickAddApi.inputPrompt("Enter value:"); + if (input === "") return; // submitted empty on purpose + + // Process input... +}; +``` + +If your script does need to react to a cancellation, tell the two apart by the error name: + +```javascript +module.exports = async (params) => { + const { quickAddApi } = params; + try { const input = await quickAddApi.inputPrompt("Enter value:"); - - if (!input) { - // User cancelled - handle gracefully - return; - } - // Process input... - } catch (error) { - console.error("Script error:", error); - - await quickAddApi.infoDialog( - "Error", - `An error occurred: ${error.message}` - ); + if (error?.name === "MacroAbortError") { + // The user backed out. Clean up and stop. + return; + } + throw error; // a real failure - let QuickAdd report it } }; ``` diff --git a/src/engine/MacroChoiceEngine.entry.test.ts b/src/engine/MacroChoiceEngine.entry.test.ts index 937f0f23d..439e63758 100644 --- a/src/engine/MacroChoiceEngine.entry.test.ts +++ b/src/engine/MacroChoiceEngine.entry.test.ts @@ -874,11 +874,14 @@ describe("QuickAddApi prompt cancellation", () => { ).rejects.toThrow(MacroAbortError); }); - it("still resolves undefined for other prompt errors", async () => { - mockInputPrompt.mockRejectedValueOnce(new Error("boom")); - - await expect( - QuickAddApi.inputPrompt(app, "Enter value"), - ).resolves.toBeUndefined(); + // A genuine failure must NOT come back as "the user gave no input" (#1575): + // the script would carry on and act on a value that was never entered. + it("propagates other prompt errors instead of resolving undefined", async () => { + const failure = new Error("boom"); + mockInputPrompt.mockRejectedValueOnce(failure); + + await expect(QuickAddApi.inputPrompt(app, "Enter value")).rejects.toBe( + failure, + ); }); }); diff --git a/src/quickAddApi.test.ts b/src/quickAddApi.test.ts index c3730462a..5366c4b29 100644 --- a/src/quickAddApi.test.ts +++ b/src/quickAddApi.test.ts @@ -152,6 +152,7 @@ vi.mock("./utils/errorUtils", async () => { const { QuickAddApi } = await import("./quickAddApi"); const { MacroAbortError } = await import("./errors/MacroAbortError"); +const { promptCancelled } = await import("./errors/UserCancelError"); // --------------------------------------------------------------------------- // Test doubles for app / plugin / choiceExecutor @@ -248,10 +249,12 @@ describe("static prompt wrappers", () => { ); }); - it("returns undefined for a generic (non-cancellation) error", async () => { - mocks.genericInputPrompt.mockRejectedValue(new Error("boom")); - const out = await QuickAddApi.inputPrompt(app, "Header"); - expect(out).toBeUndefined(); + it("propagates a generic (non-cancellation) error", async () => { + const failure = new Error("boom"); + mocks.genericInputPrompt.mockRejectedValue(failure); + await expect(QuickAddApi.inputPrompt(app, "Header")).rejects.toBe( + failure, + ); }); it("converts a cancellation string into a MacroAbortError", async () => { @@ -300,9 +303,10 @@ describe("static prompt wrappers", () => { ); }); - it("swallows generic errors as undefined", async () => { - mocks.genericWideInputPrompt.mockRejectedValue(new Error("x")); - expect(await QuickAddApi.wideInputPrompt(app, "H")).toBeUndefined(); + it("propagates generic errors", async () => { + const failure = new Error("x"); + mocks.genericWideInputPrompt.mockRejectedValue(failure); + await expect(QuickAddApi.wideInputPrompt(app, "H")).rejects.toBe(failure); }); }); @@ -327,9 +331,12 @@ describe("static prompt wrappers", () => { ); }); - it("swallows generic errors as undefined", async () => { - mocks.genericYesNoPromptAsk.mockRejectedValue(new Error("x")); - expect(await QuickAddApi.yesNoPrompt(app, "H")).toBeUndefined(); + // The worst-typed swallow of all: `undefined` is falsy, so a defect used to + // read to the script as "the user answered No". + it("propagates generic errors instead of reading as a falsy No", async () => { + const failure = new Error("x"); + mocks.genericYesNoPromptAsk.mockRejectedValue(failure); + await expect(QuickAddApi.yesNoPrompt(app, "H")).rejects.toBe(failure); }); }); @@ -339,6 +346,21 @@ describe("static prompt wrappers", () => { await QuickAddApi.infoDialog(app, "H", ["a", "b"]); expect(mocks.genericInfoDialog).toHaveBeenCalledWith(app, "H", ["a", "b"]); }); + + // Declared `Promise`, so a swallowed failure was indistinguishable + // from success - the error was not merely unlogged, it was unrepresentable. + it("propagates a generic error", async () => { + const failure = new Error("dom exploded"); + mocks.genericInfoDialog.mockRejectedValue(failure); + await expect(QuickAddApi.infoDialog(app, "H", "x")).rejects.toBe(failure); + }); + + it("converts a dismissal into a MacroAbortError", async () => { + mocks.genericInfoDialog.mockRejectedValue(promptCancelled()); + await expect(QuickAddApi.infoDialog(app, "H", "x")).rejects.toBeInstanceOf( + MacroAbortError, + ); + }); }); describe("checkboxPrompt", () => { @@ -353,9 +375,10 @@ describe("static prompt wrappers", () => { ); }); - it("returns undefined on a generic error", async () => { - mocks.genericCheckboxPrompt.mockRejectedValue(new Error("nope")); - expect(await QuickAddApi.checkboxPrompt(app, [])).toBeUndefined(); + it("propagates a generic error", async () => { + const failure = new Error("nope"); + mocks.genericCheckboxPrompt.mockRejectedValue(failure); + await expect(QuickAddApi.checkboxPrompt(app, [])).rejects.toBe(failure); }); }); }); @@ -403,9 +426,10 @@ describe("datePrompt", () => { ); }); - it("returns undefined on a generic error", async () => { - mocks.vDateInputPrompt.mockRejectedValue(new Error("fail")); - expect(await QuickAddApi.datePrompt(app, "H")).toBeUndefined(); + it("propagates a generic error", async () => { + const failure = new Error("fail"); + mocks.vDateInputPrompt.mockRejectedValue(failure); + await expect(QuickAddApi.datePrompt(app, "H")).rejects.toBe(failure); }); it("converts a cancellation into a MacroAbortError", async () => { @@ -501,9 +525,23 @@ describe("suggester", () => { }); }); - it("returns undefined on a generic error", async () => { - mocks.genericSuggester.mockRejectedValue(new Error("boom")); - expect(await QuickAddApi.suggester(app, ["a"], ["a"])).toBeUndefined(); + it("propagates a generic error", async () => { + const failure = new Error("boom"); + mocks.genericSuggester.mockRejectedValue(failure); + await expect(QuickAddApi.suggester(app, ["a"], ["a"])).rejects.toBe(failure); + }); + + // The try also spans the caller-supplied display mapping, so a buggy display + // callback used to be swallowed too - the exact #1575 field report. + it("propagates a throwing display callback instead of returning undefined", async () => { + mocks.genericSuggester.mockResolvedValue("a"); + await expect( + QuickAddApi.suggester( + app, + (value: string) => (value as unknown as { nope: string }).nope.trim(), + ["a"], + ), + ).rejects.toBeInstanceOf(TypeError); }); }); diff --git a/src/quickAddApi.ts b/src/quickAddApi.ts index c4b367945..52aec1765 100644 --- a/src/quickAddApi.ts +++ b/src/quickAddApi.ts @@ -52,7 +52,10 @@ import { FieldSuggestionCache } from "./utils/FieldSuggestionCache"; import { FieldSuggestionFileFilter } from "./utils/FieldSuggestionFileFilter"; import { InlineFieldParser } from "./utils/InlineFieldParser"; import { MacroAbortError } from "./errors/MacroAbortError"; -import { UserCancelError } from "./errors/UserCancelError"; +import { + PROMPT_CANCELLED_MESSAGE, + UserCancelError, +} from "./errors/UserCancelError"; import { formatISODate } from "./utils/dateParser"; import type { InputPromptOptions } from "./types/inputPrompt"; import type { NumericInputConfig, SliderConfig } from "./utils/valueSyntax"; @@ -229,8 +232,7 @@ export class QuickAddApi { try { collected = await provider.requestInputs(missing); } catch (error) { - throwIfPromptCancelled(error); - throw error; + rethrowPromptError(error); } } else { const modal = new OnePageInputModal( @@ -241,8 +243,7 @@ export class QuickAddApi { try { collected = await modal.waitForClose; } catch (error) { - throwIfPromptCancelled(error); - throw error; + rethrowPromptError(error); } } } @@ -874,8 +875,7 @@ export class QuickAddApi { options, ); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } @@ -905,8 +905,7 @@ export class QuickAddApi { } return value; } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } @@ -927,8 +926,7 @@ export class QuickAddApi { options, ); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } @@ -940,11 +938,10 @@ export class QuickAddApi { try { answer = await GenericYesNoPrompt.Ask(app, header, text); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } - if (answer === null) throw new UserCancelError("Input cancelled by user"); + if (answer === null) throw new UserCancelError(PROMPT_CANCELLED_MESSAGE); return answer; } @@ -956,8 +953,7 @@ export class QuickAddApi { try { return await GenericInfoDialog.Show(app, header, text); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } @@ -1004,8 +1000,7 @@ export class QuickAddApi { options?.renderItem, ); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } @@ -1022,17 +1017,34 @@ export class QuickAddApi { ? GenericCheckboxPrompt.Open(app, items, selectedItems) : GenericCheckboxPrompt.Open(app, items, selectedItems, header)); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } } -function throwIfPromptCancelled(error: unknown): void { +/** + * The single error policy for every prompt on the script API surface. + * + * A dismissal becomes a `UserCancelError` (which the macro engine turns into a + * quiet abort); anything else propagates untouched. It returns `never`, which is + * the point: these wrappers used to call a `void` helper and then + * `return undefined`, so a genuine failure inside a modal - a throwing display + * callback, a bad argument, an Obsidian API error - was handed back to the script + * as "the user gave no input", with nothing logged and nothing shown (#1575). + * `never` makes that omission a compile error rather than a silent one. + * + * Not reported here on purpose: every path that runs a script already reports a + * propagating error exactly once (MacroChoiceEngine for user scripts, + * TemplateChoiceEngine for inline scripts, main.ts / choiceSuggester at the top), + * and `reportError` raises a 15-second Notice, so reporting here too would stack + * two or three notices for one failure. + */ +function rethrowPromptError(error: unknown): never { if (error instanceof MacroAbortError) { throw error; } if (isCancellationError(error)) { - throw new UserCancelError("Input cancelled by user"); + throw new UserCancelError(PROMPT_CANCELLED_MESSAGE); } + throw error; } From f1d496dab11d61f4ac2017dac5463a9f80d0668c Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 13:55:37 +0200 Subject: [PATCH 3/8] fix: tell the user when a QuickAdd action fails instead of dying in the console MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Around forty `onClick(async …)` / `onclick={…}` handlers hand Obsidian or Svelte a promise it discards, plus a long tail of deliberately fire-and-forget `void someAsyncCall()`. When one failed the user got nothing: no notice, no entry in QuickAdd's error log, only an unhandled rejection they will never see (#1576). One seam, not forty try/catch blocks. `registerUnhandledRejectionReporter` listens once on `window` (via `registerDomEvent`, so it unregisters on unload) and reports through the same channel as every other failure. A per-handler wrapper is exactly the boilerplate #1567 failed to converge on: it cannot cover Svelte prop handlers or floated calls, and it rots the moment someone adds handler forty-one. The trade-off accepted is that the notice carries no per-action context, only the error itself. Three properties make it safe rather than noisy: - Attribution. Obsidian evaluates a plugin's main.js with a `sourceURL` of `plugin:`, so an Error built inside QuickAdd carries frames like `at mS.getChoiceByName (plugin:quickadd:414:30553)`. Verified live in Obsidian 1.13.0. Anything without that frame is left completely alone - including a non-Error rejection, which has no stack to attribute at all. - Cancellations stay silent. A floated prompt (the macro editor's script picker floats one) must not tell the user "an error occurred" because they pressed Escape. This is why the typed cancellation had to land first: the dismissal used to be a bare string with no stack, so it was neither attributable nor classifiable here. - Dedupe by throw SITE, not message, within a 10s window. Several floated calls run at high frequency - a validator on every keystroke, a reindex on every vault change, a Svelte unmount on every modal close - and messages often embed a varying value, so one loop failing over 500 notes would otherwise raise 500 fifteen-second notices. Same site collapses to one report; distinct sites stay distinct. We claim the event (`preventDefault`) for rejections we recognise so the browser does not also report what we just reported; the stack is not lost, because ConsoleErrorLogger passes the original Error to `console.error`. Verified in an isolated vault, comparing a build of 234a3493 against this one in the same instance, on a real user path: a choice whose `type` is corrupt (the #1566 shape) made the settings gear button do absolutely nothing - `Invalid choice type` escaped as an unhandled rejection with zero notices. Now it raises "A QuickAdd action failed: Invalid choice type". Dismissing the macro editor's script picker still raises no notice, and where it used to escape as the opaque string `no input given.` it is now a typed `MacroAbortError: Input cancelled by user` with a full stack. The reporter takes an injected clock: stubbing the global `Date.now` in its test deadlocks vitest, which reads it for its own timeouts. Refs #1576 --- src/main.ts | 7 + src/utils/unhandledRejectionReporter.test.ts | 143 +++++++++++++++++++ src/utils/unhandledRejectionReporter.ts | 117 +++++++++++++++ 3 files changed, 267 insertions(+) create mode 100644 src/utils/unhandledRejectionReporter.test.ts create mode 100644 src/utils/unhandledRejectionReporter.ts diff --git a/src/main.ts b/src/main.ts index e65576e48..2a13b4a59 100644 --- a/src/main.ts +++ b/src/main.ts @@ -9,6 +9,7 @@ import { ConsoleErrorLogger } from "./logger/consoleErrorLogger"; import { GuiLogger } from "./logger/guiLogger"; import { LogManager } from "./logger/logManager"; import { reportError, withErrorHandling } from "./utils/errorUtils"; +import { registerUnhandledRejectionReporter } from "./utils/unhandledRejectionReporter"; import { openQuickAddSettings } from "./utils/openPluginSettings"; import { StartupMacroEngine } from "./engine/StartupMacroEngine"; import { ChoiceExecutor } from "./choiceExecutor"; @@ -311,6 +312,12 @@ export default class QuickAdd extends Plugin { log.register(new ConsoleErrorLogger()).register(new GuiLogger(this)); + // Must come after the loggers: a QuickAdd promise that rejects with nobody + // awaiting it (a settings click handler, a floated call) used to leave the user + // with nothing but a console line. Now it reports through the same channel as + // every other failure (#1576). + registerUnhandledRejectionReporter(this); + if (this.settings.enableRibbonIcon) { this.addRibbonIcon("file-plus", "QuickAdd", () => { openChoiceLauncher(this); diff --git a/src/utils/unhandledRejectionReporter.test.ts b/src/utils/unhandledRejectionReporter.test.ts new file mode 100644 index 000000000..b0ef006b9 --- /dev/null +++ b/src/utils/unhandledRejectionReporter.test.ts @@ -0,0 +1,143 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { registerUnhandledRejectionReporter } from "./unhandledRejectionReporter"; +import { MacroAbortError } from "../errors/MacroAbortError"; +import { promptCancelled } from "../errors/UserCancelError"; +import { log } from "../logger/logManager"; + +/** + * Errors get attributed to QuickAdd by the `plugin:` marker Obsidian puts in a + * plugin bundle's stack frames (verified live in Obsidian 1.13.0). Build one, with a + * controllable throw site so the site-based dedupe can be exercised. + */ +function pluginError(message: string, site = "doThing", name = "Error"): Error { + const error = new Error(message); + error.name = name; + error.stack = `${name}: ${message}\n at mS.${site} (plugin:quickadd:414:30553)`; + return error; +} + +function foreignError(message: string): Error { + const error = new Error(message); + error.stack = `Error: ${message}\n at x (plugin:some-other-plugin:1:1)`; + return error; +} + +describe("unhandled rejection reporter (#1576)", () => { + let listener: ((event: PromiseRejectionEvent) => void) | null = null; + let logError: ReturnType; + // A hand-cranked clock, injected. Stubbing the global `Date.now` here deadlocks + // vitest, which reads it for its own timeouts. + let clock = 1_000_000; + + const host = { + manifest: { id: "quickadd" }, + registerDomEvent( + _el: Window, + _type: "unhandledrejection", + callback: (event: PromiseRejectionEvent) => void, + ) { + listener = callback; + }, + }; + + /** A minimal stand-in for PromiseRejectionEvent (jsdom's needs a real promise). */ + function fire(reason: unknown) { + let defaultPrevented = false; + listener?.({ + reason, + preventDefault() { + defaultPrevented = true; + }, + } as unknown as PromiseRejectionEvent); + return { defaultPrevented }; + } + + beforeEach(() => { + listener = null; + clock = 1_000_000; + logError = vi.spyOn(log, "logError").mockImplementation(() => {}); + registerUnhandledRejectionReporter(host, () => clock); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("reports a QuickAdd failure and suppresses the console default", () => { + const { defaultPrevented } = fire(pluginError("Invalid choice type")); + + expect(defaultPrevented).toBe(true); + expect(logError).toHaveBeenCalledTimes(1); + expect(String(logError.mock.calls[0]?.[0])).toContain("Invalid choice type"); + }); + + it("leaves another plugin's rejection completely alone", () => { + const { defaultPrevented } = fire(foreignError("not ours")); + + expect(defaultPrevented).toBe(false); + expect(logError).not.toHaveBeenCalled(); + }); + + it("ignores a rejection it cannot attribute (a bare string has no stack)", () => { + const { defaultPrevented } = fire("no input given."); + + expect(defaultPrevented).toBe(false); + expect(logError).not.toHaveBeenCalled(); + }); + + it("silences a dismissed prompt instead of reporting it", () => { + const cancelled = promptCancelled(); + cancelled.stack = "MacroAbortError: x\n at a (plugin:quickadd:1:1)"; + + const { defaultPrevented } = fire(cancelled); + + // Claimed (so the console stays quiet) but never reported. + expect(defaultPrevented).toBe(true); + expect(logError).not.toHaveBeenCalled(); + }); + + it("silences a deliberate abort", () => { + const abort = new MacroAbortError("target file missing"); + abort.stack = "MacroAbortError: x\n at a (plugin:quickadd:1:1)"; + + fire(abort); + + expect(logError).not.toHaveBeenCalled(); + }); + + it("reports a repeated failure once per window, not once per occurrence", () => { + // A validator that throws on every keystroke must not bury the user. + for (let i = 0; i < 20; i++) fire(pluginError("validator exploded")); + expect(logError).toHaveBeenCalledTimes(1); + + clock += 9_000; + fire(pluginError("validator exploded")); + expect(logError).toHaveBeenCalledTimes(1); + + clock += 2_000; + fire(pluginError("validator exploded")); + expect(logError).toHaveBeenCalledTimes(2); + }); + + it("collapses one broken loop that fails on many different values", () => { + // Same throw site, message embeds the note path: one bug, one notice. + for (let i = 0; i < 200; i++) { + fire(pluginError(`Could not read note ${i}.md`, "readNote")); + } + expect(logError).toHaveBeenCalledTimes(1); + }); + + it("does not let one noisy failure mask a different one", () => { + fire(pluginError("first", "siteA")); + fire(pluginError("second", "siteB")); + fire(pluginError("first", "siteA")); + + expect(logError).toHaveBeenCalledTimes(2); + }); + + it("bounds the dedupe map without wrongly suppressing distinct sites", () => { + for (let i = 0; i < 200; i++) fire(pluginError("boom", `site${i}`)); + // Every one is a distinct throw site, so every one is a distinct bug. + expect(logError).toHaveBeenCalledTimes(200); + }); +}); diff --git a/src/utils/unhandledRejectionReporter.ts b/src/utils/unhandledRejectionReporter.ts new file mode 100644 index 000000000..d04ec2b9c --- /dev/null +++ b/src/utils/unhandledRejectionReporter.ts @@ -0,0 +1,117 @@ +import { MacroAbortError } from "../errors/MacroAbortError"; +import { isCancellationError, reportError } from "./errorUtils"; + +/** + * Reports a promise rejection that escaped QuickAdd unhandled, instead of letting it + * die in the console. + * + * Across `src/` there are roughly forty `onClick(async …)` / `onclick={…}` handlers that + * hand Obsidian or Svelte a promise it discards, plus a long tail of deliberately + * fire-and-forget `void someAsyncCall()`. When one of those fails the user gets nothing: + * no notice, no entry in QuickAdd's error log, only an `Uncaught (in promise) …` line + * they will never see. Clicking the gear on a choice whose `type` is corrupt, for + * instance, threw `Invalid choice type` and simply did nothing (#1576). + * + * This is one seam instead of forty `try/catch` blocks. A per-handler wrapper is exactly + * the boilerplate #1567 failed to converge on: it cannot cover Svelte prop handlers or + * floated calls, and it rots the moment someone adds handler forty-one. The trade-off + * accepted is that the notice carries no per-action context, only the error itself. + */ + +/** + * Suppress a repeat of the same failure for this long. Several of the floated calls run + * at high frequency - a validator on every keystroke, a reindex on every vault change, + * a Svelte unmount on every modal close - so without this a single broken one would bury + * the user under 15-second notices. + */ +const REPEAT_WINDOW_MS = 10_000; +/** Bound the dedupe map so a pathological run cannot grow it without limit. */ +const MAX_TRACKED = 50; + +export interface UnhandledRejectionReporterHost { + manifest: { id: string }; + registerDomEvent( + el: Window, + type: "unhandledrejection", + callback: (event: PromiseRejectionEvent) => void, + ): void; +} + +/** + * Dedupe on the throw SITE, not the message. + * + * A message often embeds a varying value ("Could not read note "), so keying on it + * would let one broken loop over 500 notes raise 500 notices. The first `plugin:` + * frame is where the Error was constructed, so the same bug collapses to one report + * however many values it fails on, while two genuinely different bugs stay distinct. + */ +function dedupeKey(error: Error, pluginId: string): string { + const frame = (error.stack ?? "") + .split("\n") + .find((line) => line.includes(`plugin:${pluginId}`)); + return `${error.name}@${frame?.trim() ?? error.message}`; +} + +/** + * True only when the rejection demonstrably came from QuickAdd's own bundle. + * + * Obsidian evaluates a plugin's `main.js` with a `sourceURL` of `plugin:`, so any + * Error constructed inside QuickAdd carries frames like + * `at mS.getChoiceByName (plugin:quickadd:414:30553)`. Requiring that keeps QuickAdd from + * claiming another plugin's bug, and is why a non-Error rejection (a bare string has no + * stack) is left alone: with nothing to attribute it to, reporting it would be a guess. + */ +function isFromPlugin(reason: unknown, pluginId: string): boolean { + return ( + reason instanceof Error && + typeof reason.stack === "string" && + reason.stack.includes(`plugin:${pluginId}`) + ); +} + +export function registerUnhandledRejectionReporter( + plugin: UnhandledRejectionReporterHost, + /** + * Injected so the dedupe window can be tested without stubbing the global + * `Date.now` - vitest itself reads it for timeouts, and freezing it deadlocks the + * runner. + */ + now: () => number = Date.now, +): void { + const pluginId = plugin.manifest.id; + const recentlyReported = new Map(); + + plugin.registerDomEvent(window, "unhandledrejection", (event) => { + const reason = event.reason; + if (!isFromPlugin(reason, pluginId)) return; + + // Ours, so stop the console noise either way. + event.preventDefault(); + + // A dismissed prompt or a deliberate abort is not a failure. It reaches here + // when a handler floats a prompt (e.g. the macro editor's script picker), and + // telling the user "an error occurred" because they pressed Escape would be + // worse than the silence this replaces. + if (reason instanceof MacroAbortError || isCancellationError(reason)) return; + + const error = reason as Error; + const key = dedupeKey(error, pluginId); + const at = now(); + const last = recentlyReported.get(key); + if (last !== undefined && at - last < REPEAT_WINDOW_MS) return; + + if (recentlyReported.size >= MAX_TRACKED) { + for (const [tracked, seenAt] of recentlyReported) { + if (at - seenAt >= REPEAT_WINDOW_MS) recentlyReported.delete(tracked); + } + // Still full: every entry is fresh, so drop the oldest to make room. + if (recentlyReported.size >= MAX_TRACKED) { + const oldest = recentlyReported.keys().next(); + if (!oldest.done) recentlyReported.delete(oldest.value); + } + } + recentlyReported.set(key, at); + + reportError(error, "A QuickAdd action failed"); + }); +} From c787d30ea9559556f82b4ddd5c7eff0d36b09721 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 14:21:13 +0200 Subject: [PATCH 4/8] fix(interactive): stop fabricating a "No" from a malformed remote reply #1574's headline claim - that a remote client cannot express a dismissed Yes/No - is wrong, and this commit does not "add" a cancel channel. `submitReply` has taken a per-prompt `cancelled` flag since the seam landed (b280fcd1 / #1471), it rejects that one prompt with UserCancelError, it is documented at CLI.md, and it is tested. The issue cites interactivePromptServer.ts:440, which is inside `submitReply`, not `finish()` (`finish` rejects with a plain "Interactive session ended"). Proved end-to-end over real loopback HTTP against the isolated vault: replying `{"cancelled":true}` to a `confirm` prompt ends the run with `{"kind":"error","error":"Input cancelled by user"}` and the script's else-branch never runs - the same class and message the in-app path throws. What IS real on that seam is the leniency underneath the claim. `RemotePromptProvider.yesNoPrompt` collapsed every non-`true` reply to `false`. So a client that omitted `value`, sent `null` (exactly the shape #1574 proposed for a dismissal), or made a typo silently answered "No", and the script walked its else-branch on an answer the user never gave. Yes/No is the one prompt where a malformed reply is indistinguishable from a real one - in-app, `false` can only come from clicking No - so it is the one prompt that now validates, and a dismissal already has a wire form to use instead. The other types stay lenient on purpose: `""` and `[]` are answers a user really can give (Skip affordances, optional fields), so tightening them would break optional prompts on remote runs. `cancelled` is also now strictly `=== true`. It arrives from untrusted JSON, and the loose truthy check meant `"cancelled":"no"` killed the run, and a cancel beat a real `value` sent alongside it. The documented wire form has always been `true`. Live matrix over real HTTP, one session per row: {"cancelled":true} -> error "Input cancelled by user", no file written {"value":true} -> done, script took the Yes branch {"value":false} -> done, script took the No branch {"value":null} -> error naming the fix, was a silent "No" {"cancelled":"no","value":true} -> done on the real value, was a spurious abort Also pins what was structurally untestable: the provider's test stub could only resolve, so nothing asserted that a client cancel propagates. There is now a rejecting stub and a case per method - including `requestInputs` - plus a wire-level case for the strict flag. Two honest notes rather than code changes. The file's "a script cannot tell it was driven remotely" claim now states how a dismissal fits it, and records the one genuine asymmetry: cancelling an `info` prompt aborts the run, while GenericInfoDialog in-app has no reject path at all. That stays, because it is a remote client's only way to bail out mid-run; CLI.md now tells clients to send a plain reply when they only mean to close an info panel. Closes #1574 --- docs/src/content/docs/docs/Advanced/CLI.md | 19 +++++ .../interactivePromptServer.test.ts | 21 +++++ src/interactive/interactivePromptServer.ts | 16 +++- src/interactive/promptProvider.test.ts | 85 +++++++++++++++++++ src/interactive/promptProvider.ts | 46 +++++++++- 5 files changed, 182 insertions(+), 5 deletions(-) diff --git a/docs/src/content/docs/docs/Advanced/CLI.md b/docs/src/content/docs/docs/Advanced/CLI.md index 556d36410..78d1281d2 100644 --- a/docs/src/content/docs/docs/Advanced/CLI.md +++ b/docs/src/content/docs/docs/Advanced/CLI.md @@ -151,6 +151,25 @@ acknowledgement, `form` → an object mapping each field's `id` to its string value (date fields use the `@date:ISO` format). The run's outcome arrives as the `done`/`error` poll event. +### Cancelling + +`{"cancelled":true}` is how you say *the user dismissed this prompt*. It aborts +the run exactly as pressing Escape on the in-app dialog does. Two details worth +knowing: + +- The flag must be the literal `true`. Any other value (`"true"`, `1`, `"no"`) is + ignored and your `value` is used as a normal answer, so a typo cannot silently + kill a run. +- Do not use it just to close an `info` panel. `info` is the one prompt that + cannot be cancelled in the app - the dialog has no reject path - so cancelling + it remotely aborts a run that would have continued. Send a plain reply instead. + It stays cancellable because it is a client's only way to bail out mid-run. + +A `confirm` prompt is the one type that validates its reply: it needs `true` or +`false`. Anything else (a missing `value`, `null`, a typo) fails the prompt with a +protocol error rather than being read as "No" - the user never answered, so +QuickAdd will not invent an answer for them. + Good to know: - **Desktop only.** The bridge binds to `127.0.0.1`, is gated by the per-session `token`, rejects browser (`Origin`/`Referer`) and non-loopback `Host` requests, and the server is ephemeral - it starts on the first session and stops when the last one ends. diff --git a/src/interactive/interactivePromptServer.test.ts b/src/interactive/interactivePromptServer.test.ts index 94e3a6ae1..474f53d98 100644 --- a/src/interactive/interactivePromptServer.test.ts +++ b/src/interactive/interactivePromptServer.test.ts @@ -172,6 +172,27 @@ describe("interactivePromptServer session multiplexing", () => { interactivePromptServer.finish(s.id, { kind: "done", result: {} }); }); + // The flag arrives from untrusted JSON. A loose truthy check meant + // `"cancelled": "no"` killed the run, and a cancel beat a real `value` that was + // also present. The documented wire form has always been the literal `true`. + it.each([["no"], [1], [{}], ["false"], [[]]])( + "treats a non-true cancelled flag (%o) as a normal answer", + async (flag) => { + const s = interactivePromptServer.createSession(); + const prompt = interactivePromptServer.emitPrompt(s.id, { + type: "input", + header: "Name", + multiline: false, + }); + const rid = pendingRequestId(s.id); + expect( + interactivePromptServer.submitReply(s.id, rid, "Ada", flag), + ).toBe(true); + await expect(prompt).resolves.toBe("Ada"); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }, + ); + it("caps the number of concurrent sessions", () => { const created: string[] = []; try { diff --git a/src/interactive/interactivePromptServer.ts b/src/interactive/interactivePromptServer.ts index d54243445..342f134ad 100644 --- a/src/interactive/interactivePromptServer.ts +++ b/src/interactive/interactivePromptServer.ts @@ -17,7 +17,10 @@ * session is cleaned up, so nothing listens when no interactive run is active. */ -import { UserCancelError } from "../errors/UserCancelError"; +import { + PROMPT_CANCELLED_MESSAGE, + UserCancelError, +} from "../errors/UserCancelError"; // Minimal structural types for the slice of Node's `http` we use, declared // locally so this module never statically imports a Node builtin (keeps the @@ -426,7 +429,7 @@ class InteractivePromptServer { sessionId: string, requestId: string, value: unknown, - cancelled = false, + cancelled: unknown = false, ): boolean { const session = this.sessions.get(sessionId); if (!session) return false; @@ -436,8 +439,13 @@ class InteractivePromptServer { // A remote cancel must abort the run exactly like dismissing the Obsidian // modal: reject with UserCancelError so downstream abort handling classifies // it as a user cancellation (x-cancel, suppressed notice) rather than an error. - if (cancelled) - pending.reject(new UserCancelError("Input cancelled by user")); + // + // Strict `=== true`: the flag arrives from untrusted JSON, and a loose check + // meant any truthy value cancelled - so a client sending `"cancelled": "no"` + // killed the run, and a cancel beat a real `value` that was also present. + // The documented wire form has always been the literal `true`. + if (cancelled === true) + pending.reject(new UserCancelError(PROMPT_CANCELLED_MESSAGE)); else pending.resolve(value); return true; } diff --git a/src/interactive/promptProvider.test.ts b/src/interactive/promptProvider.test.ts index 159a50193..a83402fc3 100644 --- a/src/interactive/promptProvider.test.ts +++ b/src/interactive/promptProvider.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { RemotePromptProvider } from "./promptProvider"; import type { FieldRequirement } from "../preflight/RequirementCollector"; import type { interactivePromptServer } from "./interactivePromptServer"; +import { promptCancelled } from "../errors/UserCancelError"; type ServerLike = typeof interactivePromptServer; @@ -12,6 +13,19 @@ function fakeServer(answer: unknown): ServerLike { } as unknown as ServerLike; } +/** + * A server stub that REJECTS every emitPrompt, which is what a client cancel does + * (`submitReply(..., cancelled: true)`). The resolving stub above cannot express + * that, which is why nothing pinned cancel propagation through the provider before. + */ +function rejectingServer(error: unknown): ServerLike { + return { + emitPrompt: vi.fn(async () => { + throw error; + }), + } as unknown as ServerLike; +} + function dateField(id: string): FieldRequirement { return { id, label: id, type: "date" }; } @@ -178,3 +192,74 @@ describe("RemotePromptProvider suggester marshaling", () => { expect(result).toEqual(["custom-x"]); }); }); + +// --------------------------------------------------------------------------- +// Cancel propagation (#1574) +// --------------------------------------------------------------------------- + +describe("RemotePromptProvider cancel propagation", () => { + // A client cancel must abort the run exactly like dismissing the Obsidian modal. + // The file's own contract ("returns exactly what its in-app counterpart returns") + // only holds if this is true for EVERY method, so every method is covered. + const calls: Array<[string, (p: RemotePromptProvider) => Promise]> = [ + ["suggester", (p) => p.suggester(["a"], ["a"])], + ["suggesterMulti", (p) => p.suggesterMulti(["a"], ["a"])], + ["inputPrompt", (p) => p.inputPrompt("H")], + ["wideInputPrompt", (p) => p.wideInputPrompt("H")], + ["datePrompt", (p) => p.datePrompt("H")], + ["yesNoPrompt", (p) => p.yesNoPrompt("H")], + ["checkboxPrompt", (p) => p.checkboxPrompt(["a"])], + ["infoDialog", (p) => p.infoDialog("H", "t")], + ["requestInputs", (p) => p.requestInputs([dateField("d")])], + ]; + + for (const [name, call] of calls) { + it(`${name} propagates a client cancel as UserCancelError`, async () => { + const cancelled = promptCancelled(); + const provider = new RemotePromptProvider("s", rejectingServer(cancelled)); + await expect(call(provider)).rejects.toBe(cancelled); + }); + + it(`${name} propagates a session failure untouched`, async () => { + const failure = new Error("Interactive session ended"); + const provider = new RemotePromptProvider("s", rejectingServer(failure)); + await expect(call(provider)).rejects.toBe(failure); + }); + } +}); + +// --------------------------------------------------------------------------- +// confirm replies (#1574) +// --------------------------------------------------------------------------- + +describe("RemotePromptProvider yesNoPrompt reply validation", () => { + it.each([ + [true, true], + ["true", true], + [false, false], + ["false", false], + ])("accepts %o as %s", async (answer, expected) => { + const provider = new RemotePromptProvider("s", fakeServer(answer)); + expect(await provider.yesNoPrompt("H")).toBe(expected); + }); + + // In-app, `false` can ONLY come from clicking No. A reply that is neither a + // boolean nor a cancel used to be silently read as No, so the script took its + // else-branch on an answer the user never gave (#1574). + it.each([[null], [undefined], ["nope"], [0], [1], [{}], [[]]])( + "rejects %o instead of fabricating a No", + async (answer) => { + const provider = new RemotePromptProvider("s", fakeServer(answer)); + await expect(provider.yesNoPrompt("H")).rejects.toThrow( + /confirm prompt needs a boolean reply/, + ); + }, + ); + + it("names the documented cancel form in the error so a client can fix itself", async () => { + const provider = new RemotePromptProvider("s", fakeServer(null)); + await expect(provider.yesNoPrompt("H")).rejects.toThrow( + /\{"cancelled": true\}/, + ); + }); +}); diff --git a/src/interactive/promptProvider.ts b/src/interactive/promptProvider.ts index 3eee5df89..3449e9f00 100644 --- a/src/interactive/promptProvider.ts +++ b/src/interactive/promptProvider.ts @@ -7,6 +7,19 @@ * datePrompt / yesNoPrompt / checkboxPrompt / infoDialog. Each method returns * exactly what its in-app counterpart returns, so a script cannot tell it was * driven remotely. + * + * A dismissal is part of that contract, not an exception to it: a client replies + * `{"cancelled": true}` (see `submitReply`), the pending prompt rejects with + * `UserCancelError`, and the run aborts exactly as it does when the Obsidian modal + * is dismissed - same class, same message. `promptProvider.test.ts` pins that for + * every method. + * + * One genuine asymmetry, kept deliberately: cancelling an `info` prompt aborts the + * run, while `GenericInfoDialog` in-app has no reject path and can never abort + * anything. Removing it would take away a remote client's only way to bail out + * mid-run, so it stays and is documented at the wire (docs/.../Advanced/CLI.md) + * instead: a client that just wants to close an info panel should send a plain + * reply, not a cancel. */ import { formatISODate } from "../utils/dateParser"; @@ -23,6 +36,16 @@ import { */ const SUGGESTER_INDEX_PREFIX = "\u0000qa-idx:"; +/** A short, safe rendering of a bad reply for a protocol error message. */ +function describeReply(answer: unknown): string { + if (answer === undefined) return "no value"; + if (answer === null) return "null"; + if (typeof answer === "string") return JSON.stringify(answer.slice(0, 40)); + if (typeof answer === "number" || typeof answer === "boolean") + return String(answer); + return Array.isArray(answer) ? "an array" : typeof answer; +} + export interface PromptProvider { /** Batch multi-field prompt (`quickAddApi.requestInputs`). Returns id -> value. */ requestInputs(fields: FieldRequirement[]): Promise>; @@ -238,13 +261,34 @@ export class RemotePromptProvider implements PromptProvider { return formatted ?? iso; } + /** + * Yes/No is the one prompt where a malformed reply is indistinguishable from a + * real answer, so it is the one prompt that validates. + * + * In-app, `false` can only come from clicking No: dismissing resolves a + * three-state `null` that aborts the run. This used to collapse every non-`true` + * reply to `false`, so a client that omitted `value`, sent `null` (the shape + * #1574 proposed for a dismissal), or sent a typo silently answered "No" and the + * script walked its else-branch (#1574). A dismissal already has a wire + * representation - `{"cancelled": true}`, documented and rejecting with + * UserCancelError - so anything that is neither a boolean nor a cancel is a + * client bug, and failing loudly beats inventing an answer the user never gave. + * + * The other prompt types stay lenient on purpose: `""` and `[]` are answers a + * user really can give in-app (the Skip affordances, optional fields), so + * tightening them would break optional prompts on remote runs. + */ async yesNoPrompt(header: string, text?: string): Promise { const answer = await this.server.emitPrompt(this.sessionId, { type: "confirm", header, text, }); - return answer === true || answer === "true"; + if (answer === true || answer === "true") return true; + if (answer === false || answer === "false") return false; + throw new Error( + `A confirm prompt needs a boolean reply, or {"cancelled": true} if the user dismissed it. Got ${describeReply(answer)}.`, + ); } async checkboxPrompt( From 16a309c93150e5a89e825d88659dbd3b86323aa9 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 14:50:14 +0200 Subject: [PATCH 5/8] fix(interactive): validate a reply at the wire, where the client can still fix it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review found that the previous commit's strict `cancelled === true` traded one invented answer for another. Any non-literal flag fell through to `pending.resolve(value)`, and with no `value` present - which is exactly the shape a mistaken cancel has - `textPrompt` maps `undefined` to `""`, `requestInputs` to `{}`, `checkboxPrompt` to `[]`. So a client that meant "the user dismissed this" but wrote `"cancelled":"true"` got a run that completed with empty inputs and a `200 {"ok":true}` telling it everything was fine. That is the same harm the commit set out to remove, moved from `confirm.value` to the flag. Two reviewers found it independently. The fix is to validate at `/reply` instead of deeper in. There the client is still holding the HTTP response, so it gets a `400` naming the problem and the prompt stays PENDING - it can correct itself and POST again. Deeper in, the only options were to invent an answer or to fail the whole run with a message the client may never see: on the Template/Capture path a thrown message is replaced by "Choice execution failed; no file was created" before it reaches the poll stream, so the actionable sentence became a desktop notice nobody is looking at. That was the second blocker, and moving validation to the wire closes both. Two rules, both where a wrong answer is indistinguishable from a real one: a present-but-non-boolean `cancelled`, and a `confirm` whose value is not boolean-ish. Everything else stays lenient - `""` and `[]` are answers a user genuinely gives in-app via the Skip affordances and optional fields, so rejecting them would break optional prompts on remote runs. `submitWireReply` is the single path the HTTP layer and the tests share, so no test can pass through a shape production never sees. The provider-level throw stays as the backstop for non-HTTP callers, now documented as such rather than as the primary check. Live matrix over real loopback HTTP, one session per row: {"cancelled":true} -> 200, run ends "Input cancelled by user", nothing written {"value":true} -> 200, Yes branch {"value":false} -> 200, No branch {"value":null} -> 400 "…needs a boolean reply, or {"cancelled": true}…", then the corrected {"value":false} is accepted, 200 {"cancelled":"true"} -> 400 "…must be the literal true…", then corrected, 200 Also from the same review: - The unhandled-rejection reporter silenced every MacroAbortError. Only a USER cancellation is silenced now: ChoiceAbortError carries copy the user needs ("Selected folder not allowed."), and the rest of the plugin keeps the two apart everywhere it matters, so swallowing it left a floated involuntary abort with LESS signal than the console line it replaced. - The #1577 ratchet missed the shape most likely to reintroduce the bug - `rejectPromise(new Error("dismissed"))`, an Error `isCancellationError` cannot recognise. It now asserts the positive invariant (inside the prompt surfaces the only permitted rejection is `promptCancelled()`), strips comments before scanning so it stops matching prose about throws, catches a bare `throw "…"`, and has a guard-the-guard case so a moved directory cannot make it pass vacuously. Verified by hand that it now fails for the Error, bare-string and indirected-constant shapes. - The reporter's negative tests passed against an implementation that registered nothing: `listener?.()` on a null listener produces exactly the values they asserted. The listener is now required, and the "bounds the map" test - which asserted nothing about bounding - is replaced by two that pin the real collapse and eviction behaviour. - `choiceRename`, the one regression this PR identified, had no test: the whole suite stayed green with the fix reverted. Covered both ways, and verified the new cases fail when reverted. - `runTemplateFromFolder` was still stubbing the picker with the old bare string, so nothing fed a typed cancellation to a `return null` consumer. Both forms are covered now, plus the genuine-failure case. - CLI.md's new section contradicted itself (it promised a bad flag was "ignored", which was true for no prompt type after this change) and is rewritten around the 400. Refs #1574 #1576 #1577 --- docs/src/content/docs/docs/Advanced/CLI.md | 39 +++-- src/engine/runTemplateFromFolder.test.ts | 34 +++- src/gui/choiceRename.test.ts | 26 ++++ .../interactivePromptServer.test.ts | 129 ++++++++++++++- src/interactive/interactivePromptServer.ts | 147 ++++++++++++++---- src/interactive/promptProvider.ts | 9 +- src/utils/cancellationContract.test.ts | 83 ++++++++-- src/utils/unhandledRejectionReporter.test.ts | 50 +++++- src/utils/unhandledRejectionReporter.ts | 19 ++- 9 files changed, 455 insertions(+), 81 deletions(-) diff --git a/docs/src/content/docs/docs/Advanced/CLI.md b/docs/src/content/docs/docs/Advanced/CLI.md index 78d1281d2..fd46cbcdb 100644 --- a/docs/src/content/docs/docs/Advanced/CLI.md +++ b/docs/src/content/docs/docs/Advanced/CLI.md @@ -154,21 +154,30 @@ value (date fields use the `@date:ISO` format). The run's outcome arrives as the ### Cancelling `{"cancelled":true}` is how you say *the user dismissed this prompt*. It aborts -the run exactly as pressing Escape on the in-app dialog does. Two details worth -knowing: - -- The flag must be the literal `true`. Any other value (`"true"`, `1`, `"no"`) is - ignored and your `value` is used as a normal answer, so a typo cannot silently - kill a run. -- Do not use it just to close an `info` panel. `info` is the one prompt that - cannot be cancelled in the app - the dialog has no reject path - so cancelling - it remotely aborts a run that would have continued. Send a plain reply instead. - It stays cancellable because it is a client's only way to bail out mid-run. - -A `confirm` prompt is the one type that validates its reply: it needs `true` or -`false`. Anything else (a missing `value`, `null`, a typo) fails the prompt with a -protocol error rather than being read as "No" - the user never answered, so -QuickAdd will not invent an answer for them. +the run exactly as pressing Escape on the in-app dialog does. + +Do not use it just to close an `info` panel. `info` is the one prompt that cannot +be cancelled in the app - the dialog has no reject path - so cancelling it +remotely aborts a run that would have continued. Send a plain reply instead. It +stays cancellable because it is a client's only explicit way to bail out mid-run. + +### When a reply is rejected + +`/reply` answers `400` and leaves the prompt **pending** when it cannot honour +what you sent, so you can correct the reply and POST again. Two cases: + +- `cancelled` is present but is not a boolean (`"true"`, `1`, `"no"`). QuickAdd + will neither abort on a flag it does not recognise nor quietly answer the prompt + on the user's behalf, so it asks you to fix the flag. Use the literal `true`; + `false` and omitting it both mean "this is a real answer". +- a `confirm` reply whose `value` is not `true`/`false`. The user never answered, + and QuickAdd will not invent a "No" for them. + +Every other prompt type accepts whatever you send, including an empty answer: +`""` and `[]` are things a user genuinely submits in the app (the Skip buttons, +optional fields), so they must stay legal here too. + +A `409` means nothing was waiting on that `requestId`. Good to know: diff --git a/src/engine/runTemplateFromFolder.test.ts b/src/engine/runTemplateFromFolder.test.ts index a7c1c6071..cc66b33a5 100644 --- a/src/engine/runTemplateFromFolder.test.ts +++ b/src/engine/runTemplateFromFolder.test.ts @@ -4,6 +4,8 @@ import type QuickAdd from "../main"; import type { IChoiceExecutor } from "../IChoiceExecutor"; import type ITemplateChoice from "../types/choices/ITemplateChoice"; import { VALUE_SYNTAX } from "../constants"; +import { promptCancelled } from "../errors/UserCancelError"; +import { log } from "../logger/logManager"; vi.mock("../gui/GenericSuggester/genericSuggester", () => ({ default: { Suggest: vi.fn() }, @@ -171,10 +173,16 @@ describe("runTemplateFromFolder", () => { expect(choice.templatePath).toBe("Templates/Daily.md"); }); - it("does not execute when the picker is cancelled", async () => { + // The `isCancellationError -> return null` shape, driven with what the picker + // ACTUALLY rejects with since #1577. The legacy string is kept as a second case + // because a user script can still throw one and the contract honours that. + it.each([ + ["a typed dismissal", () => promptCancelled()], + ["a legacy sentinel a user script may throw", () => "no input given."], + ])("does not execute when the picker is cancelled with %s", async (_label, make) => { + const logError = vi.spyOn(log, "logError").mockImplementation(() => {}); const executor = createExecutor(); - // GenericSuggester rejects with this exact reason on dismissal. - suggestMock.mockRejectedValueOnce("no input given."); + suggestMock.mockRejectedValueOnce(make()); await runTemplateFromFolder( app, createPlugin({ @@ -184,6 +192,26 @@ describe("runTemplateFromFolder", () => { { choiceExecutor: executor }, ); expect(executor.execute).not.toHaveBeenCalled(); + // Quietly: backing out of the picker is not an error to report. + expect(logError).not.toHaveBeenCalled(); + logError.mockRestore(); + }); + + it("reports a genuine picker failure instead of failing silently", async () => { + const logError = vi.spyOn(log, "logError").mockImplementation(() => {}); + const executor = createExecutor(); + suggestMock.mockRejectedValueOnce(new Error("index unavailable")); + await runTemplateFromFolder( + app, + createPlugin({ + templateFolderPaths: ["Templates"], + templateFiles: [tfile("Templates/Daily.md")], + }), + { choiceExecutor: executor }, + ); + expect(executor.execute).not.toHaveBeenCalled(); + expect(logError).toHaveBeenCalledTimes(1); + logError.mockRestore(); }); }); diff --git a/src/gui/choiceRename.test.ts b/src/gui/choiceRename.test.ts index 856069986..486c8c7e5 100644 --- a/src/gui/choiceRename.test.ts +++ b/src/gui/choiceRename.test.ts @@ -19,6 +19,8 @@ vi.mock("./GenericInputPrompt/GenericInputPrompt", () => ({ import type { App } from "obsidian"; import { promptRenameChoice } from "./choiceRename"; +import { promptCancelled } from "../errors/UserCancelError"; +import { log } from "src/logger/logManager"; const app = {} as App; @@ -66,4 +68,28 @@ describe("promptRenameChoice", () => { null, ); }); + + // Since #1577 a dismissal IS an Error, so the previous `instanceof Error` gate + // would report every cancelled rename as a failure - a 15s error notice plus an + // error-log entry for pressing Escape. Nothing caught that, so pin both halves. + describe("dismissal versus failure", () => { + it("returns null and logs nothing when the user dismisses the prompt", async () => { + const logError = vi.spyOn(log, "logError").mockImplementation(() => {}); + promptSpy.mockRejectedValue(promptCancelled()); + + await expect(promptRenameChoice(app, "Old", "Macro")).resolves.toBeNull(); + expect(logError).not.toHaveBeenCalled(); + logError.mockRestore(); + }); + + it("returns null but DOES log when the prompt genuinely fails", async () => { + const logError = vi.spyOn(log, "logError").mockImplementation(() => {}); + promptSpy.mockRejectedValue(new Error("boom")); + + await expect(promptRenameChoice(app, "Old", "Macro")).resolves.toBeNull(); + expect(logError).toHaveBeenCalledTimes(1); + expect(String(logError.mock.calls[0]?.[0])).toContain("boom"); + logError.mockRestore(); + }); + }); }); diff --git a/src/interactive/interactivePromptServer.test.ts b/src/interactive/interactivePromptServer.test.ts index 474f53d98..417befe0c 100644 --- a/src/interactive/interactivePromptServer.test.ts +++ b/src/interactive/interactivePromptServer.test.ts @@ -3,9 +3,25 @@ import { interactivePromptServer, isLoopbackClient, safeEqual, + type ReplyBody, } from "./interactivePromptServer"; import { UserCancelError } from "../errors/UserCancelError"; +/** + * A reply as it arrives from a client: validated, then delivered. Goes through the + * same method the HTTP handler calls, so the validation under test is production's. + */ +function replyOverWire( + sessionId: string, + body: ReplyBody, +): { status: number; body: { error?: string } } { + const outcome = interactivePromptServer.submitWireReply(sessionId, body); + return outcome.ok + ? { status: 200, body: {} } + : { status: outcome.status, body: { error: outcome.error } }; +} + + afterEach(() => { vi.useRealTimers(); }); @@ -172,11 +188,13 @@ describe("interactivePromptServer session multiplexing", () => { interactivePromptServer.finish(s.id, { kind: "done", result: {} }); }); - // The flag arrives from untrusted JSON. A loose truthy check meant - // `"cancelled": "no"` killed the run, and a cancel beat a real `value` that was - // also present. The documented wire form has always been the literal `true`. - it.each([["no"], [1], [{}], ["false"], [[]]])( - "treats a non-true cancelled flag (%o) as a normal answer", + // The flag arrives from untrusted JSON. A loose truthy check let + // `"cancelled": "no"` kill a run; strict `=== true` alone would be worse - it would + // resolve a mistyped cancel as an EMPTY ANSWER (textPrompt maps undefined -> ""), + // handing the run a value the user never gave. So the wire rejects it and the prompt + // stays pending. Note the value-less shape, which is what a mistaken cancel has. + it.each([["no"], [1], [{}], ["true"], [[]], [null]])( + "rejects a non-boolean cancelled flag (%o) with 400 and keeps the prompt pending", async (flag) => { const s = interactivePromptServer.createSession(); const prompt = interactivePromptServer.emitPrompt(s.id, { @@ -184,15 +202,110 @@ describe("interactivePromptServer session multiplexing", () => { header: "Name", multiline: false, }); + let settled = false; + void prompt.then( + () => (settled = true), + () => (settled = true), + ); const rid = pendingRequestId(s.id); + + const outcome = replyOverWire(s.id, { requestId: rid, cancelled: flag }); + + expect(outcome.status).toBe(400); + expect(String(outcome.body.error)).toMatch(/cancelled/i); + await Promise.resolve(); + expect(settled, "the prompt must stay pending so the client can retry").toBe( + false, + ); + + // ...and the corrected reply still works. expect( - interactivePromptServer.submitReply(s.id, rid, "Ada", flag), - ).toBe(true); - await expect(prompt).resolves.toBe("Ada"); + replyOverWire(s.id, { requestId: rid, cancelled: true }).status, + ).toBe(200); + await expect(prompt).rejects.toBeInstanceOf(UserCancelError); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }, + ); + + it("accepts an explicit cancelled:false as a normal answer", async () => { + const s = interactivePromptServer.createSession(); + const prompt = interactivePromptServer.emitPrompt(s.id, { + type: "input", + header: "Name", + multiline: false, + }); + const rid = pendingRequestId(s.id); + expect( + replyOverWire(s.id, { requestId: rid, value: "Ada", cancelled: false }).status, + ).toBe(200); + await expect(prompt).resolves.toBe("Ada"); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }); + + // A confirm reply that is neither boolean nor a cancel is validated HERE, while the + // client is still holding the HTTP response, because deeper in the only options are + // to invent an answer or to fail the run with a message the Template/Capture path + // replaces with a generic sentence the client never sees. + it.each([[undefined], [null], ["yes"], [0], [{}]])( + "rejects a non-boolean confirm reply (%o) with 400", + async (value) => { + const s = interactivePromptServer.createSession(); + const prompt = interactivePromptServer.emitPrompt(s.id, { + type: "confirm", + header: "Proceed?", + }); + const rid = pendingRequestId(s.id); + + const outcome = replyOverWire(s.id, { requestId: rid, value }); + + expect(outcome.status).toBe(400); + expect(String(outcome.body.error)).toContain("confirm prompt needs a boolean"); + expect(String(outcome.body.error)).toContain('{"cancelled": true}'); + + expect(replyOverWire(s.id, { requestId: rid, value: false }).status).toBe(200); + await expect(prompt).resolves.toBe(false); interactivePromptServer.finish(s.id, { kind: "done", result: {} }); }, ); + it.each([[true], [false], ["true"], ["false"]])( + "accepts a boolean-ish confirm reply (%o)", + async (value) => { + const s = interactivePromptServer.createSession(); + const prompt = interactivePromptServer.emitPrompt(s.id, { + type: "confirm", + header: "Proceed?", + }); + const rid = pendingRequestId(s.id); + expect(replyOverWire(s.id, { requestId: rid, value }).status).toBe(200); + await expect(prompt).resolves.toBe(value); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }, + ); + + it("still 409s an unknown requestId", () => { + const s = interactivePromptServer.createSession(); + const outcome = replyOverWire(s.id, { requestId: "nope", value: "x" }); + expect(outcome.status).toBe(409); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }); + + // Leniency preserved on purpose: "" and [] are answers a user really gives in-app + // via the Skip affordances and optional fields, so an empty reply must stay legal or + // optional prompts break on remote runs. + it("leaves an empty answer to a non-confirm prompt alone", async () => { + const s = interactivePromptServer.createSession(); + const prompt = interactivePromptServer.emitPrompt(s.id, { + type: "input", + header: "Name", + multiline: false, + }); + const rid = pendingRequestId(s.id); + expect(replyOverWire(s.id, { requestId: rid, value: null }).status).toBe(200); + await expect(prompt).resolves.toBeNull(); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }); + it("caps the number of concurrent sessions", () => { const created: string[] = []; try { diff --git a/src/interactive/interactivePromptServer.ts b/src/interactive/interactivePromptServer.ts index 342f134ad..795d504b1 100644 --- a/src/interactive/interactivePromptServer.ts +++ b/src/interactive/interactivePromptServer.ts @@ -142,7 +142,12 @@ interface Session { waiterTimer: number | null; pending: Map< string, - { resolve: (value: unknown) => void; reject: (error: Error) => void } + { + resolve: (value: unknown) => void; + reject: (error: Error) => void; + /** What kind of reply this prompt accepts, for wire-level validation. */ + promptType: PromptSpec["type"]; + } >; finished: boolean; cleanupTimer: number | null; @@ -215,6 +220,67 @@ function randomId(): string { ); } +/** Body of a `POST /reply`. Every field is untrusted JSON, hence the `unknown`s. */ +export interface ReplyBody { + requestId?: string; + value?: unknown; + cancelled?: unknown; +} + +export type ReplyOutcome = + | { ok: true } + | { ok: false; status: 400 | 409; error: string }; + +const NO_PENDING_PROMPT = "No pending prompt for that requestId"; + +/** + * Why this reply cannot be honoured, or null if it can. + * + * Validating here rather than deeper in is what makes a malformed reply safe: the + * client is still waiting on the HTTP response, so it gets an actionable 400 and the + * prompt stays pending. Deeper in, the only options are to invent an answer (which + * is the bug this seam had) or to fail the whole run with a message the client may + * never see - the Template/Capture path replaces it with a generic sentence. + * + * Only two things are checked, both because a wrong answer there is + * indistinguishable from a real one. Empty answers stay legal everywhere else: + * `""` and `[]` are things a user genuinely submits in-app via the Skip + * affordances and optional fields, so rejecting them would break optional prompts + * on remote runs. + */ +function describeReplyProblem( + promptType: PromptSpec["type"], + body: ReplyBody, +): string | null { + if (body.cancelled !== undefined && typeof body.cancelled !== "boolean") { + return 'The "cancelled" flag must be the literal true (or omitted). A cancel that is not recognised would otherwise be answered on the user\'s behalf.'; + } + if (body.cancelled === true) return null; + + if (promptType === "confirm") { + const value = body.value; + const isBoolean = + value === true || + value === false || + value === "true" || + value === "false"; + if (!isBoolean) { + return `A confirm prompt needs a boolean reply, or {"cancelled": true} if the user dismissed it. Got ${describeValue(value)}.`; + } + } + return null; +} + +/** A short, safe rendering of a bad reply value for an error message. */ +function describeValue(value: unknown): string { + if (value === undefined) return "no value"; + if (value === null) return "null"; + if (typeof value === "string") return JSON.stringify(value.slice(0, 40)); + if (typeof value === "number" || typeof value === "boolean") + return String(value); + return Array.isArray(value) ? "an array" : typeof value; +} + class InteractivePromptServer { private server: HttpServer | null = null; private port = 0; @@ -308,7 +374,11 @@ class InteractivePromptServer { } const requestId = randomId(); return new Promise((resolve, reject) => { - session.pending.set(requestId, { resolve, reject }); + session.pending.set(requestId, { + resolve, + reject, + promptType: prompt.type, + }); this.push(session, { kind: "prompt", requestId, prompt }); }); } @@ -440,16 +510,24 @@ class InteractivePromptServer { // modal: reject with UserCancelError so downstream abort handling classifies // it as a user cancellation (x-cancel, suppressed notice) rather than an error. // - // Strict `=== true`: the flag arrives from untrusted JSON, and a loose check - // meant any truthy value cancelled - so a client sending `"cancelled": "no"` - // killed the run, and a cancel beat a real `value` that was also present. - // The documented wire form has always been the literal `true`. + // Strict `=== true`. A malformed flag must never reach here as a plain answer - + // `describeReplyProblem` turns it into a 400 at the wire, leaving the prompt + // pending - because resolving it would hand the run an empty answer the user + // never gave, which is worse than the spurious abort a loose truthy check caused. if (cancelled === true) pending.reject(new UserCancelError(PROMPT_CANCELLED_MESSAGE)); else pending.resolve(value); return true; } + /** The pending prompt's type, or undefined if nothing is waiting on that id. */ + private pendingPromptType( + session: Session, + requestId: string, + ): PromptSpec["type"] | undefined { + return session.pending.get(requestId)?.promptType; + } + private send(res: HttpServerResponse, status: number, body: unknown): void { const payload = JSON.stringify(body); res.writeHead(status, { @@ -495,20 +573,17 @@ class InteractivePromptServer { return; } if (req.method === "POST" && url.pathname === "/reply") { - const body = (await this.readBody(req)) as { - requestId?: string; - value?: unknown; - cancelled?: boolean; - }; - const accepted = this.handleReply(session, body); - // A missing/stale requestId matched no pending prompt: tell the - // client so it doesn't believe a blocked run was answered. - if (accepted) this.send(res, 200, { ok: true }); - else - this.send(res, 409, { - ok: false, - error: "No pending prompt for that requestId", - }); + const body = (await this.readBody(req)) as ReplyBody; + const outcome = this.submitWireReply(session.id, body); + if (outcome.ok) { + this.send(res, 200, { ok: true }); + } else { + // 400: the reply itself is malformed, and the prompt is STILL + // pending so the client can correct itself and try again. + // 409: nothing was waiting on that requestId, so the client + // doesn't believe a blocked run was answered. + this.send(res, outcome.status, { ok: false, error: outcome.error }); + } return; } @@ -572,17 +647,37 @@ class InteractivePromptServer { }); } - private handleReply( - session: Session, - body: { requestId?: string; value?: unknown; cancelled?: boolean }, - ): boolean { - if (!body.requestId) return false; - return this.submitReply( + /** + * Validate and deliver a reply that arrived over the wire. Public so the HTTP layer + * and tests share ONE path - the validation is the interesting part, and a test that + * bypassed it would be testing a shape production never sees. + */ + submitWireReply(sessionId: string, body: ReplyBody): ReplyOutcome { + const session = this.sessions.get(sessionId); + if (!session) { + return { ok: false, status: 409, error: NO_PENDING_PROMPT }; + } + if (!body.requestId) { + return { ok: false, status: 409, error: NO_PENDING_PROMPT }; + } + const promptType = this.pendingPromptType(session, body.requestId); + if (!promptType) { + return { ok: false, status: 409, error: NO_PENDING_PROMPT }; + } + // Validate BEFORE settling. A reply we cannot honour must leave the prompt + // pending and say why, never resolve it with something the user did not send. + const problem = describeReplyProblem(promptType, body); + if (problem) return { ok: false, status: 400, error: problem }; + + const accepted = this.submitReply( session.id, body.requestId, body.value, body.cancelled, ); + return accepted + ? { ok: true } + : { ok: false, status: 409, error: NO_PENDING_PROMPT }; } } diff --git a/src/interactive/promptProvider.ts b/src/interactive/promptProvider.ts index 3449e9f00..d4ff9e224 100644 --- a/src/interactive/promptProvider.ts +++ b/src/interactive/promptProvider.ts @@ -272,7 +272,14 @@ export class RemotePromptProvider implements PromptProvider { * script walked its else-branch (#1574). A dismissal already has a wire * representation - `{"cancelled": true}`, documented and rejecting with * UserCancelError - so anything that is neither a boolean nor a cancel is a - * client bug, and failing loudly beats inventing an answer the user never gave. + * client bug, and failing beats inventing an answer the user never gave. + * + * A live client never sees this throw: the same rule is enforced at `/reply` + * (`describeReplyProblem`), where a 400 reaches the client while it is still + * holding the response and the prompt stays pending. This is the backstop for + * every other caller of the provider, and the reason it is not the primary check + * is that on the Template/Capture path a thrown message is replaced by a generic + * sentence before the client ever polls for it. * * The other prompt types stay lenient on purpose: `""` and `[]` are answers a * user really can give in-app (the Skip affordances, optional fields), so diff --git a/src/utils/cancellationContract.test.ts b/src/utils/cancellationContract.test.ts index e2f6bac8f..23e4a30ef 100644 --- a/src/utils/cancellationContract.test.ts +++ b/src/utils/cancellationContract.test.ts @@ -34,34 +34,76 @@ function walk(dir: string, out: string[] = []): string[] { } /** - * A reject/throw whose argument is a bare string literal. Deliberately narrow: - * it catches the exact shape every prompt used before #1577 and would use again - * by copy-paste, without flagging `new Error("...")`. + * Comments talk ABOUT the shapes below ("would throw \"Unknown location\""), so scanning + * raw text produces false positives. Blank them, preserving offsets so reported line + * numbers still point at the real line. */ -const BARE_STRING_REJECTION = - /\b(?:reject|rejectPromise|rejectPromise!)\s*\(\s*(["'`])/g; +function stripComments(text: string): string { + return text + .replace(/\/\*[\s\S]*?\*\//g, (m) => m.replace(/[^\n]/g, " ")) + .replace(/(^|[^:])\/\/[^\n]*/g, (m, lead: string) => lead + " ".repeat(m.length - lead.length)); +} + +/** + * Any `reject(...)` / `rejectPromise(...)` CALL, capturing the rest of the line. + * Matching the call rather than a bare-string argument is deliberate: the shape that + * would actually reintroduce #1577 is not only `reject("dismissed")` but also + * `reject(new Error("dismissed"))` - an Error `isCancellationError` cannot recognise, + * so a dismissal reads as a failure again with nothing to catch it. + */ +const REJECT_CALL = /\b(?:reject|rejectPromise)!?\??\s*\(\s*([^\n]*)/g; +/** A `throw` of a string literal, i.e. a bare-string rejection inside an async method. */ +const THROW_STRING = /\bthrow\s+["'`]/g; + +/** Directories whose modules are prompt surfaces: a rejection there means a dismissal. */ +const PROMPT_DIRS = [join(SRC, "gui"), join(SRC, "preflight")]; describe("cancellation contract (#1577)", () => { - it("no source file rejects a promise with a bare string literal", () => { + // The positive invariant, not a negative syntax shape: inside the prompt + // surfaces, the ONLY thing a promise may be rejected with is promptCancelled(). + it("every prompt rejection is promptCancelled()", () => { + const offenders: string[] = []; + for (const dir of PROMPT_DIRS) { + for (const file of walk(dir)) { + const text = stripComments(readFileSync(file, "utf8")); + for (const match of text.matchAll(REJECT_CALL)) { + const arg = (match[1] ?? "").trim(); + // `(resolve, reject) => {` etc: the binding, not a call on it. + if (arg === "" || arg.startsWith(")")) continue; + if (arg.startsWith("promptCancelled(")) continue; + const line = text.slice(0, match.index).split("\n").length; + offenders.push( + `${file.slice(SRC.length + 1)}:${line} — ${match[0].trim().slice(0, 60)}`, + ); + } + } + } + expect( + offenders, + "A dismissed prompt must reject with promptCancelled(). A bare string is " + + "unrecognisable and untraceable; a plain Error reads as a genuine failure.", + ).toEqual([]); + }); + + it("no source file throws a bare string literal", () => { const offenders: string[] = []; for (const file of walk(SRC)) { - const text = readFileSync(file, "utf8"); - for (const match of text.matchAll(BARE_STRING_REJECTION)) { + const text = stripComments(readFileSync(file, "utf8")); + for (const match of text.matchAll(THROW_STRING)) { const line = text.slice(0, match.index).split("\n").length; - offenders.push( - `${file.slice(SRC.length + 1)}:${line} — ${match[0].trim()}…`, - ); + offenders.push(`${file.slice(SRC.length + 1)}:${line} — ${match[0].trim()}…`); } } expect( offenders, - "A dismissed prompt must reject with promptCancelled(), not a string. " + - "For a genuine failure, reject with an Error so it carries a stack.", + "Throw an Error so the value carries a stack and can be classified.", ).toEqual([]); }); it("no source file re-introduces a legacy cancellation sentinel", () => { - const sentinels = ['"No input given."', '"no input given."']; + // Unquoted so single-quoted and template forms count too, and including + // "cancelled" - the sentinel OnePageInputModal actually used. + const sentinels = ["No input given.", "no input given."]; const offenders: string[] = []; for (const file of walk(SRC)) { // errorUtils owns the compatibility list by design. @@ -76,6 +118,19 @@ describe("cancellation contract (#1577)", () => { expect(offenders).toEqual([]); }); + // Guards the guard: if the walk stops finding files (a moved directory, a changed + // extension filter), every scan above passes vacuously. + it("the scan actually reads the prompt sources", () => { + const scanned = PROMPT_DIRS.flatMap((dir) => walk(dir)); + expect(scanned.length).toBeGreaterThan(50); + expect( + scanned.some((f) => f.endsWith(join("GenericSuggester", "genericSuggester.ts"))), + ).toBe(true); + expect( + scanned.some((f) => f.endsWith(join("preflight", "OnePageInputModal.ts"))), + ).toBe(true); + }); + it("recognises a typed prompt dismissal", () => { expect(isCancellationError(promptCancelled())).toBe(true); expect(isCancellationError(new UserCancelError("anything"))).toBe(true); diff --git a/src/utils/unhandledRejectionReporter.test.ts b/src/utils/unhandledRejectionReporter.test.ts index b0ef006b9..304d436f9 100644 --- a/src/utils/unhandledRejectionReporter.test.ts +++ b/src/utils/unhandledRejectionReporter.test.ts @@ -1,6 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { registerUnhandledRejectionReporter } from "./unhandledRejectionReporter"; -import { MacroAbortError } from "../errors/MacroAbortError"; +import { ChoiceAbortError } from "../errors/ChoiceAbortError"; import { promptCancelled } from "../errors/UserCancelError"; import { log } from "../logger/logManager"; @@ -40,10 +40,16 @@ describe("unhandled rejection reporter (#1576)", () => { }, }; - /** A minimal stand-in for PromiseRejectionEvent (jsdom's needs a real promise). */ + /** + * A minimal stand-in for PromiseRejectionEvent (jsdom's needs a real promise). + * Non-optional call on purpose: if the reporter ever stops registering a listener, + * the negative tests below would otherwise pass vacuously - a dead listener + * produces exactly the values they assert. + */ function fire(reason: unknown) { + if (!listener) throw new Error("no unhandledrejection listener was registered"); let defaultPrevented = false; - listener?.({ + listener({ reason, preventDefault() { defaultPrevented = true; @@ -57,6 +63,7 @@ describe("unhandled rejection reporter (#1576)", () => { clock = 1_000_000; logError = vi.spyOn(log, "logError").mockImplementation(() => {}); registerUnhandledRejectionReporter(host, () => clock); + expect(listener, "the reporter must register a listener").not.toBeNull(); }); afterEach(() => { @@ -96,13 +103,20 @@ describe("unhandled rejection reporter (#1576)", () => { expect(logError).not.toHaveBeenCalled(); }); - it("silences a deliberate abort", () => { - const abort = new MacroAbortError("target file missing"); + // Not every MacroAbortError is a user cancellation. ChoiceAbortError carries copy + // the user needs ("Selected folder not allowed."), and the rest of the plugin keeps + // the two apart - so swallowing it here would leave a floated involuntary abort with + // LESS signal than the console line this replaces. + it("reports an involuntary abort, which is not a cancellation", () => { + const abort = new ChoiceAbortError("Selected folder not allowed."); abort.stack = "MacroAbortError: x\n at a (plugin:quickadd:1:1)"; fire(abort); - expect(logError).not.toHaveBeenCalled(); + expect(logError).toHaveBeenCalledTimes(1); + expect(String(logError.mock.calls[0]?.[0])).toContain( + "Selected folder not allowed.", + ); }); it("reports a repeated failure once per window, not once per occurrence", () => { @@ -135,9 +149,29 @@ describe("unhandled rejection reporter (#1576)", () => { expect(logError).toHaveBeenCalledTimes(2); }); - it("bounds the dedupe map without wrongly suppressing distinct sites", () => { + it("reports every distinct throw site, however many there are", () => { for (let i = 0; i < 200; i++) fire(pluginError("boom", `site${i}`)); - // Every one is a distinct throw site, so every one is a distinct bug. expect(logError).toHaveBeenCalledTimes(200); }); + + // The map is bounded (MAX_TRACKED), so past that many LIVE sites the "one report + // per site per window" guarantee degrades to reporting again. That is the deliberate + // trade against unbounded growth; pin it so the behaviour is a decision, not a + // surprise. Fewer sites than the bound must still collapse within the window. + it("keeps collapsing a repeat while the site is still tracked", () => { + for (let i = 0; i < 10; i++) fire(pluginError("boom", `site${i}`)); + expect(logError).toHaveBeenCalledTimes(10); + + fire(pluginError("boom", "site0")); + expect(logError).toHaveBeenCalledTimes(10); + }); + + it("evicts under pressure rather than growing without limit", () => { + // Far more live sites than the bound; the oldest entries get dropped, so an + // early site is reported a second time instead of being remembered forever. + for (let i = 0; i < 200; i++) fire(pluginError("boom", `site${i}`)); + logError.mockClear(); + fire(pluginError("boom", "site0")); + expect(logError).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/utils/unhandledRejectionReporter.ts b/src/utils/unhandledRejectionReporter.ts index d04ec2b9c..7bebb46fe 100644 --- a/src/utils/unhandledRejectionReporter.ts +++ b/src/utils/unhandledRejectionReporter.ts @@ -1,4 +1,3 @@ -import { MacroAbortError } from "../errors/MacroAbortError"; import { isCancellationError, reportError } from "./errorUtils"; /** @@ -88,11 +87,19 @@ export function registerUnhandledRejectionReporter( // Ours, so stop the console noise either way. event.preventDefault(); - // A dismissed prompt or a deliberate abort is not a failure. It reaches here - // when a handler floats a prompt (e.g. the macro editor's script picker), and - // telling the user "an error occurred" because they pressed Escape would be - // worse than the silence this replaces. - if (reason instanceof MacroAbortError || isCancellationError(reason)) return; + // A dismissed prompt is not a failure. It reaches here when a handler floats a + // prompt (e.g. the macro editor's script picker), and telling the user "an + // error occurred" because they pressed Escape would be worse than the silence + // this replaces. + // + // Only a USER cancellation is silenced, not every MacroAbortError. Its other + // subclass, ChoiceAbortError, is how QuickAdd reports involuntary aborts that + // carry copy the user needs ("Selected folder not allowed.", "…re-run with the + // ui flag."), and the rest of the plugin keeps the two apart everywhere it + // matters - choiceExecutor's cancelKind, macroAbortHandler's notice + // suppression, x-cancel vs x-error. Swallowing those here would leave a floated + // involuntary abort with LESS signal than the console line it replaces. + if (isCancellationError(reason)) return; const error = reason as Error; const key = dedupeKey(error, pluginId); From c63f35cd80e56d94428897dedfc2a9890e6def73 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 14:56:33 +0200 Subject: [PATCH 6/8] fix: make the rejection reporter's guarantees hold, and stop overclaiming them Second adversarial pass, all four findings verified live in the isolated vault. - The dedupe window restarted on every REPORT instead of sliding on every OCCURRENCE, so a continuously failing site raised a 15-second notice every 10 seconds forever - six a minute, worse than the silence it replaced. The timestamp is now stamped on each occurrence, so a sustained failure reports once and goes quiet until it stops and recurs. Verified: five rapid occurrences -> one notice. - `preventDefault()` ran before the report decision, so a SUPPRESSED repeat lost the browser's unhandled-rejection line as well as the notice - strictly less evidence than master for occurrences 2..n. It now only claims the event when it actually handles it (report, or a silenced cancellation). - `plugin:quickadd` is a prefix of `plugin:quickadd-beta`, so an undelimited match claimed a fork's or beta's rejections and suppressed their only console line. Matching `plugin::` fixes both this and the dedupe key. Verified: a `plugin:quickadd-beta` rejection now produces nothing. - The eviction path could drop the key it had just stamped, which would make the currently-failing site report on every occurrence. It skips that key now. Also: the debounced settings write floated `saveData()` from a NON-async arrow, so the rejection's stack carried no QuickAdd frame at all (it is constructed inside Obsidian's FS plumbing after an await) and the reporter could not attribute it. That is the one float where the user most needs to hear about a failure - their settings silently did not persist. Awaiting inside an async IIFE puts a QuickAdd frame on the stack. Verified: a throwing `saveData` now raises "A QuickAdd action failed: disk full", where before it produced nothing. Three comments corrected, because a comment that justifies a decision on a false premise is worse than no comment: - `rethrowPromptError` claimed every path reports a propagating error "exactly once". It is at least once: measured on the real command-palette path, a user script error raises TWO notices (MacroChoiceEngine reports and rethrows into main.ts, which reports again). That is pre-existing, is now filed separately, and it strengthens rather than weakens the reason not to add a third report here. My earlier verification of #1575 said "exactly one notice" - it went through `api.executeChoice`, which skips main.ts's handler. Corrected. - `previewDiagnostics`' message fallback does not cover a `toError(err, context)` wrap (the context prefix matches no sentinel). What it covers is a user script throwing an Error whose message IS a legacy sentinel. Said so, so nobody re-attempts the reverted commit. - The sentinel scan's comment claimed it included "cancelled"; it does not, and should not - as a bare substring it is far too common in src/. The reject-call invariant is what guards that modal. Refs #1576 --- src/formatters/previewDiagnostics.ts | 7 ++- src/main.ts | 16 ++++-- src/quickAddApi.ts | 12 +++-- src/utils/cancellationContract.test.ts | 6 ++- src/utils/unhandledRejectionReporter.test.ts | 55 +++++++++++++++++++- src/utils/unhandledRejectionReporter.ts | 48 +++++++++++------ 6 files changed, 115 insertions(+), 29 deletions(-) diff --git a/src/formatters/previewDiagnostics.ts b/src/formatters/previewDiagnostics.ts index ad11fda32..6c2b6b272 100644 --- a/src/formatters/previewDiagnostics.ts +++ b/src/formatters/previewDiagnostics.ts @@ -80,7 +80,12 @@ export function describePreviewFailure(error: unknown): string | null { if (isCancellationError(error)) return null; if (!(error instanceof Error)) return PREVIEW_FAILED_MESSAGE; const message = error.message ?? ""; - // Also covers a cancellation that was wrapped in an Error on the way up. + // Covers a USER SCRIPT that throws an Error whose message is one of the legacy + // sentinels (see `isCancellationError`) - the shape a script written against the + // pre-#1577 strings still uses. It deliberately does NOT cover + // `toError(err, context)`, which prefixes the context and so matches no sentinel; + // adding the canonical message to that set would not fix it either, and nothing + // wraps on the way into this function. if (isCancellationError(message)) return null; // By far the most frequent throw: the token autocomplete inserts `{{VALUE:}}` // with the caret between the colon and the braces, and VARIABLE_REGEX matches diff --git a/src/main.ts b/src/main.ts index 2a13b4a59..2c465d772 100644 --- a/src/main.ts +++ b/src/main.ts @@ -85,10 +85,18 @@ export default class QuickAdd extends Plugin { private unsubscribeSettingsStore: () => void; // Debounced disk write for the store subscriber. saveSettings() stays immediate // (migrations await it) and cancels this; onunload flushes it. - private requestSave: Debouncer<[], void> = debounce( - () => void this.saveData(this.settings), - SETTINGS_SAVE_DEBOUNCE_MS, - ); + // + // The async IIFE is load-bearing, not style: floating `saveData()` straight from a + // non-async arrow leaves NO QuickAdd frame on the rejection's stack (it is + // constructed inside Obsidian's own FS plumbing, after an await), so a failed + // settings write would be invisible to the unhandled-rejection reporter - which is + // the one failure here the user most needs to hear about, since their settings + // silently did not persist. Awaiting inside a QuickAdd frame puts us on the stack. + private requestSave: Debouncer<[], void> = debounce(() => { + void (async () => { + await this.saveData(this.settings); + })(); + }, SETTINGS_SAVE_DEBOUNCE_MS); get api(): ReturnType { return QuickAddApi.GetApi( diff --git a/src/quickAddApi.ts b/src/quickAddApi.ts index 52aec1765..324533ece 100644 --- a/src/quickAddApi.ts +++ b/src/quickAddApi.ts @@ -1034,10 +1034,14 @@ export class QuickAddApi { * `never` makes that omission a compile error rather than a silent one. * * Not reported here on purpose: every path that runs a script already reports a - * propagating error exactly once (MacroChoiceEngine for user scripts, - * TemplateChoiceEngine for inline scripts, main.ts / choiceSuggester at the top), - * and `reportError` raises a 15-second Notice, so reporting here too would stack - * two or three notices for one failure. + * propagating error (MacroChoiceEngine for user scripts, TemplateChoiceEngine for + * inline scripts, main.ts / choiceSuggester at the top), and `reportError` raises a + * 15-second Notice - so reporting here too would stack another one. + * + * "Already reports" is at least once, not exactly once: on the command-palette macro + * path MacroChoiceEngine reports and rethrows into main.ts, which reports again, so a + * user script error raises TWO notices today. That is pre-existing and tracked + * separately; adding a third here would make it worse, not better. */ function rethrowPromptError(error: unknown): never { if (error instanceof MacroAbortError) { diff --git a/src/utils/cancellationContract.test.ts b/src/utils/cancellationContract.test.ts index 23e4a30ef..49ba1b349 100644 --- a/src/utils/cancellationContract.test.ts +++ b/src/utils/cancellationContract.test.ts @@ -101,8 +101,10 @@ describe("cancellation contract (#1577)", () => { }); it("no source file re-introduces a legacy cancellation sentinel", () => { - // Unquoted so single-quoted and template forms count too, and including - // "cancelled" - the sentinel OnePageInputModal actually used. + // Unquoted so single-quoted and template forms count too. OnePageInputModal's + // old sentinel, "cancelled", is deliberately absent: as a bare substring it is + // far too common a word in src/ to scan for. The reject-call invariant above is + // what actually guards that modal. const sentinels = ["No input given.", "no input given."]; const offenders: string[] = []; for (const file of walk(SRC)) { diff --git a/src/utils/unhandledRejectionReporter.test.ts b/src/utils/unhandledRejectionReporter.test.ts index 304d436f9..96eea7df6 100644 --- a/src/utils/unhandledRejectionReporter.test.ts +++ b/src/utils/unhandledRejectionReporter.test.ts @@ -127,12 +127,40 @@ describe("unhandled rejection reporter (#1576)", () => { clock += 9_000; fire(pluginError("validator exploded")); expect(logError).toHaveBeenCalledTimes(1); + }); - clock += 2_000; - fire(pluginError("validator exploded")); + // The window SLIDES on occurrences, it does not restart on reports. Stamping only + // on report would let a continuously failing site raise a notice every 10 seconds + // indefinitely - six 15-second notices a minute, worse than the silence it replaced. + it("stays quiet while a failure keeps recurring inside the window", () => { + fire(pluginError("stuck loop")); + expect(logError).toHaveBeenCalledTimes(1); + + // Failing every 9s for two minutes: still one report. + for (let i = 0; i < 14; i++) { + clock += 9_000; + fire(pluginError("stuck loop")); + } + expect(logError).toHaveBeenCalledTimes(1); + }); + + it("reports again once the failure has stopped and recurs", () => { + fire(pluginError("intermittent")); + expect(logError).toHaveBeenCalledTimes(1); + + clock += 11_000; // quiet for longer than the window + fire(pluginError("intermittent")); expect(logError).toHaveBeenCalledTimes(2); }); + // Silencing the notice must never leave an occurrence with LESS evidence than + // before this reporter existed, so a suppressed repeat keeps the browser's own + // unhandled-rejection line. + it("leaves the browser default alone for a suppressed repeat", () => { + expect(fire(pluginError("noisy")).defaultPrevented).toBe(true); + expect(fire(pluginError("noisy")).defaultPrevented).toBe(false); + }); + it("collapses one broken loop that fails on many different values", () => { // Same throw site, message embeds the note path: one bug, one notice. for (let i = 0; i < 200; i++) { @@ -141,6 +169,18 @@ describe("unhandled rejection reporter (#1576)", () => { expect(logError).toHaveBeenCalledTimes(1); }); + // `plugin:quickadd` is a PREFIX of `plugin:quickadd-beta`, so an undelimited match + // would claim a fork's rejections and suppress their only console line. + it("does not claim a plugin whose id merely starts with ours", () => { + const error = new Error("beta bug"); + error.stack = "Error: beta bug\n at f (plugin:quickadd-beta:1:1)"; + + const { defaultPrevented } = fire(error); + + expect(defaultPrevented).toBe(false); + expect(logError).not.toHaveBeenCalled(); + }); + it("does not let one noisy failure mask a different one", () => { fire(pluginError("first", "siteA")); fire(pluginError("second", "siteB")); @@ -174,4 +214,15 @@ describe("unhandled rejection reporter (#1576)", () => { fire(pluginError("boom", "site0")); expect(logError).toHaveBeenCalledTimes(1); }); + + // Eviction must never drop the key it just stamped, or the site currently failing + // would report on every single occurrence. + it("does not evict the site it is currently handling", () => { + for (let i = 0; i < 200; i++) fire(pluginError("boom", `site${i}`)); + logError.mockClear(); + fire(pluginError("boom", "hot")); + expect(logError).toHaveBeenCalledTimes(1); + fire(pluginError("boom", "hot")); + expect(logError).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/utils/unhandledRejectionReporter.ts b/src/utils/unhandledRejectionReporter.ts index 7bebb46fe..0537d8347 100644 --- a/src/utils/unhandledRejectionReporter.ts +++ b/src/utils/unhandledRejectionReporter.ts @@ -47,7 +47,7 @@ export interface UnhandledRejectionReporterHost { function dedupeKey(error: Error, pluginId: string): string { const frame = (error.stack ?? "") .split("\n") - .find((line) => line.includes(`plugin:${pluginId}`)); + .find((line) => line.includes(`plugin:${pluginId}:`)); return `${error.name}@${frame?.trim() ?? error.message}`; } @@ -64,7 +64,10 @@ function isFromPlugin(reason: unknown, pluginId: string): boolean { return ( reason instanceof Error && typeof reason.stack === "string" && - reason.stack.includes(`plugin:${pluginId}`) + // Trailing colon: the frame is `plugin:::`, so an undelimited + // match would claim `plugin:quickadd-beta` - a fork's or beta's rejections, + // whose only console line we would then suppress on their behalf. + reason.stack.includes(`plugin:${pluginId}:`) ); } @@ -78,15 +81,12 @@ export function registerUnhandledRejectionReporter( now: () => number = Date.now, ): void { const pluginId = plugin.manifest.id; - const recentlyReported = new Map(); + const recentlySeen = new Map(); plugin.registerDomEvent(window, "unhandledrejection", (event) => { const reason = event.reason; if (!isFromPlugin(reason, pluginId)) return; - // Ours, so stop the console noise either way. - event.preventDefault(); - // A dismissed prompt is not a failure. It reaches here when a handler floats a // prompt (e.g. the macro editor's script picker), and telling the user "an // error occurred" because they pressed Escape would be worse than the silence @@ -99,26 +99,42 @@ export function registerUnhandledRejectionReporter( // matters - choiceExecutor's cancelKind, macroAbortHandler's notice // suppression, x-cancel vs x-error. Swallowing those here would leave a floated // involuntary abort with LESS signal than the console line it replaces. - if (isCancellationError(reason)) return; + if (isCancellationError(reason)) { + // Claimed: a dismissal is not something the console should shout about. + event.preventDefault(); + return; + } const error = reason as Error; const key = dedupeKey(error, pluginId); const at = now(); - const last = recentlyReported.get(key); - if (last !== undefined && at - last < REPEAT_WINDOW_MS) return; + const last = recentlySeen.get(key); + // Stamp every OCCURRENCE, not every report. Stamping only on report restarts + // the window each time, so a site failing continuously would raise a notice + // every REPEAT_WINDOW_MS forever; stamping here makes the window slide, so a + // sustained failure reports once and then stays quiet until it stops and recurs. + recentlySeen.set(key, at); + if (last !== undefined && at - last < REPEAT_WINDOW_MS) { + // Deliberately NOT preventDefault: a suppressed repeat keeps the browser's + // own unhandled-rejection line, so silencing the notice never leaves the + // occurrence with less evidence than before this reporter existed. + return; + } - if (recentlyReported.size >= MAX_TRACKED) { - for (const [tracked, seenAt] of recentlyReported) { - if (at - seenAt >= REPEAT_WINDOW_MS) recentlyReported.delete(tracked); + if (recentlySeen.size >= MAX_TRACKED) { + for (const [tracked, seenAt] of recentlySeen) { + if (at - seenAt >= REPEAT_WINDOW_MS) recentlySeen.delete(tracked); } // Still full: every entry is fresh, so drop the oldest to make room. - if (recentlyReported.size >= MAX_TRACKED) { - const oldest = recentlyReported.keys().next(); - if (!oldest.done) recentlyReported.delete(oldest.value); + if (recentlySeen.size >= MAX_TRACKED) { + const oldest = recentlySeen.keys().next(); + if (!oldest.done && oldest.value !== key) { + recentlySeen.delete(oldest.value); + } } } - recentlyReported.set(key, at); + event.preventDefault(); reportError(error, "A QuickAdd action failed"); }); } From 83cd6b95b4dd0b592ccb2d40e7c0f5222d8858e8 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 15:16:10 +0200 Subject: [PATCH 7/8] docs: mark the new prompt error contracts as unreleased Docs deploy from master immediately while plugin releases are cut manually, so a page describing behaviour nobody has installed yet misleads current users - AGENTS.md requires an unreleased marker for exactly that window. Flagged in review on #1606. The two windows that matter: the interactive seam's 400-and-retry semantics (the old server consumed `cancelled: "true"` as a cancellation and read a value-less confirm as "No"), and the script API's two-outcome contract (a non-cancellation failure used to resolve `undefined`). --- docs/src/content/docs/docs/Advanced/CLI.md | 6 ++++++ docs/src/content/docs/docs/QuickAddAPI.md | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/docs/src/content/docs/docs/Advanced/CLI.md b/docs/src/content/docs/docs/Advanced/CLI.md index fd46cbcdb..a0c80bbfa 100644 --- a/docs/src/content/docs/docs/Advanced/CLI.md +++ b/docs/src/content/docs/docs/Advanced/CLI.md @@ -163,6 +163,12 @@ stays cancellable because it is a client's only explicit way to bail out mid-run ### When a reply is rejected +:::note[Available in the next release] +The `400`-and-retry semantics below are new. Before them, a `cancelled` flag that was not the +literal `true` was consumed as a cancellation, and a `confirm` prompt with no value was read as +"No". +::: + `/reply` answers `400` and leaves the prompt **pending** when it cannot honour what you sent, so you can correct the reply and POST again. Two cases: diff --git a/docs/src/content/docs/docs/QuickAddAPI.md b/docs/src/content/docs/docs/QuickAddAPI.md index 3c6d8f843..8e91917b9 100644 --- a/docs/src/content/docs/docs/QuickAddAPI.md +++ b/docs/src/content/docs/docs/QuickAddAPI.md @@ -936,6 +936,11 @@ module.exports = async (params) => { ## Error Handling Best Practices +:::note[Available in the next release] +Until then, a prompt that failed for a reason other than the user cancelling resolved +`undefined` instead of rejecting, so a script could not tell a failure from an empty answer. +::: + A prompt has exactly two non-happy outcomes, and they are different things: - **The user dismissed it** (Escape, Cancel, closing the dialog). The promise rejects with `MacroAbortError("Input cancelled by user")`. Let it bubble and QuickAdd stops the run quietly - which is almost always what you want. From 69f600add3fafbfa44f216f58720bc150be26467 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 15:27:25 +0200 Subject: [PATCH 8/8] test: make the cancellation ratchet see a multiline rejection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reject-call pattern was line-bounded, so an argument split across lines captured as EMPTY - and the empty capture is the exemption for an executor binding (`new Promise((resolve, reject) => …)`), so a multiline `rejectPromise(\n "dismissed",\n)` was waved straight through the guard that exists to catch it. Flagged in review on #1606. The pattern now spans lines and stops at the closing paren, the exemption keys on the binding shape rather than on emptiness, and it accepts optional-call syntax (`rejectPromise?.(…)`), which it also silently missed. Guard the guard: the detector is extracted from the directory walk and tested against the seven shapes that would reintroduce #1577 (bare string, unrecognisable Error, indirected constant, optional call, raw binding, and both multiline forms) plus the four it must allow (the contract, the contract multiline, an executor binding, and a comment about the old shape). A ratchet that silently stops matching is worse than no ratchet, and this one had already been wrong twice. --- src/utils/cancellationContract.test.ts | 80 +++++++++++++++++++++----- 1 file changed, 65 insertions(+), 15 deletions(-) diff --git a/src/utils/cancellationContract.test.ts b/src/utils/cancellationContract.test.ts index 49ba1b349..2d289e4c8 100644 --- a/src/utils/cancellationContract.test.ts +++ b/src/utils/cancellationContract.test.ts @@ -45,19 +45,41 @@ function stripComments(text: string): string { } /** - * Any `reject(...)` / `rejectPromise(...)` CALL, capturing the rest of the line. - * Matching the call rather than a bare-string argument is deliberate: the shape that + * Any `reject(...)` / `rejectPromise(...)` CALL, capturing its argument up to the first + * `)`. Matching the call rather than a bare-string argument is deliberate: the shape that * would actually reintroduce #1577 is not only `reject("dismissed")` but also * `reject(new Error("dismissed"))` - an Error `isCancellationError` cannot recognise, * so a dismissal reads as a failure again with nothing to catch it. + * + * `[\s\S]` rather than `[^\n]`: an argument split across lines would otherwise capture + * as empty and be waved through as an executor binding. */ -const REJECT_CALL = /\b(?:reject|rejectPromise)!?\??\s*\(\s*([^\n]*)/g; +const REJECT_CALL = /\b(?:reject|rejectPromise)(?:!|\?\.|\?)?\s*\(\s*([\s\S]*?)\)/g; /** A `throw` of a string literal, i.e. a bare-string rejection inside an async method. */ const THROW_STRING = /\bthrow\s+["'`]/g; /** Directories whose modules are prompt surfaces: a rejection there means a dismissal. */ const PROMPT_DIRS = [join(SRC, "gui"), join(SRC, "preflight")]; +/** + * Every rejection in `text` that is not `promptCancelled()`, as `line — snippet`. + * Extracted from the directory scan so the detector itself is testable: this guard is + * load-bearing, and a guard that silently stops matching is worse than none. + */ +export function findNonCancelledRejections(text: string): string[] { + const found: string[] = []; + const stripped = stripComments(text); + for (const match of stripped.matchAll(REJECT_CALL)) { + const arg = (match[1] ?? "").trim(); + // `new Promise((resolve, reject) => …)`: the binding, not a call on it. + if (arg === "" || arg.startsWith("=>")) continue; + if (arg.startsWith("promptCancelled(")) continue; + const line = stripped.slice(0, match.index).split("\n").length; + found.push(`${line} — ${match[0].trim().replace(/\s+/g, " ").slice(0, 60)}`); + } + return found; +} + describe("cancellation contract (#1577)", () => { // The positive invariant, not a negative syntax shape: inside the prompt // surfaces, the ONLY thing a promise may be rejected with is promptCancelled(). @@ -65,16 +87,8 @@ describe("cancellation contract (#1577)", () => { const offenders: string[] = []; for (const dir of PROMPT_DIRS) { for (const file of walk(dir)) { - const text = stripComments(readFileSync(file, "utf8")); - for (const match of text.matchAll(REJECT_CALL)) { - const arg = (match[1] ?? "").trim(); - // `(resolve, reject) => {` etc: the binding, not a call on it. - if (arg === "" || arg.startsWith(")")) continue; - if (arg.startsWith("promptCancelled(")) continue; - const line = text.slice(0, match.index).split("\n").length; - offenders.push( - `${file.slice(SRC.length + 1)}:${line} — ${match[0].trim().slice(0, 60)}`, - ); + for (const hit of findNonCancelledRejections(readFileSync(file, "utf8"))) { + offenders.push(`${file.slice(SRC.length + 1)}:${hit}`); } } } @@ -120,8 +134,44 @@ describe("cancellation contract (#1577)", () => { expect(offenders).toEqual([]); }); - // Guards the guard: if the walk stops finding files (a moved directory, a changed - // extension filter), every scan above passes vacuously. + // Guards the guard: these are the shapes that would reintroduce #1577, and a + // detector that stops seeing one of them fails silently. + describe("the rejection detector", () => { + it.each([ + ["a bare string", 'this.rejectPromise("dismissed");'], + ["an unrecognisable Error", 'this.rejectPromise(new Error("dismissed"));'], + ["an indirected constant", "this.rejectPromise(CANCEL_MESSAGE);"], + ["an optional call", 'this.rejectPromise?.("dismissed");'], + ["the raw reject binding", 'reject("dismissed");'], + // The multiline forms: captured as EMPTY by a line-bounded pattern, which + // would then wave them through as an executor binding. + ["a multiline string", 'this.rejectPromise(\n\t"dismissed",\n);'], + [ + "a multiline Error", + 'this.rejectPromise(\n\tnew Error("dismissed"),\n);', + ], + ])("flags %s", (_label, source) => { + expect(findNonCancelledRejections(source)).not.toEqual([]); + }); + + it.each([ + ["the contract", "this.rejectPromise(promptCancelled());"], + ["the contract, multiline", "this.rejectPromise(\n\tpromptCancelled(),\n);"], + [ + "an executor binding", + "this.waitForClose = new Promise((resolve, reject) => {\n\t\tthis.rejectPromise = reject;\n\t});", + ], + [ + "a comment about the old shape", + '// this.rejectPromise("dismissed") - the pre-#1577 shape', + ], + ])("allows %s", (_label, source) => { + expect(findNonCancelledRejections(source)).toEqual([]); + }); + }); + + // If the walk stops finding files (a moved directory, a changed extension filter), + // every scan above passes vacuously. it("the scan actually reads the prompt sources", () => { const scanned = PROMPT_DIRS.flatMap((dir) => walk(dir)); expect(scanned.length).toBeGreaterThan(50);