From 865cdfa666d7b499d0e03d7d65b90216d46e70f7 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Tue, 28 Jul 2026 00:54:49 +0200 Subject: [PATCH 1/8] fix: a run that changes nothing stops reporting a side effect it never had A choice run answered "success" to the question "did you finish?", which is not the question an automation asks - it asks "did anything land?". Those had come apart in two shipped configurations that need no interactivity at all: - a Capture whose formatted payload is empty deliberately leaves the file untouched (inserting would add a blank line or, on `currentLine`, DELETE the selection) and says so in a notice, but still recorded a plain success; - a Template set to "Do nothing" when the file already exists is, by name, a no-op, and recorded the same. Both answered `verified:true` with a file path over a byte-identical vault, so a caller counting captures or writing an idempotency marker recorded work that never happened. `ChoiceOutcome`'s success variant now carries a `ChoiceEffect` - `created` / `changed` / `unchanged` - and the recorder takes it as a REQUIRED argument so every call site has to state its claim rather than inherit a positive one by omission. That is what surfaced the two Template sites: of the four success sites in the tree, the two nobody had looked at were the two that commit nothing. The claim is about persisted bytes, never about the payload. An empty payload can still legitimately create a note (create-if-not-found, possibly with a rendered template body), and a non-empty one can still write what was already there - so Capture compares the file's prior content, plus any front-matter post-processing, instead of asking whether the payload was blank. Two consequences worth naming: - The canvas path now checks for the no-op BEFORE it writes. It re-serialises the whole `.canvas` JSON, so writing identical card text still rewrote the file - which would have made the `unchanged` it was about to report false. - An `unchanged` run no longer closes the outcome. The close-guard exists to stop an automation retrying and DUPLICATING a side effect; a run that left none has nothing to protect, and closing there would only hide a real post-commit failure behind a benign "nothing to capture". Refs #1615 --- src/engine/CaptureChoiceEngine.effect.test.ts | 231 ++++++++++++++++++ src/engine/CaptureChoiceEngine.notice.test.ts | 3 + .../CaptureChoiceEngine.selection.test.ts | 1 + src/engine/CaptureChoiceEngine.ts | 47 +++- ...emplateChoiceEngine.audit-template.test.ts | 1 + .../TemplateChoiceEngine.discovery.test.ts | 1 + .../TemplateChoiceEngine.notice.test.ts | 3 + src/engine/TemplateChoiceEngine.ts | 19 +- src/engine/choiceOutcomeRecorder.test.ts | 38 ++- src/engine/choiceOutcomeRecorder.ts | 39 ++- src/types/ChoiceOutcome.ts | 34 ++- 11 files changed, 389 insertions(+), 28 deletions(-) create mode 100644 src/engine/CaptureChoiceEngine.effect.test.ts diff --git a/src/engine/CaptureChoiceEngine.effect.test.ts b/src/engine/CaptureChoiceEngine.effect.test.ts new file mode 100644 index 00000000..ab8319c8 --- /dev/null +++ b/src/engine/CaptureChoiceEngine.effect.test.ts @@ -0,0 +1,231 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * What a Capture run reports it did to the vault (#1615). + * + * The claim has to come from the FILE, not from the payload. A capture whose formatted + * payload is empty deliberately leaves the note untouched — but the same empty payload + * can still legitimately create a note via "create file if it doesn't exist", and a + * non-empty payload can still write bytes identical to what was already there. Every + * case below is one of those, so a payload-derived flag would fail at least one of them. + */ + +vi.mock("../quickAddSettingsTab", () => ({ + DEFAULT_SETTINGS: { choices: [], migrations: {} }, + QuickAddSettingsTab: class {}, +})); + +const { formatted } = vi.hoisted(() => ({ formatted: { value: "" } })); + +vi.mock("../formatters/captureChoiceFormatter", () => { + class CaptureChoiceFormatterMock { + setLinkToCurrentFileBehavior() {} + setTitle() {} + setPromptRunContext() {} + setDestinationFile() {} + setDestinationSourcePath() {} + setUseSelectionAsCaptureValue() {} + async formatContentOnly() { + return formatted.value; + } + // The engine's second pass: splice the payload into the file. An empty payload + // yields the file back unchanged, which is what the real formatter does. + async formatContentWithFile(content: string, _choice: unknown, file: string) { + return content.trim() ? `${file}${content}` : file; + } + async formatFileName(name: string) { + return name; + } + getAndClearTemplatePropertyVars() { + return new Map(); + } + getCaptureInsertionEndOffset() { + return undefined; + } + consumeCreatedClipboardAttachmentPaths() { + return []; + } + async withTemplatePropertyCollection(work: () => Promise) { + return await work(); + } + } + return { CaptureChoiceFormatter: CaptureChoiceFormatterMock }; +}); + +vi.mock("../utils/fileLinks", () => ({ + appendFileLinkToDestinationFile: vi.fn(), + copyFileLinkToClipboard: vi.fn(), + getAppendLinkDestinationFile: vi.fn(), +})); + +vi.mock("../utilityObsidian", () => ({ + appendToCurrentLine: vi.fn(), + getMarkdownFilesInFolder: vi.fn(async () => []), + getMarkdownFilesWithTag: vi.fn(async () => []), + insertFileLinkToActiveView: vi.fn(), + insertOnNewLineAbove: vi.fn(), + insertOnNewLineBelow: vi.fn(), + isFolder: vi.fn(() => false), + openExistingFileTab: vi.fn(() => null), + openFile: vi.fn(), + overwriteTemplaterOnce: vi.fn(), + templaterParseTemplate: vi.fn(async (_app: unknown, content: string) => content), + getTemplater: vi.fn(() => ({})), + isTemplaterTriggerOnCreateEnabled: vi.fn(() => false), + waitForTemplaterTriggerOnCreateToComplete: vi.fn(async () => {}), + withTemplaterFileCreationSuppressed: vi.fn(async (_app: unknown, _p: string, run: () => unknown) => await run()), +})); + +vi.mock("three-way-merge", () => ({ default: vi.fn(() => ({})), __esModule: true })); +vi.mock("src/gui/InputSuggester/inputSuggester", () => ({ + default: class InputSuggesterMock {}, +})); +vi.mock("../main", () => ({ default: class QuickAddMock {} })); +vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() })); + +import { TFile, type App } from "obsidian"; +import { CaptureChoiceEngine } from "./CaptureChoiceEngine"; +import type { IChoiceExecutor } from "../IChoiceExecutor"; +import type ICaptureChoice from "../types/choices/ICaptureChoice"; +import { settingsStore } from "../settingsStore"; + +function createTestFile(path: string): TFile { + const file = new TFile(); + file.path = path; + file.name = path.slice(path.lastIndexOf("/") + 1); + file.extension = file.name.slice(file.name.lastIndexOf(".") + 1); + file.basename = file.name.replace(/\.[^.]+$/, ""); + return file; +} + +const createChoice = (): ICaptureChoice => ({ + name: "Inbox", + id: "capture-effect", + type: "Capture", + command: false, + captureTo: "Inbox.md", + captureToActiveFile: false, + createFileIfItDoesntExist: { + enabled: false, + createWithTemplate: false, + template: "", + }, + format: { enabled: true, format: "{{VALUE}}" }, + insertAfter: { + enabled: false, + after: "", + insertAtEnd: false, + considerSubsections: false, + createIfNotFound: false, + createIfNotFoundLocation: "top", + }, + prepend: false, + appendLink: false, + task: false, + openFile: false, + fileOpening: {}, +} as unknown as ICaptureChoice); + +function harness({ exists, existing }: { exists: boolean; existing: string }) { + const captureFile = createTestFile("Inbox.md"); + const created: Array<{ path: string; content: string }> = []; + const app = { + vault: { + adapter: { exists: vi.fn(async () => exists) }, + getAbstractFileByPath: vi.fn(() => (exists ? captureFile : null)), + read: vi.fn(async () => existing), + modify: vi.fn(), + create: vi.fn(async (path: string, content: string) => { + created.push({ path, content }); + return captureFile; + }), + createFolder: vi.fn(), + }, + workspace: { + getActiveFile: vi.fn(() => null), + getActiveViewOfType: vi.fn(() => null), + }, + fileManager: { getNewFileParent: vi.fn(() => ({ path: "" })) }, + } as unknown as App; + + const choiceExecutor: IChoiceExecutor = { + execute: vi.fn(), + recordExecutionResult: vi.fn(), + variables: new Map(), + } as unknown as IChoiceExecutor; + + const plugin = { + settings: { ...settingsStore.getState(), showCaptureNotification: false }, + } as never; + + const choice = createChoice(); + return { + app, + choice, + choiceExecutor, + captureFile, + created, + engine: new CaptureChoiceEngine(app, plugin, choice, choiceExecutor), + }; +} + +const recordedEffect = (executor: IChoiceExecutor) => { + const calls = (executor.recordExecutionResult as ReturnType).mock + .calls; + return calls.at(-1)?.[0]; +}; + +describe("CaptureChoiceEngine reports what it did to the vault (#1615)", () => { + beforeEach(() => { + formatted.value = ""; + vi.clearAllMocks(); + }); + + // The exact shape from the issue: an empty {{VALUE}} answer. Before this, the run + // answered `verified:true` over a byte-identical note. + it("reports 'unchanged' when an empty payload leaves an existing note alone", async () => { + const { engine, choiceExecutor, captureFile } = harness({ + exists: true, + existing: "# Inbox\n", + }); + + await engine.run(); + + expect(recordedEffect(choiceExecutor)).toEqual({ + status: "success", + file: captureFile, + effect: "unchanged", + }); + }); + + it("reports 'changed' when the payload actually lands", async () => { + formatted.value = "- a real line"; + const { engine, choiceExecutor } = harness({ + exists: true, + existing: "# Inbox\n", + }); + + await engine.run(); + + expect(recordedEffect(choiceExecutor)).toMatchObject({ effect: "changed" }); + }); + + // The counterexample that kills a payload-derived flag: the payload is empty, so a + // `!captureIsNoOp` predicate would say "unchanged" — but a note really was created. + it("reports 'created' when an empty payload still creates the note", async () => { + const { engine, choice, choiceExecutor, created } = harness({ + exists: false, + existing: "", + }); + choice.createFileIfItDoesntExist = { + enabled: true, + createWithTemplate: false, + template: "", + }; + + await engine.run(); + + expect(created).toHaveLength(1); + expect(recordedEffect(choiceExecutor)).toMatchObject({ effect: "created" }); + }); +}); diff --git a/src/engine/CaptureChoiceEngine.notice.test.ts b/src/engine/CaptureChoiceEngine.notice.test.ts index d3013cf6..bcb81ba1 100644 --- a/src/engine/CaptureChoiceEngine.notice.test.ts +++ b/src/engine/CaptureChoiceEngine.notice.test.ts @@ -419,6 +419,7 @@ describe("CaptureChoiceEngine append-link destination", () => { expect(choiceExecutor.recordExecutionResult).toHaveBeenCalledWith({ status: "success", file: captureFile, + effect: "changed", }); expect(copyFileLinkToClipboardMock).toHaveBeenCalledWith(captureFile); expect(appendFileLinkToDestinationFileMock).not.toHaveBeenCalled(); @@ -440,6 +441,7 @@ describe("CaptureChoiceEngine append-link destination", () => { expect(choiceExecutor.recordExecutionResult).toHaveBeenCalledWith({ status: "success", file: captureFile, + effect: "changed", }); }); @@ -454,6 +456,7 @@ describe("CaptureChoiceEngine append-link destination", () => { expect(choiceExecutor.recordExecutionResult).toHaveBeenCalledWith({ status: "success", file: captureFile, + effect: "changed", }); expect(appendFileLinkToDestinationFileMock).toHaveBeenCalledWith( app, diff --git a/src/engine/CaptureChoiceEngine.selection.test.ts b/src/engine/CaptureChoiceEngine.selection.test.ts index 8f663955..d34b16b1 100644 --- a/src/engine/CaptureChoiceEngine.selection.test.ts +++ b/src/engine/CaptureChoiceEngine.selection.test.ts @@ -1742,6 +1742,7 @@ describe("CaptureChoiceEngine capture target resolution", () => { expect(executor.recordExecutionResult).toHaveBeenCalledWith({ status: "success", file: linkedFile, + effect: "changed", }); expect(insertFileLinkToActiveView).toHaveBeenCalledWith( app, diff --git a/src/engine/CaptureChoiceEngine.ts b/src/engine/CaptureChoiceEngine.ts index 037c95be..1111e7e3 100644 --- a/src/engine/CaptureChoiceEngine.ts +++ b/src/engine/CaptureChoiceEngine.ts @@ -55,6 +55,7 @@ import { ChoiceOutcomeRecorder, failureReason, } from "./choiceOutcomeRecorder"; +import type { ChoiceEffect } from "../types/ChoiceOutcome"; import type { FieldFilter } from "../utils/FieldSuggestionParser"; import { resolveCaptureTarget as resolveCaptureTargetFromString, @@ -114,6 +115,13 @@ type CaptureWriteResult = { file: TFile; newFileContent: string; captureContent: string; + /** + * The bytes on disk immediately before the write, so `run()` can report whether the + * capture actually changed anything rather than inferring it from the payload + * (#1615). An empty payload can still create a note, and a non-empty one can still + * leave the file untouched, so the payload is the wrong thing to ask. + */ + priorContent: string; cursorEndOffset?: number; cursorPlacementSafe?: boolean; }; @@ -512,12 +520,14 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { file, newFileContent, captureContent, + priorContent, cursorEndOffset, cursorPlacementSafe = true, } = await getFileAndAddContentFn(filePath, content); let expectedCursorContent: string | null = null; let canPlaceCursorAtCapture = cursorPlacementSafe; + let wroteFrontmatter = false; // The formatted capture payload is empty/whitespace-only: the formatter // returns the file unchanged and editor insertion replaces the selection // with "" — i.e. a no-op. Surface a distinct notice instead of a false @@ -580,13 +590,32 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { if (frontmatterPostProcessed) { canPlaceCursorAtCapture = false; } + // Post-processing writes front matter of its own, so it counts as a + // change even when the capture body itself was a no-op. + wroteFrontmatter = frontmatterPostProcessed; expectedCursorContent = canPlaceCursorAtCapture ? newFileContent : null; } // Content is committed. Record success before append-link/open-file steps // so a later post-commit failure cannot make automation callers retry and // duplicate the Capture side effect. - this.outcome.success(file); + // + // What landed, judged by the file rather than the payload (#1615). The + // editor-insertion branch above SKIPS the insertion entirely on a no-op, so + // there the payload is the file; every other branch compares persisted bytes. + // `createFileIfItDoesntExist` can legitimately create a note (possibly with a + // rendered template body) from an empty payload, which is why "created" is + // tested before emptiness. + const effect: ChoiceEffect = isEditorInsertionAction + ? captureIsNoOp + ? "unchanged" + : "changed" + : !fileAlreadyExists + ? "created" + : newFileContent !== priorContent || wroteFrontmatter + ? "changed" + : "unchanged"; + this.outcome.success(file, effect); // Show success notification if (this.plugin.settings.showCaptureNotification) { @@ -745,16 +774,22 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { this.captureResolvedOrderedHeading(); - await setCanvasTextCaptureContent(this.app, target, nextText); - markContentCommitted(); - // An empty/whitespace capture leaves the card text unchanged (the formatter // returns existingText as-is) — surface a no-op notice instead of a false // "Captured to …" success, consistent with the note-body path in run(). const captureIsNoOp = nextText === existingText; + // Checked BEFORE the write, not after. `setCanvasTextCaptureContent` re-serialises + // the whole .canvas JSON, so writing identical card text can still rewrite the + // file's bytes — which would make the "unchanged" this run is about to report + // false, and would touch a file the user was told was left alone (#1615). + if (!captureIsNoOp) { + await setCanvasTextCaptureContent(this.app, target, nextText); + } + markContentCommitted(); + // Committed; append-link/open-file steps remain post-commit (see run()). - this.outcome.success(file); + this.outcome.success(file, captureIsNoOp ? "unchanged" : "changed"); if (this.plugin.settings.showCaptureNotification) { if (captureIsNoOp) { @@ -1439,6 +1474,7 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { file, newFileContent, captureContent: formatted, + priorContent: secondReadFileContent, cursorEndOffset: cursorEndOffset ?? undefined, cursorPlacementSafe, }; @@ -1556,6 +1592,7 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { file, newFileContent, captureContent: formattedCaptureContent, + priorContent: updatedFileContent, cursorEndOffset: cursorEndOffset ?? undefined, cursorPlacementSafe: true, }; diff --git a/src/engine/TemplateChoiceEngine.audit-template.test.ts b/src/engine/TemplateChoiceEngine.audit-template.test.ts index d6916407..62e23774 100644 --- a/src/engine/TemplateChoiceEngine.audit-template.test.ts +++ b/src/engine/TemplateChoiceEngine.audit-template.test.ts @@ -238,6 +238,7 @@ describe("TemplateChoiceEngine post-commit link failure (audit)", () => { expect(choiceExecutor.recordExecutionResult).toHaveBeenCalledWith({ status: "success", file: createdFile, + effect: "created", }); // The link failure surfaces as a warning that names the created file, not // a fatal "Error running template choice". diff --git a/src/engine/TemplateChoiceEngine.discovery.test.ts b/src/engine/TemplateChoiceEngine.discovery.test.ts index 39ab4dd4..e1de245e 100644 --- a/src/engine/TemplateChoiceEngine.discovery.test.ts +++ b/src/engine/TemplateChoiceEngine.discovery.test.ts @@ -214,6 +214,7 @@ describe("TemplateChoiceEngine note discovery", () => { expect(choiceExecutor.recordExecutionResult).toHaveBeenCalledWith({ status: "success", file: existing, + effect: "unchanged", }); }); diff --git a/src/engine/TemplateChoiceEngine.notice.test.ts b/src/engine/TemplateChoiceEngine.notice.test.ts index af285b4b..40848a0e 100644 --- a/src/engine/TemplateChoiceEngine.notice.test.ts +++ b/src/engine/TemplateChoiceEngine.notice.test.ts @@ -548,6 +548,7 @@ describe("TemplateChoiceEngine cancellation notices", () => { expect(choiceExecutor.recordExecutionResult).toHaveBeenCalledWith({ status: "success", file: createdFile, + effect: "created", }); }); @@ -659,6 +660,7 @@ describe("TemplateChoiceEngine cancellation notices", () => { expect(choiceExecutor.recordExecutionResult).toHaveBeenCalledWith({ status: "success", file: createdFile, + effect: "created", }); expect(store.get(draftKey)).toBeUndefined(); }); @@ -704,6 +706,7 @@ describe("TemplateChoiceEngine cancellation notices", () => { expect(choiceExecutor.recordExecutionResult).toHaveBeenCalledWith({ status: "success", file: createdFile, + effect: "created", }); }); }); diff --git a/src/engine/TemplateChoiceEngine.ts b/src/engine/TemplateChoiceEngine.ts index 50bce3c4..60d0ccda 100644 --- a/src/engine/TemplateChoiceEngine.ts +++ b/src/engine/TemplateChoiceEngine.ts @@ -17,6 +17,7 @@ import { shouldRunTemplateNoteDiscovery, } from "./templateNoteDiscovery"; import type ITemplateChoice from "../types/choices/ITemplateChoice"; +import type { ChoiceEffect } from "../types/ChoiceOutcome"; import { normalizeAppendLinkOptions, placementSupportsFrontmatter, @@ -116,7 +117,10 @@ export class TemplateChoiceEngine extends TemplateEngine { ); if (discovery.kind === "openExisting") { await this.openDiscoveredExistingNote(discovery.file); - this.outcome.success(discovery.file); + // Opening a note is not writing one: this path exists precisely to + // AVOID creating a duplicate, so it leaves the vault byte-identical + // (#1615). + this.outcome.success(discovery.file, "unchanged"); return; } @@ -187,9 +191,20 @@ export class TemplateChoiceEngine extends TemplateEngine { let createdFile: TFile | null; let shouldAutoOpen = false; let createdNew = false; + // What this run did to the vault (#1615). Derived from the file-exists + // resolution the engine actually performed, which is exact for the two + // answers an automation acts on: "createNew" always writes a new note, and + // "reuseExisting" — the shipped "Do nothing" mode — writes nothing at all. + let effect: ChoiceEffect = "created"; if (await this.app.vault.adapter.exists(targetFilePath)) { const modeId = await this.getSelectedFileExistsMode(); const mode = getFileExistsMode(modeId); + effect = + mode.resolutionKind === "reuseExisting" + ? "unchanged" + : mode.resolutionKind === "createNew" + ? "created" + : "changed"; const existingFile = mode.requiresExistingFile ? this.findExistingFile(targetFilePath) : null; @@ -250,7 +265,7 @@ export class TemplateChoiceEngine extends TemplateEngine { // File is created/resolved (the commit point). Record success before // append-link/open-file steps so a later post-commit failure cannot make // automation callers retry and duplicate the Template side effect. - this.outcome.success(createdFile); + this.outcome.success(createdFile, effect); if (linkOptions.enabled && createdFile) { // The note is already committed (success recorded above). A link diff --git a/src/engine/choiceOutcomeRecorder.test.ts b/src/engine/choiceOutcomeRecorder.test.ts index 367d7300..e6047863 100644 --- a/src/engine/choiceOutcomeRecorder.test.ts +++ b/src/engine/choiceOutcomeRecorder.test.ts @@ -22,17 +22,41 @@ describe("ChoiceOutcomeRecorder (#1603)", () => { // duplicate the side effect. Terminality has to belong to the recorder: Capture // commits from two methods, and the canvas path keeps going into steps whose // throws unwind into run()'s catch. - it("is a no-op once the run has committed", () => { + it.each(["created", "changed"] as const)( + "is a no-op once the run has committed (%s)", + (effect) => { + const target = executor(); + const recorder = new ChoiceOutcomeRecorder(target); + + recorder.success({ path: "Note.md" } as never, effect); + recorder.failure( + "Cannot append link because no active Markdown view is available.", + ); + + expect(target.recordExecutionResult).toHaveBeenCalledTimes(1); + expect(target.recordExecutionResult).toHaveBeenCalledWith({ + status: "success", + file: { path: "Note.md" }, + effect, + }); + }, + ); + + // The close-guard exists to stop an automation retrying and DUPLICATING a side + // effect. An "unchanged" run left no side effect, so there is nothing to protect + // and a real post-commit failure must still reach the caller instead of being + // swallowed behind a benign "nothing to capture" (#1615). + it("stays open after an unchanged run, so a later failure is still reported", () => { const target = executor(); const recorder = new ChoiceOutcomeRecorder(target); - recorder.success({ path: "Note.md" } as never); - recorder.failure("Cannot append link because no active Markdown view is available."); + recorder.success({ path: "Inbox.md" } as never, "unchanged"); + recorder.failure("Append link target file not found."); - expect(target.recordExecutionResult).toHaveBeenCalledTimes(1); - expect(target.recordExecutionResult).toHaveBeenCalledWith({ - status: "success", - file: { path: "Note.md" }, + expect(target.recordExecutionResult).toHaveBeenCalledTimes(2); + expect(target.recordExecutionResult).toHaveBeenLastCalledWith({ + status: "error", + reason: "Append link target file not found.", }); }); diff --git a/src/engine/choiceOutcomeRecorder.ts b/src/engine/choiceOutcomeRecorder.ts index bfc3d27f..35a2dd8b 100644 --- a/src/engine/choiceOutcomeRecorder.ts +++ b/src/engine/choiceOutcomeRecorder.ts @@ -1,5 +1,6 @@ import type { TFile } from "obsidian"; import type { IChoiceExecutor } from "../IChoiceExecutor"; +import type { ChoiceEffect } from "../types/ChoiceOutcome"; /** * Records what a choice run actually did, for the callers that must report it back to @@ -19,24 +20,40 @@ import type { IChoiceExecutor } from "../IChoiceExecutor"; * have five other places that log and return, and each one used to produce the same * reason-less outcome. Routing them all through {@link failure} is what makes the CLI's * fixed sentence unreachable rather than merely less common. - * - **The failure recorder is a no-op once the run has committed.** Both engines record - * success at their commit point precisely so a later append-link or open-file failure - * cannot make an automation caller retry and duplicate the side effect. That has to be - * a property of the recorder, not of one method: Capture commits from two places (the - * note path and the canvas path), and the canvas path keeps going into link and + * - **The failure recorder is a no-op once the run has left a side effect.** Both engines + * record success at their commit point precisely so a later append-link or open-file + * failure cannot make an automation caller retry and duplicate the side effect. That has + * to be a property of the recorder, not of one method: Capture commits from two places + * (the note path and the canvas path), and the canvas path keeps going into link and * open-file steps whose throws unwind into `run()`'s catch. + * + * An `unchanged` run is the exception, and for the same reason: it left nothing a retry + * could duplicate, so closing the outcome there would only hide a real post-commit + * failure behind a benign "nothing to capture" (#1615). */ export class ChoiceOutcomeRecorder { - private committed = false; + /** + * The outcome is settled — {@link failure} can no longer overwrite it. Distinct from + * the {@link ChoiceEffect} the run reports: `closed` is about this recorder's state, + * `effect` is about the vault. + */ + private closed = false; constructor( private readonly executor: Pick, ) {} - /** The run committed its side effect. Records success and closes the outcome. */ - success(file?: TFile): void { - this.committed = true; - this.executor.recordExecutionResult?.({ status: "success", file }); + /** + * The run reached its commit point. Records success with what it did to the vault. + * + * `effect` is required rather than defaulted: every call site has to state its claim, + * so a new one cannot inherit a positive "something landed" by omission. The four + * existing sites split evenly — two of them (Template's "Do nothing" mode and its + * open-an-existing-note discovery path) commit nothing at all. + */ + success(file: TFile | undefined, effect: ChoiceEffect): void { + if (effect !== "unchanged") this.closed = true; + this.executor.recordExecutionResult?.({ status: "success", file, effect }); } /** @@ -44,7 +61,7 @@ export class ChoiceOutcomeRecorder { * notice carries, so a remote client and a local user learn the same thing. */ failure(reason: string): void { - if (this.committed) return; + if (this.closed) return; this.executor.recordExecutionResult?.({ status: "error", reason }); } } diff --git a/src/types/ChoiceOutcome.ts b/src/types/ChoiceOutcome.ts index b444c2a1..cd23700d 100644 --- a/src/types/ChoiceOutcome.ts +++ b/src/types/ChoiceOutcome.ts @@ -1,12 +1,37 @@ import type { TFile } from "obsidian"; +/** + * What a successful run actually did to the vault. + * + * `success` alone answers "did the run finish?", which is not the question an + * automation asks — it asks "did anything land?". Those came apart in two shipped + * configurations that need no interactivity at all: a Capture whose formatted payload + * is empty deliberately leaves the file untouched (inserting would add a blank line + * or, on `currentLine`, DELETE the selection), and a Template set to "Do nothing" when + * the file exists is, by name, a no-op. Both reported an unqualified success, so a + * caller counting captures or writing an idempotency marker recorded work that never + * happened (#1615). + * + * `created` means the file did not exist before this run. `changed` means its bytes + * differ. `unchanged` means the vault is byte-identical — nothing failed, and nothing + * a retry could duplicate happened either. + * + * The claim is about PERSISTED BYTES, never about the payload: a Capture with an empty + * payload can still legitimately `create` a note (create-if-not-found, possibly with a + * rendered template body), and one whose payload is non-empty can still leave the file + * untouched. Deriving this from the formatted content instead of the file would just + * relocate the lie. + */ +export type ChoiceEffect = "created" | "changed" | "unchanged"; + /** * The result of executing a single choice, surfaced by * {@link ChoiceExecutor.executeWithOutcome} for callers (e.g. the URI x-callback * handler) that must report success / failure / cancellation back to an external * caller. * - * `success` carries the affected file when one is known (Template, Capture). + * `success` carries the affected file when one is known (Template, Capture) and the + * {@link ChoiceEffect} it had on the vault. * `cancelled` distinguishes a genuine user prompt-dismissal (`"user"`) from an * involuntary script/config abort (`"aborted"`). `error` means the choice failed. * @@ -16,9 +41,12 @@ import type { TFile } from "obsidian"; * interactive bridge, both loopback- and token-gated, surface it, because on those * paths nobody is looking at the desktop notice that carries the same text (#1603). * The URI x-callback handler deliberately ignores it on BOTH variants, reporting only - * a fixed code, so no vault detail leaks to an external callback URL. + * a fixed code, so no vault detail leaks to an external callback URL. `effect` is not + * subject to that rule: it is a three-token enum with no vault detail in it, and the + * handler already sends the note's path, so withholding it would leave the very + * automation #1615 is about still counting captures that never happened. */ export type ChoiceOutcome = - | { status: "success"; file?: TFile } + | { status: "success"; file?: TFile; effect: ChoiceEffect } | { status: "error"; reason?: string } | { status: "cancelled"; cancelKind: "user" | "aborted"; reason?: string }; From 0c3bb7a3b8d8539c3e426d5a7b4281fd7b7dd4ff Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Tue, 28 Jul 2026 00:58:27 +0200 Subject: [PATCH 2/8] fix: tell automation what a run did to the vault, without redefining `verified` The `effect` a run had is now on every terminal frame the CLI, the interactive bridge and the x-callback emit, so a caller can finally ask "did anything land?" instead of inferring it from "did the run finish?". `verified` is deliberately left exactly as it was. It already means "QuickAdd could not confirm this - go look", and the handler's own comment invites retry logic on `verified:false`. Folding "we confirmed, and nothing happened" into that same value would make a correct, permanently-unchanging run look retryable forever - the one change here that would have broken a shipped contract silently. So the new fact gets a new key rather than a new meaning for an old one. `unknown` is stated rather than omitted, on both legacy tails. A missing key reads as `false` to `jq '.effect'` and to `!res.effect` alike, which is exactly the wrong direction: it would turn "QuickAdd did not look" into "nothing happened". The interactive legacy tail emitted neither flag before, so a client could not tell it apart from the verified frame at all; it now says both. `capabilities` gains `outcome-effect` so a bridge client can feature-detect this instead of probing for the key, and the x-callback success params carry `effect` too. That last one is not a leak: it is a three-token enum, and the same callback already sends the note's path and the vault name - withholding it would leave the Shortcut that writes an idempotency marker on `status=success`, which is the automation this issue is about, still marking captures that never happened. Refs #1615 --- docs/src/content/docs/docs/Advanced/CLI.md | 34 +++++++++++++++++++-- src/cli/registerQuickAddCliHandlers.test.ts | 11 ++++++- src/cli/registerQuickAddCliHandlers.ts | 29 +++++++++++++++--- src/main.ts | 22 +++++++++++-- 4 files changed, 85 insertions(+), 11 deletions(-) diff --git a/docs/src/content/docs/docs/Advanced/CLI.md b/docs/src/content/docs/docs/Advanced/CLI.md index 6c099551..c53d2a21 100644 --- a/docs/src/content/docs/docs/Advanced/CLI.md +++ b/docs/src/content/docs/docs/Advanced/CLI.md @@ -125,6 +125,35 @@ In a [scheduled job](/docs/Advanced/TriggerQuickAddFromOutsideObsidian/#run-quic only add `ui` when the job runs while you are logged in and able to answer the prompts. +## Knowing whether anything actually landed {#verified-and-effect} + +`ok:true` means the choice ran without aborting. It does **not** mean your vault +changed. Two more keys answer the questions an automation actually asks: + +| Key | Question it answers | Values | +| --- | --- | --- | +| `verified` | Did QuickAdd confirm what the engine did? | `true` on the outcome path (`verify` on a Template/Capture choice), `false` when it could not look | +| `effect` | What did the run do to the vault? | `created`, `changed`, `unchanged`, `unknown` | + +```bash +obsidian vault=dev quickadd:run choice="Inbox" value-value=" " verify=true +# -> {"ok":true,"choice":{…},"file":"Inbox.md","verified":true,"effect":"unchanged","durationMs":6} +``` + +That run is working exactly as designed: the capture's payload was empty, so +QuickAdd deliberately left `Inbox.md` alone rather than writing a blank line, and +said so in a notice. A Template set to **Do nothing** when the file already exists +reports the same. If you are counting captures, writing an idempotency marker, or +deciding whether to retry, key off `effect`, not `ok`. + +`effect` is always present on a terminal frame, and `unknown` is stated rather than +omitted - a missing key reads as `false` in both `jq` and JavaScript, which would +turn "QuickAdd did not look" into "nothing happened". `verified:false` still means +only *"not confirmed - go look"*; it never means *"confirmed that nothing changed"*. + +The `obsidian://quickadd` [x-callback](/docs/Advanced/TriggerQuickAddFromOutsideObsidian/) +success callback carries the same `effect` value. + ## Answer run-time prompts from outside: `quickadd:interactive` {#interactive-runs-quickaddinteractive} Some choices prompt at *run time* for inputs that can't be gathered up front - @@ -136,7 +165,7 @@ the prompts opening in Obsidian. ```bash obsidian vault=dev quickadd:interactive choice="Import from Readwise" -# -> {"ok":true,"host":"127.0.0.1","port":51789,"sessionId":"…","token":"…","capabilities":["abort"]} +# -> {"ok":true,"host":"127.0.0.1","port":51789,"sessionId":"…","token":"…","capabilities":["abort","outcome-effect"]} ``` The command returns connection details immediately and runs the choice in the @@ -150,7 +179,8 @@ Prompt `type`s and the `value` you reply with: `suggester`/`input`/`date` → string, `confirm` → boolean, `checkbox` → string array, `info` → 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. +`done`/`error` poll event: `done` carries the same `verified` and `effect` keys +described under [Knowing whether anything actually landed](#verified-and-effect). ### Cancelling, and ending a run diff --git a/src/cli/registerQuickAddCliHandlers.test.ts b/src/cli/registerQuickAddCliHandlers.test.ts index a63a82b0..c9e42da4 100644 --- a/src/cli/registerQuickAddCliHandlers.test.ts +++ b/src/cli/registerQuickAddCliHandlers.test.ts @@ -152,6 +152,7 @@ describe("registerQuickAddCliHandlers", () => { executeWithOutcome: vi.fn().mockResolvedValue({ status: "success", file: { path: "Created Note.md" }, + effect: "created", }), variables: new Map(), consumeAbortSignal: vi.fn().mockReturnValue(null), @@ -250,6 +251,11 @@ describe("registerQuickAddCliHandlers", () => { expect(verified.ok).toBe(true); expect(verified.verified).toBe(true); expect(verified.file).toBe("Created Note.md"); + // `verified` says QuickAdd confirmed the run; `effect` says what it did to the + // vault. They are separate keys on purpose - a correctly-behaving no-op is + // `verified:true` with `effect:"unchanged"`, and overloading `verified` would + // have made it look retryable forever (#1615). + expect(verified.effect).toBe("created"); expect(executors[0].executeWithOutcome).toHaveBeenCalledWith( templateChoice, ); @@ -269,6 +275,9 @@ describe("registerQuickAddCliHandlers", () => { expect(legacy.ok).toBe(true); expect(legacy.verified).toBe(false); expect(legacy.file).toBeUndefined(); + // Stated, not omitted: an absent key reads as `false` to both `jq` and JS, + // which would turn "we did not look" into "nothing happened". + expect(legacy.effect).toBe("unknown"); expect(executors[1].execute).toHaveBeenCalledWith(templateChoice); }); @@ -419,7 +428,7 @@ describe("registerQuickAddCliHandlers", () => { // run (#1605). An unknown path and an unauthed session answer the same 404 // shape, so without this a client could only detect the build by string-matching // an error body. - expect(response.capabilities).toEqual(["abort"]); + expect(response.capabilities).toEqual(["abort", "outcome-effect"]); // The run is fire-and-forget; let its microtasks settle. await new Promise((resolve) => setTimeout(resolve, 0)); diff --git a/src/cli/registerQuickAddCliHandlers.ts b/src/cli/registerQuickAddCliHandlers.ts index 6b5f830a..bddb3125 100644 --- a/src/cli/registerQuickAddCliHandlers.ts +++ b/src/cli/registerQuickAddCliHandlers.ts @@ -79,7 +79,7 @@ const RUN_FLAGS: CliFlags = { }, verify: { description: - "Report the verified outcome for Template/Capture choices (file path on success, honest failure when the engine swallows an error)", + "Report the verified outcome for Template/Capture choices (file path and effect on success, honest failure when the engine swallows an error)", }, }; @@ -433,6 +433,12 @@ async function runResolvedChoice( // The outcome path confirms the engine actually completed (a file // was created / capture written), so this success is verified. verified: true, + // What the run did to the vault. `verified` answers "did QuickAdd + // confirm the run?"; `effect` answers "did anything land?" - the + // question an automation counting captures or writing an idempotency + // marker actually asks. A correctly-behaving no-op is + // `verified:true, effect:"unchanged"` (#1615). + effect: outcome.effect, durationMs, }); } @@ -493,6 +499,9 @@ async function runResolvedChoice( // logic off `ok` can tell it apart from the verified outcome path (and use // quickadd:check up front, or quickadd:run-template for a verified create). verified: false, + // Stated, never left absent: a missing key reads as `false` to both `jq` + // and JS, which would turn "we did not look" into "nothing happened". + effect: "unknown", durationMs, }); } catch (error) { @@ -784,6 +793,7 @@ async function interactiveHandler( choice: describeChoice(choice), file: outcome.file?.path, verified: true, + effect: outcome.effect, }, }); return; @@ -816,7 +826,16 @@ async function interactiveHandler( } interactivePromptServer.finish(sessionId, { kind: "done", - result: { ok: true, choice: describeChoice(choice) }, + // The legacy void-execute tail (Macro, and anything without the + // outcome path). It used to emit neither flag, so a client could not + // tell this frame apart from the verified one above; both are now + // stated explicitly (#1615). + result: { + ok: true, + choice: describeChoice(choice), + verified: false, + effect: "unknown", + }, }); } catch (error) { interactivePromptServer.finish(sessionId, { @@ -839,7 +858,7 @@ async function interactiveHandler( // panel instead of ending the run (#1605). Without it a client could only // tell the two behaviours apart by string-matching a 404 body, since an // unknown path and an unauthed session answer the same shape. - capabilities: ["abort"], + capabilities: ["abort", "outcome-effect"], }); } catch (error) { return serialize({ @@ -908,14 +927,14 @@ export function registerQuickAddCliHandlers(plugin: QuickAdd): boolean { register( CLI_COMMANDS.runDefault, - "Run a QuickAdd choice (ok:true reports the choice ran without aborting; check the verified flag to know if a file was created)", + "Run a QuickAdd choice (ok:true reports the choice ran without aborting; check verified to know QuickAdd confirmed the run, and effect to know whether the vault changed: created/changed/unchanged/unknown)", RUN_FLAGS, (params: CliData) => runChoiceHandler(plugin, params, CLI_COMMANDS.runDefault), ); register( CLI_COMMANDS.run, - "Run a QuickAdd choice (ok:true reports the choice ran without aborting; check the verified flag to know if a file was created)", + "Run a QuickAdd choice (ok:true reports the choice ran without aborting; check verified to know QuickAdd confirmed the run, and effect to know whether the vault changed: created/changed/unchanged/unknown)", RUN_FLAGS, (params: CliData) => runChoiceHandler(plugin, params, CLI_COMMANDS.run), ); diff --git a/src/main.ts b/src/main.ts index 4de4e8ce..bb0a5a24 100644 --- a/src/main.ts +++ b/src/main.ts @@ -49,6 +49,7 @@ import { setQuickAddInstance } from "./quickAddInstance"; import { applyTemplateToNote } from "./engine/applyTemplateToActiveNote"; import type ITemplateChoice from "./types/choices/ITemplateChoice"; import type ICaptureChoice from "./types/choices/ICaptureChoice"; +import type { ChoiceEffect } from "./types/ChoiceOutcome"; import { buildCallbackUrl, buildObsidianOpenUrl, @@ -307,7 +308,7 @@ export default class QuickAdd extends Plugin { switch (outcome.status) { case "success": - this.fireUriSuccess(targets, outcome.file); + this.fireUriSuccess(targets, outcome.file, outcome.effect); break; case "cancelled": if (outcome.cancelKind === "user") { @@ -454,8 +455,23 @@ export default class QuickAdd extends Plugin { }); } - private fireUriSuccess(targets: CallbackTargets, file?: TFile): void { - const params: Record = { status: "success" }; + /** + * `effect` rides along with the success callback, unlike `reason`, which this + * handler deliberately withholds on both failure variants. + * + * The rule it is not breaking is "leak no vault detail to an external callback + * URL": `effect` is a three-token enum (`created`/`changed`/`unchanged`) with + * nothing vault-specific in it, and this same callback already carries the note's + * path and the vault name. Withholding it would leave the exact automation #1615 + * is about — a Shortcut writing an idempotency marker on `status=success` — still + * marking captures that never happened. + */ + private fireUriSuccess( + targets: CallbackTargets, + file: TFile | undefined, + effect: ChoiceEffect, + ): void { + const params: Record = { status: "success", effect }; if (file) { params.path = file.path; params.url = buildObsidianOpenUrl(this.app.vault.getName(), file.path); From e65cdd6522a830e7f5fd20d61a643358fab41b46 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Tue, 28 Jul 2026 01:14:09 +0200 Subject: [PATCH 3/8] fix: an interactive run stops opening its "file already exists" chooser on the desktop `quickadd:interactive` promises to forward a run's prompts to the connected client. It kept that promise for every prompt a script or the formatter opens, because each of those hand-wrote an `if (promptProvider)` branch at its own call site. The engines never got one, so a Template whose target note already existed opened its chooser in Obsidian while the client's `/poll` returned nothing - and the run sat there until someone walked past the machine, on exactly the headless setups this seam exists for. `routePrompt` is a seam rather than a seventh hand-written branch, because "remember to add the branch" had already been missed six times. It deliberately owns only what is uniform: the ORDER of the three destinations and the `isCancellationError` -> UserCancelError mapping every site was repeating. Each site still passes three closures and keeps its own modal call verbatim, because the variance here is entirely in modal construction - and because the headless branch is not uniform either. Most sites abort; MacroChoiceEngine legitimately runs a sole exported member without asking, and a fixed ladder would have regressed that. Engine replies are also validated, which the in-app modal gets for free: GenericSuggester can only ever resolve an element of its own list, and routing the prompt must not widen a closed list into free text just because the answer now arrives over HTTP. `RemotePromptProvider.suggester` returns any string verbatim and maps a missing value to `""`; both are correct for the script callers it was written for and dangerous here - `""` reaches `getFileExistsMode("")`, whose internal `Unknown file exists mode: ` is what a client would have seen. An off-list reply now ends the run naming the fix, and an empty one is treated as the dismissal it is. The lenient path is untouched for the script and formatter callers that depend on it. Session teardown now rejects pending prompts with ChoiceAbortError instead of a bare Error. That only became load-bearing here: `isCancellationError` is false for a plain Error, so once engine prompts travel this bridge, a client that simply stopped polling would have been reported as a run FAILURE - a red notice on a desktop nobody is sitting at. Verified live in an isolated vault: the chooser now arrives over `/poll` with an empty `.modal-container` list in Obsidian, an off-list reply is refused, a dismissal ends the run like Escape does, and `POST /abort` reports `interrupted:1` where #1614 documented `0`. Refs #1614 --- src/engine/TemplateChoiceEngine.ts | 63 ++++---- src/interactive/engineChoice.ts | 77 +++++++++ src/interactive/interactivePromptServer.ts | 25 ++- src/interactive/routePrompt.test.ts | 178 +++++++++++++++++++++ src/interactive/routePrompt.ts | 73 +++++++++ 5 files changed, 385 insertions(+), 31 deletions(-) create mode 100644 src/interactive/engineChoice.ts create mode 100644 src/interactive/routePrompt.test.ts create mode 100644 src/interactive/routePrompt.ts diff --git a/src/engine/TemplateChoiceEngine.ts b/src/engine/TemplateChoiceEngine.ts index 60d0ccda..3f1a6060 100644 --- a/src/engine/TemplateChoiceEngine.ts +++ b/src/engine/TemplateChoiceEngine.ts @@ -18,6 +18,8 @@ import { } from "./templateNoteDiscovery"; import type ITemplateChoice from "../types/choices/ITemplateChoice"; import type { ChoiceEffect } from "../types/ChoiceOutcome"; +import { routePrompt } from "../interactive/routePrompt"; +import { promptEngineChoice } from "../interactive/engineChoice"; import { normalizeAppendLinkOptions, placementSupportsFrontmatter, @@ -29,7 +31,7 @@ import { openExistingFileTab, openFile, } from "../utilityObsidian"; -import { isCancellationError, reportError } from "../utils/errorUtils"; +import { reportError } from "../utils/errorUtils"; import { ChoiceOutcomeRecorder, failureReason, @@ -49,7 +51,6 @@ import { appendLinkToFrontmatterProperty } from "../utils/frontmatterPropertyLin import { InputPromptDraftStore } from "../utils/InputPromptDraftStore"; import { TemplateEngine } from "./TemplateEngine"; import { TemplateInsertEngine } from "./TemplateInsertEngine"; -import { UserCancelError } from "../errors/UserCancelError"; import { MacroAbortError } from "../errors/MacroAbortError"; import { ChoiceAbortError } from "../errors/ChoiceAbortError"; import { handleMacroAbort } from "../utils/macroAbortHandler"; @@ -432,32 +433,40 @@ export class TemplateChoiceEngine extends TemplateEngine { return this.choice.fileExistsBehavior.mode; } - // Non-interactive run (CLI without `ui`): there is no one to answer the - // "file already exists" prompt, so opening it would hang forever. Abort with - // an actionable error instead. This is the default behaviour for new Template - // choices, so it is the most common non-interactive hang. - if (this.choiceExecutor.interactive === false) { - throw new ChoiceAbortError( - `'${this.choice.name}' needs to ask what to do because a note with that name already exists, but this run is non-interactive. ` + - `Set the choice's "If the file already exists" behaviour to a specific action (e.g. Increment, Overwrite), or re-run with the ui flag.`, - ); - } - const promptModes = getPromptModes(); - - try { - return await GenericSuggester.Suggest( - this.app, - promptModes.map((mode) => mode.label), - promptModes.map((mode) => mode.id), - "If the target file already exists", - ); - } catch (error) { - if (isCancellationError(error)) { - throw new UserCancelError("Input cancelled by user"); - } - throw error; - } + const placeholder = "If the target file already exists"; + + return (await routePrompt(this.choiceExecutor, { + // An interactive run drives this from the client, like every other prompt + // the run opens. Before, it opened on the desktop while the client's /poll + // returned nothing, and the run waited for someone to walk past (#1614). + remote: (provider) => + promptEngineChoice(provider, { + items: promptModes.map((mode) => ({ + value: mode.id, + title: mode.label, + })), + placeholder, + what: 'the "file already exists" chooser', + }), + // Non-interactive run (CLI without `ui`): there is no one to answer, so + // opening it would hang forever. Abort with an actionable error instead. + // This is the default behaviour for new Template choices, so it is the most + // common non-interactive hang. + headless: () => { + throw new ChoiceAbortError( + `'${this.choice.name}' needs to ask what to do because a note with that name already exists, but this run is non-interactive. ` + + `Set the choice's "If the file already exists" behaviour to a specific action (e.g. Increment, Overwrite), or re-run with the ui flag.`, + ); + }, + app: () => + GenericSuggester.Suggest( + this.app, + promptModes.map((mode) => mode.label), + promptModes.map((mode) => mode.id), + placeholder, + ), + })) as FileExistsModeId; } private async applyFileExistsMode( diff --git a/src/interactive/engineChoice.ts b/src/interactive/engineChoice.ts new file mode 100644 index 00000000..781dcf31 --- /dev/null +++ b/src/interactive/engineChoice.ts @@ -0,0 +1,77 @@ +/** + * The strict reply contract for a prompt an ENGINE opens over the interactive bridge. + * + * `RemotePromptProvider.suggester` is tuned for the callers it was written for — scripts + * and the formatter — and is lenient in two ways that are correct there and wrong here: + * + * 1. A missing/`null` value becomes `""` (promptProvider.ts). For a script's optional + * suggester that is a real answer ("skipped"). For an engine picker it is not an + * answer at all, and `""` is actively dangerous: it reaches + * `getFileExistsMode("")`, which throws the internal `Unknown file exists mode: `, + * and it makes the folder chooser resolve to the empty path — creating the note in + * the VAULT ROOT, where dismissing the same modal in Obsidian would have cancelled + * the run. + * 2. Any non-token string comes back verbatim, whether or not the site allows custom + * input. In Obsidian that cannot happen: `GenericSuggester` can only ever resolve an + * element of its list. Routing the prompt must not quietly widen a closed list into + * free text just because the answer now arrives over HTTP. + * + * So engine prompts validate what comes back against the list they offered. A reply that + * the in-app modal could not have produced ends the run with a message naming the fix, + * instead of being acted on. The lenient path is left exactly as it is for the script and + * formatter callers that depend on it. + */ + +import { UserCancelError } from "../errors/UserCancelError"; +import type { PromptProvider } from "./promptProvider"; + +export interface EngineChoiceItem { + /** The real item. Never crosses the wire — the provider tokenises by index. */ + value: T; + /** What the client shows. Must be a human label, not a fuzzy-search blob. */ + title: string; +} + +/** + * Ask the connected client to pick from a list the engine controls. + * + * Returns one of the `items` values, or — only when `allowCustomInput` is set — the + * client's own string. Throws {@link UserCancelError} when the client answers nothing, + * matching what dismissing the equivalent Obsidian modal does. + */ +export async function promptEngineChoice( + provider: PromptProvider, + spec: { + items: EngineChoiceItem[]; + placeholder?: string; + allowCustomInput?: boolean; + /** Names the picker in the protocol error, e.g. `the "file already exists" chooser`. */ + what: string; + }, +): Promise { + const values = spec.items.map((item) => item.value); + const answer = await provider.suggester( + spec.items.map((item) => item.title), + // The provider tokenises by index and hands back the ORIGINAL entry, so object + // identity survives even though only titles are sent. + values as unknown as string[], + spec.placeholder, + spec.allowCustomInput ?? false, + ); + + if (values.includes(answer as T)) return answer as T; + + const text = answer == null ? "" : String(answer); + // An empty reply is the shape `suggester` produces for a missing value, and no + // engine picker offers an empty row. Treat it as the dismissal it almost certainly + // is rather than acting on it — a client that means to cancel has + // `{"cancelled": true}`, which has already rejected before we get here. + if (text === "") throw new UserCancelError("Input cancelled by user"); + + if (spec.allowCustomInput) return text; + + throw new Error( + `A reply to ${spec.what} has to be one of the offered options, and "${text.slice(0, 60)}" is not. ` + + `Reply with the item's \`value\` token exactly as it was sent, or \`{"cancelled": true}\` to dismiss it.`, + ); +} diff --git a/src/interactive/interactivePromptServer.ts b/src/interactive/interactivePromptServer.ts index ddfa6a4f..a5040e8b 100644 --- a/src/interactive/interactivePromptServer.ts +++ b/src/interactive/interactivePromptServer.ts @@ -21,6 +21,7 @@ import { PROMPT_CANCELLED_MESSAGE, UserCancelError, } from "../errors/UserCancelError"; +import { ChoiceAbortError } from "../errors/ChoiceAbortError"; // 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 @@ -240,6 +241,20 @@ export type ReplyOutcome = const NO_PENDING_PROMPT = "No pending prompt for that requestId"; +/** + * Why a prompt that was waiting on the client will never be answered. + * + * Thrown as a {@link ChoiceAbortError}, not a bare `Error`, and that matters now that + * ENGINE prompts travel this bridge too (#1614). `isCancellationError` is false for a + * plain Error, so the engines' catch blocks would classify a client that simply stopped + * polling as a run FAILURE - a red "Error running choice" notice on a desktop nobody is + * sitting at, and an `error` outcome where the truth is an abort. A ChoiceAbortError + * flows through the existing abort handling and lands as `cancelled`/`aborted` carrying + * this reason. + */ +const SESSION_ENDED_REASON = + "The interactive client disconnected, so the run was ended."; + /** * Why this reply cannot be honoured, or null if it can. * @@ -378,7 +393,7 @@ class InteractivePromptServer { // already fired finish()) must reject, not park forever, so the executor // aborts instead of hanging. if (session.finished) { - return Promise.reject(new Error("Interactive session ended")); + return Promise.reject(new ChoiceAbortError(SESSION_ENDED_REASON)); } // The client already asked to end this run; do not park a new prompt it will // never answer. @@ -413,7 +428,7 @@ class InteractivePromptServer { session.pollWatchdog = null; } for (const [, pending] of session.pending) { - pending.reject(new Error("Interactive session ended")); + pending.reject(new ChoiceAbortError(SESSION_ENDED_REASON)); } session.pending.clear(); this.push(session, event); @@ -478,7 +493,9 @@ class InteractivePromptServer { if (session.attachTimer) window.clearTimeout(session.attachTimer); if (session.pollWatchdog) window.clearTimeout(session.pollWatchdog); for (const [, pending] of session.pending) { - pending.reject(new Error("QuickAdd unloaded")); + pending.reject( + new ChoiceAbortError("QuickAdd was unloaded during an interactive run."), + ); } // Close any parked long-poll response so it isn't orphaned after // server.close() (which won't terminate in-flight connections). @@ -517,7 +534,7 @@ class InteractivePromptServer { // Reject any still-pending prompt so a caller awaiting it aborts rather // than hanging (finish() normally clears these, but be defensive). for (const [, pending] of session.pending) { - pending.reject(new Error("Interactive session ended")); + pending.reject(new ChoiceAbortError(SESSION_ENDED_REASON)); } session.pending.clear(); this.sessions.delete(session.id); diff --git a/src/interactive/routePrompt.test.ts b/src/interactive/routePrompt.test.ts new file mode 100644 index 00000000..2609385a --- /dev/null +++ b/src/interactive/routePrompt.test.ts @@ -0,0 +1,178 @@ +import { describe, expect, it, vi } from "vitest"; +import { routePrompt } from "./routePrompt"; +import { promptEngineChoice } from "./engineChoice"; +import { UserCancelError } from "../errors/UserCancelError"; +import { ChoiceAbortError } from "../errors/ChoiceAbortError"; +import type { PromptProvider } from "./promptProvider"; +import type { PromptRoutingContext } from "./routePrompt"; + +const routes = () => ({ + remote: vi.fn(async () => "remote"), + headless: vi.fn(async () => "headless"), + app: vi.fn(async () => "app"), +}); + +describe("routePrompt (#1614)", () => { + it("sends an engine prompt to the connected client when one is driving the run", async () => { + const r = routes(); + const provider = {} as PromptProvider; + + await expect( + routePrompt({ promptProvider: provider, interactive: true } as PromptRoutingContext, r), + ).resolves.toBe("remote"); + expect(r.app).not.toHaveBeenCalled(); + expect(r.remote).toHaveBeenCalledWith(provider); + }); + + // `interactive = true` is what an interactive run sets, and it used to do nothing + // but DISABLE the headless guards - so the modal opened on a desktop nobody was + // watching. The provider check has to come first, not the interactive flag. + it("prefers the client over the modal even though the run is marked interactive", async () => { + const r = routes(); + + await routePrompt( + { promptProvider: {} as PromptProvider, interactive: true } as PromptRoutingContext, + r, + ); + + expect(r.remote).toHaveBeenCalledTimes(1); + expect(r.app).not.toHaveBeenCalled(); + expect(r.headless).not.toHaveBeenCalled(); + }); + + it("takes the site's own headless branch on a non-interactive run", async () => { + const r = routes(); + + await expect( + routePrompt({ interactive: false } as PromptRoutingContext, r), + ).resolves.toBe("headless"); + expect(r.app).not.toHaveBeenCalled(); + }); + + // Not every site aborts headlessly: MacroChoiceEngine runs a sole exported member + // without asking. A fixed ladder that always threw would have regressed that, which + // is why `headless` is a closure the site owns rather than a behaviour the seam picks. + it("lets a headless branch resolve instead of throwing", async () => { + await expect( + routePrompt({ interactive: false } as PromptRoutingContext, { + remote: async () => "remote", + headless: async () => "the only export", + app: async () => "app", + }), + ).resolves.toBe("the only export"); + }); + + it("opens the Obsidian modal for an ordinary in-app run", async () => { + const r = routes(); + + await expect(routePrompt({} as PromptRoutingContext, r)).resolves.toBe("app"); + }); + + it.each([ + ["app", { app: true }], + ["remote", { promptProvider: {} as PromptProvider }], + ])( + "maps a %s dismissal to UserCancelError, so every route reads the same", + async (_label, ctx) => { + const dismissal = new UserCancelError("Prompt cancelled"); + const throwing = { + remote: async () => { + throw dismissal; + }, + headless: async () => "headless", + app: async () => { + throw dismissal; + }, + }; + + await expect( + routePrompt(ctx as PromptRoutingContext, throwing), + ).rejects.toBeInstanceOf(UserCancelError); + }, + ); + + it("passes a non-cancellation failure through untouched", async () => { + const boom = new ChoiceAbortError("needs a specific action"); + + await expect( + routePrompt({ interactive: false } as PromptRoutingContext, { + remote: async () => "remote", + headless: async () => { + throw boom; + }, + app: async () => "app", + }), + ).rejects.toBe(boom); + }); +}); + +describe("promptEngineChoice enforces what the in-app modal enforces structurally", () => { + const items = [ + { value: "appendBottom", title: "Append to bottom" }, + { value: "doNothing", title: "Do nothing" }, + ]; + const what = 'the "file already exists" chooser'; + + const providerReturning = (answer: unknown) => + ({ suggester: vi.fn(async () => answer) }) as unknown as PromptProvider; + + it("returns the offered value when the client picks one", async () => { + await expect( + promptEngineChoice(providerReturning("doNothing"), { items, what }), + ).resolves.toBe("doNothing"); + }); + + // GenericSuggester can only ever resolve an element of its list. Routing the prompt + // must not quietly widen a closed list into free text just because the answer now + // arrives over HTTP - `getFileExistsMode` would throw `Unknown file exists mode:`. + it("refuses an off-list reply on a closed list, naming the fix", async () => { + await expect( + promptEngineChoice(providerReturning("Archive/Secret"), { items, what }), + ).rejects.toThrow(/has to be one of the offered options/); + }); + + // `RemotePromptProvider.suggester` maps a missing value to "" for the script callers + // that treat it as "skipped". At an engine picker there is no empty row, and acting + // on "" is how the folder chooser used to create the note in the VAULT ROOT. + it.each([[""], [null], [undefined]])( + "treats an empty reply (%s) as a dismissal, never as an answer", + async (answer) => { + await expect( + promptEngineChoice(providerReturning(answer), { items, what }), + ).rejects.toBeInstanceOf(UserCancelError); + }, + ); + + it("accepts a typed-in value only where the site allows custom input", async () => { + await expect( + promptEngineChoice(providerReturning("A brand new note"), { + items, + what, + allowCustomInput: true, + }), + ).resolves.toBe("A brand new note"); + }); + + it("still refuses an empty reply where custom input is allowed", async () => { + await expect( + promptEngineChoice(providerReturning(""), { + items, + what, + allowCustomInput: true, + }), + ).rejects.toBeInstanceOf(UserCancelError); + }); + + it("sends titles, not the underlying values, and asks for a closed list by default", async () => { + const provider = providerReturning("appendBottom"); + + await promptEngineChoice(provider, { items, what, placeholder: "Pick one" }); + + expect(provider.suggester).toHaveBeenCalledWith( + ["Append to bottom", "Do nothing"], + ["appendBottom", "doNothing"], + "Pick one", + false, + ); + }); +}); diff --git a/src/interactive/routePrompt.ts b/src/interactive/routePrompt.ts new file mode 100644 index 00000000..b5ec514b --- /dev/null +++ b/src/interactive/routePrompt.ts @@ -0,0 +1,73 @@ +/** + * Where a prompt opened by an ENGINE goes. + * + * `quickadd:interactive` promises to forward a run's prompts to the connected client, + * and it kept that promise for every prompt a *script* or the *formatter* opens, because + * each of those hand-wrote an `if (choiceExecutor.promptProvider)` branch at its call + * site. The engines never got one. So a Template whose target note already existed + * opened its "file already exists" chooser on the desktop while the client's `/poll` + * returned nothing, and the run sat there until someone walked past the machine — on a + * headless or Raycast setup, the entire premise of the seam, until the watchdog gave up + * (#1614). + * + * The fix is a seam rather than six more hand-written branches, because "remember to add + * the branch" had already been missed six times. But the seam deliberately owns only the + * part that is genuinely uniform: + * + * - the ORDER of the three destinations, and + * - the `isCancellationError` → {@link UserCancelError} mapping every site repeated. + * + * Everything else stays at the call site. A wrapper that *built* the modal would have to + * model every per-site difference as an option it understands — and the variance here is + * entirely in modal construction (`renderItem`, `searchItems`, `valueExists`, + * `customValueLabel`, `allowCustomValue`) and in the headless branch, which is not + * uniform either: most sites abort, but `MacroChoiceEngine` legitimately RESOLVES a sole + * exported member without asking. A fixed three-step ladder would have regressed that. + * So each site passes three closures and keeps its own modal call verbatim. + */ + +import type { IChoiceExecutor } from "../IChoiceExecutor"; +import { UserCancelError } from "../errors/UserCancelError"; +import { isCancellationError } from "../utils/errorUtils"; +import type { PromptProvider } from "./promptProvider"; + +export interface PromptRoute { + /** Ask the connected client. Only called when a provider is attached. */ + remote(provider: PromptProvider): Promise; + /** + * Nobody can answer (a non-interactive CLI run). Throws the site's own + * `ChoiceAbortError` with the text that says how to configure the choice so it + * stops needing to ask — or resolves, where the site has one unambiguous answer. + */ + headless(): Promise; + /** Open the Obsidian modal. */ + app(): Promise; +} + +/** + * The executor is required, not optional: an engine that cannot supply one is a compile + * error rather than a prompt that silently opens on the desktop. + */ +export type PromptRoutingContext = Pick< + IChoiceExecutor, + "promptProvider" | "interactive" +>; + +export async function routePrompt( + executor: PromptRoutingContext, + route: PromptRoute, +): Promise { + try { + if (executor.promptProvider) return await route.remote(executor.promptProvider); + if (executor.interactive === false) return await route.headless(); + return await route.app(); + } catch (error) { + // One mapping for all three destinations, so a dismissal reads the same to the + // engine whether it came from Escape on a modal or `{"cancelled":true}` on the + // wire. Every converted site used to repeat this catch verbatim. + if (isCancellationError(error)) { + throw new UserCancelError("Input cancelled by user"); + } + throw error; + } +} From e93b1ea8a295419cfc4eeb69f6e87bf07e3fac4f Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Tue, 28 Jul 2026 01:26:32 +0200 Subject: [PATCH 4/8] fix: route the rest of a run's stranding prompts to the interactive client Four more prompts an engine opens now go to the connected client instead of the desktop. Each was confirmed to strand a real run first, by starting `quickadd:interactive` in an isolated vault and checking both sides: the client's `/poll` returned nothing while `document.querySelectorAll(".modal-container")` showed the modal sitting in Obsidian. - the folder chooser (TemplateEngine) - the note-discovery picker (templateNoteDiscovery) - the highest-value one, since the one-page preflight deliberately filters its requirement out, so an interactive run collected NOTHING and walked into a desktop list of every note - the heading picker (CaptureChoiceEngine) - the user-script member picker (MacroChoiceEngine) The capture-target pickers are deliberately NOT converted. A remote run forces the one-page preflight, which already collects the target as a first-class `__qa.captureTargetFilePath` requirement - verified live: the client receives a `form` prompt and no modal opens. Routing them would have meant bypassing `confinePreselectedToScope`, the confinement this codebase already requires for a capture target that arrives from outside the picker, to fix a symptom that does not occur. Three details the conversion forced: - The discovery picker's `display` is a fuzzy-SEARCH blob (basename + path + aliases) and the visible row comes from `renderItem`, which does not travel. It now carries an explicit `title`, so a client shows "Areas/Work/Tom.md" rather than "Tom Areas/Work/Tom.md Thomas". - The folder chooser's re-prompt loop is unbounded because a Notice tells the user why their pick was refused. A client gets no Notice, so an identical re-prompt is indistinguishable from a hang; a remote run is bounded at three attempts and then ends with the text the Notice carries, via one shared message. - The member picker validates with `Object.hasOwn` before indexing the script module, since a reply that used to come from a fixed key list now arrives over the wire. Also folds in a one-line guard for the AI tool-confirmation modal, which is worse than #1614 rather than an instance of it: it has neither a provider route nor a non-interactive guard, so a plain headless `quickadd:run` of a Macro whose script calls `quickAddApi.ai.agent` with a non-readOnly tool never returned at all. Refs #1614 --- src/ai/tools/Agent.ts | 13 ++ src/engine/CaptureChoiceEngine.ts | 66 +++---- src/engine/MacroChoiceEngine.ts | 56 +++--- src/engine/TemplateChoiceEngine.ts | 15 +- src/engine/TemplateEngine.ts | 162 +++++++++++------- ...mplateNoteDiscovery.audit-template.test.ts | 8 +- src/engine/templateNoteDiscovery.test.ts | 14 +- src/engine/templateNoteDiscovery.ts | 53 +++++- 8 files changed, 261 insertions(+), 126 deletions(-) diff --git a/src/ai/tools/Agent.ts b/src/ai/tools/Agent.ts index 39e50d38..a9228934 100644 --- a/src/ai/tools/Agent.ts +++ b/src/ai/tools/Agent.ts @@ -13,6 +13,7 @@ import type { IChoiceExecutor } from "../../IChoiceExecutor"; import { settingsStore } from "../../settingsStore"; import { CompleteFormatter } from "../../formatters/completeFormatter"; import { makeNoticeHandler } from "../makeNoticeHandler"; +import { ChoiceAbortError } from "src/errors/ChoiceAbortError"; import { MacroAbortError } from "../../errors/MacroAbortError"; import { resolveModelInputOrThrow } from "../aiHelpers"; import type { AIProvider, Model } from "../Provider"; @@ -373,6 +374,18 @@ export class Agent { if (!needs) return true; if (this.approveAllThisRun && !perToolFloor) return true; + // Non-interactive run (CLI without `ui`): nobody can approve this, and the + // modal would park forever - a plain `quickadd:run` of a Macro whose script + // calls `quickAddApi.ai.agent` with a non-readOnly tool simply never returned + // (`needsGlobalConfirmation` defaults to "destructive", so most tools ask). + // Refuse with the text that says how to make the choice runnable headlessly. + if (this.choiceExecutor.interactive === false) { + throw new ChoiceAbortError( + `'${call.name}' needs approval before it runs, but this run is non-interactive. ` + + `Set the AI assistant's tool-confirmation to "never", mark the tool readOnly, or re-run with the ui flag.`, + ); + } + const outcome = await AIToolConfirmModal.Prompt( this.app, call.name, diff --git a/src/engine/CaptureChoiceEngine.ts b/src/engine/CaptureChoiceEngine.ts index 1111e7e3..f6f614a7 100644 --- a/src/engine/CaptureChoiceEngine.ts +++ b/src/engine/CaptureChoiceEngine.ts @@ -56,6 +56,8 @@ import { failureReason, } from "./choiceOutcomeRecorder"; import type { ChoiceEffect } from "../types/ChoiceOutcome"; +import { routePrompt } from "../interactive/routePrompt"; +import { promptEngineChoice } from "../interactive/engineChoice"; import type { FieldFilter } from "../utils/FieldSuggestionParser"; import { resolveCaptureTarget as resolveCaptureTargetFromString, @@ -884,15 +886,6 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { const insertAfter = this.choice.insertAfter; if (!insertAfter?.enabled || !insertAfter.promptHeading) return; - // Non-interactive run (CLI without `ui`): the heading picker has no one to - // answer it, so opening it would hang. Abort with an actionable error. - if (this.choiceExecutor.interactive === false) { - throw new ChoiceAbortError( - `'${this.choice.name}' needs to ask which heading to capture under, but this run is non-interactive. ` + - `Turn off "Choose heading when capturing" and set a fixed heading, or re-run with the ui flag.`, - ); - } - const allowCreate = !!insertAfter.createIfNotFound; const lines = getLinesInString(content); @@ -903,28 +896,41 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { ); const headingTexts = headings.map((h) => h.text); - let chosen: string; - try { - chosen = await InputSuggester.Suggest( - this.app, - headingDisplay, - headingLines, - { - allowCustomValue: allowCreate, - placeholder: "Choose a heading to insert under", - emptyStateText: allowCreate - ? "No headings found — type a heading to create" - : "No headings found in the target note", - customValueLabel: (value) => - `Insert after new line: ${value}`, + const placeholder = "Choose a heading to insert under"; + const chosen = String( + await routePrompt(this.choiceExecutor, { + // Routed like every other prompt the run opens. Before, an interactive + // run opened this on the desktop while the client's /poll returned + // nothing (#1614). + remote: (provider) => + promptEngineChoice(provider, { + items: headingLines.map((line, index) => ({ + value: line, + title: headingDisplay[index] ?? line, + })), + placeholder, + allowCustomInput: allowCreate, + what: "the heading picker", + }), + // Non-interactive run (CLI without `ui`): no one can answer, so opening + // it would hang. Abort with an actionable error. + headless: () => { + throw new ChoiceAbortError( + `'${this.choice.name}' needs to ask which heading to capture under, but this run is non-interactive. ` + + `Turn off "Choose heading when capturing" and set a fixed heading, or re-run with the ui flag.`, + ); }, - ); - } catch (error) { - if (isCancellationError(error)) { - throw new UserCancelError("Input cancelled by user"); - } - throw error; - } + app: () => + InputSuggester.Suggest(this.app, headingDisplay, headingLines, { + allowCustomValue: allowCreate, + placeholder, + emptyStateText: allowCreate + ? "No headings found — type a heading to create" + : "No headings found in the target note", + customValueLabel: (value) => `Insert after new line: ${value}`, + }), + }), + ); invariant( !!chosen && chosen.length > 0, diff --git a/src/engine/MacroChoiceEngine.ts b/src/engine/MacroChoiceEngine.ts index 5bf7944e..da420b0a 100644 --- a/src/engine/MacroChoiceEngine.ts +++ b/src/engine/MacroChoiceEngine.ts @@ -11,6 +11,8 @@ import type { ICommand } from "../types/macros/ICommand"; import { QuickAddChoiceEngine } from "./QuickAddChoiceEngine"; import type { IMacro } from "../types/macros/IMacro"; import GenericSuggester from "../gui/GenericSuggester/genericSuggester"; +import { routePrompt } from "../interactive/routePrompt"; +import { promptEngineChoice } from "../interactive/engineChoice"; import type { IChoiceCommand } from "../types/macros/IChoiceCommand"; import type QuickAdd from "../main"; import { getQuickAddInstance } from "../quickAddInstance"; @@ -524,31 +526,43 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine { return; } - // Non-interactive run (CLI without `ui`): a user script that exports an object - // of MULTIPLE named members opens a picker to choose which to run, which has - // no one to answer it headlessly. A single-member export is unambiguous, so - // run it directly (the interactive path keeps its existing picker behaviour). - if (this.choiceExecutor.interactive === false) { - const keys = Object.keys(obj); - if (keys.length === 1) { - await this.userScriptDelegator(obj[keys[0]]); - return; - } - throw new ChoiceAbortError( - "This macro's user script exports multiple members and needs to ask which one to run, but this run is non-interactive. " + - "Reference a single member (e.g. myScript::start), or re-run with the ui flag.", - ); - } + const keys = Object.keys(obj); try { - const keys = Object.keys(obj); - const selected: string = await GenericSuggester.Suggest( - this.app, - keys, - keys, - this.promptLabel, + const selected = String( + await routePrompt(this.choiceExecutor, { + // Routed like the run's other prompts, instead of opening on a desktop + // nobody is watching during an interactive run (#1614). + remote: (provider) => + promptEngineChoice(provider, { + items: keys.map((key) => ({ value: key, title: key })), + placeholder: this.promptLabel, + what: "the user-script member picker", + }), + // A single-member export is unambiguous, so a headless run just runs + // it. This is why the seam takes a closure per destination rather than + // imposing one headless behaviour: most sites abort here, and this one + // legitimately answers itself. + headless: () => { + if (keys.length === 1) return Promise.resolve(keys[0]); + throw new ChoiceAbortError( + "This macro's user script exports multiple members and needs to ask which one to run, but this run is non-interactive. " + + "Reference a single member (e.g. myScript::start), or re-run with the ui flag.", + ); + }, + app: () => + GenericSuggester.Suggest(this.app, keys, keys, this.promptLabel), + }), ); + // `Object.hasOwn`, not `obj[selected]`: the reply now travels over the wire, + // and a member name like "constructor" would otherwise resolve to something + // that is not an exported script at all. + if (!Object.hasOwn(obj, selected)) { + throw new Error( + `This macro's user script does not export a member named "${selected}".`, + ); + } await this.userScriptDelegator(obj[selected]); } catch (err) { if (err instanceof MacroAbortError) { diff --git a/src/engine/TemplateChoiceEngine.ts b/src/engine/TemplateChoiceEngine.ts index 3f1a6060..082de809 100644 --- a/src/engine/TemplateChoiceEngine.ts +++ b/src/engine/TemplateChoiceEngine.ts @@ -115,6 +115,7 @@ export class TemplateChoiceEngine extends TemplateEngine { const discovery = await promptForTemplateNoteDiscovery( this.app, this.choice, + this.choiceExecutor, ); if (discovery.kind === "openExisting") { await this.openDiscoveredExistingNote(discovery.file); @@ -702,9 +703,9 @@ export class TemplateChoiceEngine extends TemplateEngine { ]); const currentFolder = this.getCurrentFolderSuggestion(); const topItems = currentFolder ? [currentFolder] : []; - // Propagate non-interactivity so a folder chooser aborts (instead of hanging) - // in a headless CLI run; a single configured folder never prompts regardless. - const interactive = this.choiceExecutor.interactive; + // Where the folder chooser goes: the client on an interactive run, an abort on + // a headless one, the modal otherwise. A single configured folder never prompts. + const executor = this.choiceExecutor; if ( this.choice.folder?.chooseFromSubfolders && @@ -726,7 +727,7 @@ export class TemplateChoiceEngine extends TemplateEngine { allowCreate: true, allowedRoots: folders, topItems, - interactive, + executor, }); } @@ -737,7 +738,7 @@ export class TemplateChoiceEngine extends TemplateEngine { return await this.getOrCreateFolder(allFoldersInVault, { allowCreate: true, topItems, - interactive, + executor, }); } @@ -754,7 +755,7 @@ export class TemplateChoiceEngine extends TemplateEngine { return await this.getOrCreateFolder([activeFile.parent.path], { allowCreate: true, topItems, - interactive, + executor, }); } @@ -762,7 +763,7 @@ export class TemplateChoiceEngine extends TemplateEngine { allowCreate: true, allowedRoots: folders, topItems, - interactive, + executor, }); } diff --git a/src/engine/TemplateEngine.ts b/src/engine/TemplateEngine.ts index 0c0c1ab3..d74e2b04 100644 --- a/src/engine/TemplateEngine.ts +++ b/src/engine/TemplateEngine.ts @@ -36,25 +36,49 @@ import { isReservedWindowsDeviceName, } from "../utils/pathValidation"; import { MacroAbortError } from "../errors/MacroAbortError"; -import { UserCancelError } from "../errors/UserCancelError"; import { ChoiceAbortError } from "../errors/ChoiceAbortError"; -import { isCancellationError } from "../utils/errorUtils"; import type { IChoiceExecutor } from "../IChoiceExecutor"; +import { + routePrompt, + type PromptRoutingContext, +} from "../interactive/routePrompt"; +import { promptEngineChoice } from "../interactive/engineChoice"; import { log } from "../logger/logManager"; import { assertCreatableFilePath } from "./assertCreatableFilePath"; +/** One wording for both surfaces: the desktop Notice, and the remote run's abort. */ +function folderNotAllowedMessage(roots: string[]): string { + const displayRoots = roots.map((root) => (root ? root : "/")); + const list = + displayRoots.length > 3 + ? `${displayRoots.slice(0, 3).join(", ")}...` + : displayRoots.join(", "); + return `Folder must be under: ${list}`; +} + +/** + * How many times a REMOTE run may re-ask the folder chooser before giving up. The + * in-app loop is unbounded because the Notice tells the user why their pick was + * refused; a client sees an identical prompt with no explanation, so looping there + * is indistinguishable from a hang. + */ +const MAX_REMOTE_FOLDER_ATTEMPTS = 3; + type FolderChoiceOptions = { allowCreate?: boolean; placeholder?: string; allowedRoots?: string[]; topItems?: Array<{ path: string; label: string }>; /** - * When `false`, refuse to open the folder-chooser suggester (no one can answer - * it in a non-interactive CLI run) and abort with a clear error instead of - * hanging. Defaults to interactive. A single configured folder never prompts, - * so it is unaffected regardless of this flag. + * Where the folder chooser goes: to a connected interactive client, to an abort + * (a non-interactive CLI run has no one to answer it), or to the Obsidian modal. + * + * Required, not optional. It replaced a bare `interactive?: boolean` that could + * only ever say "do not open the modal" - which is why an INTERACTIVE run, where + * the flag is `true`, opened the chooser on a desktop nobody was watching (#1614). + * A single configured folder never prompts, so it is unaffected either way. */ - interactive?: boolean; + executor: PromptRoutingContext; }; type FolderSelectionContext = { @@ -120,7 +144,7 @@ export abstract class TemplateEngine extends QuickAddEngine { protected async getOrCreateFolder( folders: string[], - options: FolderChoiceOptions = {}, + options: FolderChoiceOptions, ): Promise { const context = this.buildFolderSelectionContext(folders, options); @@ -128,16 +152,7 @@ export abstract class TemplateEngine extends QuickAddEngine { return await this.handleSingleSelection(context); } - // Non-interactive run (CLI without `ui`): the folder chooser has no one to - // answer it, so opening it would hang. Abort with an actionable error. - if (options.interactive === false) { - throw new ChoiceAbortError( - "This choice needs to ask which folder to create the note in, but this run is non-interactive. " + - "Configure a single target folder, or re-run with the ui flag.", - ); - } - - const selection = await this.promptUntilAllowed(context); + const selection = await this.promptUntilAllowed(context, options.executor); return selection.isEmpty ? "" : selection.resolved; } @@ -181,40 +196,64 @@ export abstract class TemplateEngine extends QuickAddEngine { ); } - private async promptForFolder(context: FolderSelectionContext): Promise { - try { - if (context.allowCreate) { - return await InputSuggester.Suggest( - this.app, - context.displayItems, - context.items, - { - placeholder: - context.placeholder ?? "Choose a folder or type to create one", - renderItem: (item, el) => { - this.renderFolderSuggestion( - item, - el, - context.existingSet, - context.displayByNormalized, - ); - }, - }, - ); - } - - return await GenericSuggester.Suggest( - this.app, - context.displayItems, - context.items, - context.placeholder, - ); - } catch (error) { - if (isCancellationError(error)) { - throw new UserCancelError("Input cancelled by user"); - } - throw error; - } + private async promptForFolder( + context: FolderSelectionContext, + executor: PromptRoutingContext, + ): Promise { + const placeholder = + context.placeholder ?? "Choose a folder or type to create one"; + + return String( + await routePrompt(executor, { + // The client renders its own list, so the in-app "Create folder" badge + // does not travel - but `allowCreate` does, as `allowCustomInput`, which + // is the part that changes what the run can DO. Every reply still goes + // through resolveSelection -> allowedRoots -> validateFolderPath below, + // so a routed answer is confined exactly like a typed-in one. + remote: (provider) => + promptEngineChoice(provider, { + items: context.items.map((item, index) => ({ + value: item, + title: context.displayItems[index] ?? item, + })), + placeholder, + allowCustomInput: context.allowCreate, + what: "the folder chooser", + }), + // Non-interactive run (CLI without `ui`): no one can answer, so opening + // it would hang. Abort with an actionable error. + headless: () => { + throw new ChoiceAbortError( + "This choice needs to ask which folder to create the note in, but this run is non-interactive. " + + "Configure a single target folder, or re-run with the ui flag.", + ); + }, + app: () => + context.allowCreate + ? InputSuggester.Suggest( + this.app, + context.displayItems, + context.items, + { + placeholder, + renderItem: (item, el) => { + this.renderFolderSuggestion( + item, + el, + context.existingSet, + context.displayByNormalized, + ); + }, + }, + ) + : GenericSuggester.Suggest( + this.app, + context.displayItems, + context.items, + context.placeholder, + ), + }), + ); } private async resolveSelection( @@ -248,10 +287,20 @@ export abstract class TemplateEngine extends QuickAddEngine { private async promptUntilAllowed( context: FolderSelectionContext, + executor: PromptRoutingContext, ): Promise { + // A rejected selection is re-asked, and in Obsidian the reason arrives as a + // Notice next to the reopened modal. A remote client gets no Notice, so the + // re-prompt is indistinguishable from the first one - an unbounded loop of an + // identical question. Bound it there and end with the text the Notice carries. + let remoteAttempts = executor.promptProvider ? MAX_REMOTE_FOLDER_ATTEMPTS : 0; + // Keep prompting until the user provides an allowed selection or cancels. for (;;) { - const raw = await this.promptForFolder(context); + if (executor.promptProvider && remoteAttempts-- <= 0) { + throw new ChoiceAbortError(folderNotAllowedMessage(context.allowedRoots)); + } + const raw = await this.promptForFolder(context, executor); const selection = await this.resolveSelection(raw, context); if (selection.isEmpty) { @@ -381,12 +430,7 @@ export abstract class TemplateEngine extends QuickAddEngine { } private showFolderNotAllowedNotice(roots: string[]): void { - const displayRoots = roots.map((root) => (root ? root : "/")); - const list = - displayRoots.length > 3 - ? `${displayRoots.slice(0, 3).join(", ")}...` - : displayRoots.join(", "); - new Notice(`Folder must be under: ${list}`); + new Notice(folderNotAllowedMessage(roots)); } private buildFolderSuggestions( diff --git a/src/engine/templateNoteDiscovery.audit-template.test.ts b/src/engine/templateNoteDiscovery.audit-template.test.ts index 6e6472fe..78e3b648 100644 --- a/src/engine/templateNoteDiscovery.audit-template.test.ts +++ b/src/engine/templateNoteDiscovery.audit-template.test.ts @@ -18,6 +18,10 @@ import { type App, type TFile } from "obsidian"; import type ITemplateChoice from "src/types/choices/ITemplateChoice"; import { promptForTemplateNoteDiscovery } from "./templateNoteDiscovery"; +// An ordinary in-app run: no interactive client attached and not headless, so the +// picker opens the Obsidian modal - exactly the path these tests exercise. +const IN_APP_RUN = {} as never; + function choice(overrides: Partial = {}): ITemplateChoice { return { id: "template", @@ -78,7 +82,7 @@ describe("template note discovery — typed name vault-relative parity (audit)", // (vault-relative), not anchor under the configured/default folder. inputSuggestMock.mockResolvedValue("Projects/My New Note"); - const result = await promptForTemplateNoteDiscovery(app([]), choice()); + const result = await promptForTemplateNoteDiscovery(app([]), choice(), IN_APP_RUN); expect(result).toEqual({ kind: "create", @@ -90,7 +94,7 @@ describe("template note discovery — typed name vault-relative parity (audit)", it("leaves a plain typed name without a vault-relative path", async () => { inputSuggestMock.mockResolvedValue("My New Note"); - const result = await promptForTemplateNoteDiscovery(app([]), choice()); + const result = await promptForTemplateNoteDiscovery(app([]), choice(), IN_APP_RUN); expect(result).toEqual({ kind: "create", title: "My New Note" }); }); diff --git a/src/engine/templateNoteDiscovery.test.ts b/src/engine/templateNoteDiscovery.test.ts index 54a9e35f..66d72e8b 100644 --- a/src/engine/templateNoteDiscovery.test.ts +++ b/src/engine/templateNoteDiscovery.test.ts @@ -17,10 +17,14 @@ vi.mock("obsidian-dataview", () => ({ import { TFile, type App } from "obsidian"; import type ITemplateChoice from "src/types/choices/ITemplateChoice"; import { + promptForTemplateNoteDiscovery, shouldRunTemplateNoteDiscovery, testExports, } from "./templateNoteDiscovery"; +// An ordinary in-app run: no interactive client attached and not headless, so the +// picker opens the Obsidian modal - exactly the path these tests exercise. +const IN_APP_RUN = {} as never; function file(path: string): TFile { const tfile = new TFile(); @@ -189,7 +193,7 @@ describe("template note discovery", () => { const alice = file("People/Alice.md"); inputSuggestMock.mockImplementation(async (_app, _display, items) => items[0]); - const result = await promptForTemplateNoteDiscovery(app([alice]), choice()); + const result = await promptForTemplateNoteDiscovery(app([alice]), choice(), IN_APP_RUN); expect(result).toEqual({ kind: "openExisting", file: alice }); }); @@ -200,7 +204,7 @@ describe("template note discovery", () => { items.find((item: string) => item.includes("Missing Project")), ); - const result = await promptForTemplateNoteDiscovery(app([alice]), choice()); + const result = await promptForTemplateNoteDiscovery(app([alice]), choice(), IN_APP_RUN); expect(result).toEqual({ kind: "create", title: "Missing Project" }); }); @@ -211,7 +215,7 @@ describe("template note discovery", () => { items.find((item: string) => item.includes("Projects/Missing Roadmap")), ); - const result = await promptForTemplateNoteDiscovery(app([alice]), choice()); + const result = await promptForTemplateNoteDiscovery(app([alice]), choice(), IN_APP_RUN); expect(result).toEqual({ kind: "create", @@ -228,7 +232,7 @@ describe("template note discovery", () => { inputSuggestMock.mockImplementation(async (_app, _display, items) => items[0]); await expect( - promptForTemplateNoteDiscovery(staleApp, choice()), + promptForTemplateNoteDiscovery(staleApp, choice(), IN_APP_RUN), ).rejects.toThrow("Selected note no longer exists"); }); @@ -236,7 +240,7 @@ describe("template note discovery", () => { const alice = file("People/Alice.md"); inputSuggestMock.mockResolvedValue("Fresh Idea"); - await promptForTemplateNoteDiscovery(app([alice]), choice()); + await promptForTemplateNoteDiscovery(app([alice]), choice(), IN_APP_RUN); const options = inputSuggestMock.mock.calls[0]?.[3]; expect(options.valueExists("Alice")).toBe(true); diff --git a/src/engine/templateNoteDiscovery.ts b/src/engine/templateNoteDiscovery.ts index 313a0a6c..3fbd3369 100644 --- a/src/engine/templateNoteDiscovery.ts +++ b/src/engine/templateNoteDiscovery.ts @@ -1,5 +1,11 @@ import { TFile, type App } from "obsidian"; import InputSuggester from "src/gui/InputSuggester/inputSuggester"; +import { + routePrompt, + type PromptRoutingContext, +} from "../interactive/routePrompt"; +import { promptEngineChoice } from "../interactive/engineChoice"; +import { ChoiceAbortError } from "../errors/ChoiceAbortError"; import { renderNotePathSuggestion } from "src/gui/InputSuggester/renderNotePathSuggestion"; import { orderFilesForPicker } from "src/utils/fileOrdering"; import { buildPickerOrderingDeps } from "src/utils/pickerOrderingDeps"; @@ -23,7 +29,16 @@ export type TemplateNoteDiscoveryResult = type DiscoveryCandidate = { item: string; + /** + * The fuzzy-SEARCH text, not a label: basename + path + aliases, joined. The + * in-app picker feeds this to the matcher and draws the visible row from + * `renderItem` instead. A remote client has no `renderItem`, so it gets + * {@link DiscoveryCandidate.title} - sending `display` there would have shown + * rows reading "Tom Areas/Work/Tom.md Thomas" (#1614). + */ display: string; + /** The human label: the note's basename, or the unresolved link's target. */ + title: string; renderPath?: string; renderAlias?: string; unresolvedTitle?: string; @@ -162,6 +177,7 @@ function buildDiscoveryCandidates(app: App, choice: ITemplateChoice): { candidates.push({ item: encodeExisting(file.path), display: searchable, + title: aliases[0] ? `${file.basename} (${file.path})` : file.path, renderPath: file.path, renderAlias: aliases[0], }); @@ -173,6 +189,7 @@ function buildDiscoveryCandidates(app: App, choice: ITemplateChoice): { candidates.push({ item: encodeUnresolved(target), display: target, + title: `${target} (unresolved link)`, unresolvedTitle: target, }); } @@ -202,19 +219,49 @@ function renderExistingSuggestion( export async function promptForTemplateNoteDiscovery( app: App, choice: ITemplateChoice, + executor: PromptRoutingContext, ): Promise { const { candidates, existingKeys } = buildDiscoveryCandidates(app, choice); const candidateByItem = new Map( candidates.map((candidate) => [candidate.item, candidate]), ); + const placeholder = `Search notes or create ${choice.name}`; + try { - const selected = await InputSuggester.Suggest( + const selected = String( + await routePrompt(executor, { + // This picker is the one the one-page preflight deliberately does NOT + // pre-collect (it filters the `value` requirement out for discovery), so + // before this an interactive run collected nothing and walked straight + // into a desktop list of every note in the vault (#1614). + remote: (provider) => + promptEngineChoice(provider, { + items: candidates.map((candidate) => ({ + value: candidate.item, + title: candidate.title, + })), + placeholder, + // "Create new note" - the whole point of the picker. + allowCustomInput: true, + what: "the note-discovery picker", + }), + // A headless run never reaches here: `shouldRunTemplateNoteDiscovery` + // needs an unresolved `value`, which the CLI refuses up front as a + // missing input. Guarded anyway, so the branch cannot become a hang. + headless: () => { + throw new ChoiceAbortError( + `'${choice.name}' needs to ask which note to open or create, but this run is non-interactive. ` + + `Pass the note name (e.g. value-value=), or re-run with the ui flag.`, + ); + }, + app: () => + InputSuggester.Suggest( app, candidates.map((candidate) => candidate.display), candidates.map((candidate) => candidate.item), { - placeholder: `Search notes or create ${choice.name}`, + placeholder, allowCustomValue: true, customValueLabel: (value) => `Create new note: ${value}`, valueExists: (value) => { @@ -244,6 +291,8 @@ export async function promptForTemplateNoteDiscovery( } }, }, + ), + }), ); if (isExistingItem(selected)) { From a9160a285d57a43a3225c2f705c60c3f0f564361 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Tue, 28 Jul 2026 01:27:18 +0200 Subject: [PATCH 5/8] docs: describe engine prompts on the interactive bridge, and what /abort now reaches Refs #1614 --- docs/src/content/docs/docs/Advanced/CLI.md | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/docs/src/content/docs/docs/Advanced/CLI.md b/docs/src/content/docs/docs/Advanced/CLI.md index c53d2a21..66371225 100644 --- a/docs/src/content/docs/docs/Advanced/CLI.md +++ b/docs/src/content/docs/docs/Advanced/CLI.md @@ -65,7 +65,7 @@ obsidian vault=dev quickadd:run-template \ - `path=` is the template file (vault-relative). A leading slash is allowed and a missing `.md` extension is added, matching how Template choices resolve paths. If no file resolves there, the command returns `{"ok":false}` up front. - The new note's name comes from `{{VALUE}}` - pass it as `value-value=...`. A non-interactive run with an empty or missing name returns `missingFlags` instead of creating an unnamed note. The note is created in Obsidian's "Default location for new notes". - The picker (interactive command) only lists templates inside your configured template folder(s); `path=` here is explicit, so any vault file resolves. -- Like `quickadd:run`, name collisions on the target note still prompt interactively (the file-exists choice is not a pre-collected input). +- Like `quickadd:run`, name collisions on the target note still prompt (the file-exists choice is not a pre-collected input). Under `quickadd:interactive` that prompt is forwarded to you like any other. _Introduced in QuickAdd 2.14.0._ @@ -175,6 +175,13 @@ background. Attach to the session and drive it: - `POST http://127.0.0.1:/reply?session=&token=` with body `{"requestId":…,"value":…}` to answer, or `{"requestId":…,"cancelled":true}` to cancel (which ends the run - except on an `info` panel, see below). - `POST http://127.0.0.1:/abort?session=&token=` - end the run. Answers `{"ok":true,"interrupted":}`, where `n` is how many pending prompts it rejected; `409` if the run had already finished (benign - poll for the terminal event); `404` for an unknown session or token, or for any method other than `POST`. +Prompts a Template or Capture run opens itself - the "file already exists" chooser, +the folder picker, the note-discovery picker, the heading picker - are forwarded like +any other. They arrive as `suggester` prompts, and because the engine controls the +list, a reply that is not one of the offered `value` tokens is refused rather than +acted on (unless the prompt sets `allowCustomInput`, as the folder and discovery +pickers do so you can create something new). + Prompt `type`s and the `value` you reply with: `suggester`/`input`/`date` → string, `confirm` → boolean, `checkbox` → string array, `info` → acknowledgement, `form` → an object mapping each field's `id` to its string @@ -201,12 +208,9 @@ whether it stopped anything. :::caution[What `/abort` cannot reach] `/abort` interrupts prompts that were routed **to you**. A run that is mid-work -between prompts keeps going, and a Template or Capture run still opens some prompts in -Obsidian itself - the "file already exists" chooser, the folder picker, the -capture-target picker - which do not travel over this bridge -([#1614](https://github.com/chhoumann/quickadd/issues/1614)). So `"interrupted":0` -means nothing was waiting on you, and the run may still succeed and commit its side -effects. +between prompts keeps going, so `"interrupted":0` means nothing was waiting on you and +the run may still finish and commit its side effects. Keep polling for the terminal +event either way. ::: :::note[Available in the next release] From 0254d0242090e694aa0b3d2a9fa4872e86284677 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Tue, 28 Jul 2026 01:47:47 +0200 Subject: [PATCH 6/8] fix: route the capture-target pickers too, and correct three claims review found false An adversarial pass took apart the previous commit's reasoning, and it was right about the biggest piece: the capture-target pickers DO strand a remote run. The justification for leaving them - "the forced one-page preflight already collects the target" - holds only while the preflight actually runs and can classify the target. Two shipped configurations break it, both reproduced live: - "Capture to" carrying format syntax (`Journal/{{DATE:YYYY-MM}}/`): `classifyCaptureTargetScope` returns null for any target containing `{{...}}`, so no requirement is collected and the picker opens at run time. - a choice whose one-page input is set to "never": `shouldUseOnePager` is false even for a remote run, so there is no preflight at all. Both now route, and the confinement the earlier design review warned about is preserved rather than bypassed. The folder-scoped picker needed nothing extra - every reply is re-prefixed into the folder afterwards, exactly what `confinePreselectedToScope` does. The tag/property picker did: nothing downstream re-confines it, and in Obsidian the rule is enforced structurally by `valueExists` suppressing the "Create new note" row for a name that already exists, so a typed value can only ever create a NEW note. A routed reply naming an existing note the scope does not match is now refused with that same rule. Three other corrections: - **`unchanged` closes the outcome again.** The previous commit exempted it on the grounds that a no-op "left nothing a retry could duplicate". False: the run continues past the commit point into the append-link step, which writes to a DIFFERENT note. Letting a later failure overwrite the recorded success put the caller straight back to retrying a run that already had side effects. - **`effect` is about the choice's TARGET, not the whole vault**, and now says so. Append-link and copy-link are real writes this flag does not describe; claiming the vault was unchanged while a link was appended elsewhere was the same kind of overclaim the flag exists to remove. - **A post-commit rewrite counts as a change.** Whole-file Templater (`afterCapture: "wholeFile"`) rewrites the note AFTER the bytes were compared, so a note containing `<% %>` really did change even when the payload was a no-op. Plus: an empty reply is checked BEFORE list membership, because a folder list can legitimately contain "" and that is the one reply whose two readings differ most ("the client sent nothing" vs "create at the vault root"); the folder chooser's remote abort carries the reason that actually fired instead of always blaming the allowed roots; and two comments and the CLI docs that the previous commit left describing behaviour the code no longer has. Refs #1614 Refs #1615 --- docs/src/content/docs/docs/Advanced/CLI.md | 16 +- src/engine/CaptureChoiceEngine.ts | 181 +++++++++++++-------- src/engine/TemplateChoiceEngine.ts | 11 +- src/engine/TemplateEngine.ts | 12 +- src/engine/choiceOutcomeRecorder.test.ts | 20 ++- src/engine/choiceOutcomeRecorder.ts | 10 +- src/interactive/engineChoice.ts | 15 +- src/interactive/interactivePromptServer.ts | 9 +- src/interactive/routePrompt.test.ts | 15 ++ src/types/ChoiceOutcome.ts | 13 +- 10 files changed, 193 insertions(+), 109 deletions(-) diff --git a/docs/src/content/docs/docs/Advanced/CLI.md b/docs/src/content/docs/docs/Advanced/CLI.md index 66371225..51ad1ace 100644 --- a/docs/src/content/docs/docs/Advanced/CLI.md +++ b/docs/src/content/docs/docs/Advanced/CLI.md @@ -146,10 +146,12 @@ said so in a notice. A Template set to **Do nothing** when the file already exis reports the same. If you are counting captures, writing an idempotency marker, or deciding whether to retry, key off `effect`, not `ok`. -`effect` is always present on a terminal frame, and `unknown` is stated rather than -omitted - a missing key reads as `false` in both `jq` and JavaScript, which would -turn "QuickAdd did not look" into "nothing happened". `verified:false` still means -only *"not confirmed - go look"*; it never means *"confirmed that nothing changed"*. +`effect` is present on every **success** payload (`ok:true`), and `unknown` is stated +rather than omitted - a missing key reads as `false` in both `jq` and JavaScript, which +would turn "QuickAdd did not look" into "nothing happened". A failed or cancelled run +carries `error` instead and no `effect`, because there is no outcome to describe. +`verified:false` still means only *"not confirmed - go look"*; it never means +*"confirmed that nothing changed"*. The `obsidian://quickadd` [x-callback](/docs/Advanced/TriggerQuickAddFromOutsideObsidian/) success callback carries the same `effect` value. @@ -176,8 +178,10 @@ background. Attach to the session and drive it: - `POST http://127.0.0.1:/abort?session=&token=` - end the run. Answers `{"ok":true,"interrupted":}`, where `n` is how many pending prompts it rejected; `409` if the run had already finished (benign - poll for the terminal event); `404` for an unknown session or token, or for any method other than `POST`. Prompts a Template or Capture run opens itself - the "file already exists" chooser, -the folder picker, the note-discovery picker, the heading picker - are forwarded like -any other. They arrive as `suggester` prompts, and because the engine controls the +the folder picker, the note-discovery picker, the heading picker, the capture-target +picker - are forwarded like any other. (The AI assistant's tool-confirmation dialog is +the one that is not: run such a choice at the desktop, or set tool confirmation to +"never".) They arrive as `suggester` prompts, and because the engine controls the list, a reply that is not one of the offered `value` tokens is refused rather than acted on (unless the prompt sets `allowCustomInput`, as the folder and discovery pickers do so you can create something new). diff --git a/src/engine/CaptureChoiceEngine.ts b/src/engine/CaptureChoiceEngine.ts index f6f614a7..614e1053 100644 --- a/src/engine/CaptureChoiceEngine.ts +++ b/src/engine/CaptureChoiceEngine.ts @@ -50,7 +50,7 @@ import { templaterParseTemplate, waitForTemplaterTriggerOnCreateToComplete, } from "../utilityObsidian"; -import { isCancellationError, reportError } from "../utils/errorUtils"; +import { reportError } from "../utils/errorUtils"; import { ChoiceOutcomeRecorder, failureReason, @@ -87,7 +87,6 @@ import { } from "./helpers/frontmatterPostProcessor"; import { ChoiceAbortError } from "../errors/ChoiceAbortError"; import { assertCreatableFilePath } from "./assertCreatableFilePath"; -import { UserCancelError } from "../errors/UserCancelError"; import { SingleTemplateEngine } from "./SingleTemplateEngine"; import { getCaptureAction, type CaptureAction } from "./captureAction"; import { @@ -529,7 +528,10 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { await getFileAndAddContentFn(filePath, content); let expectedCursorContent: string | null = null; let canPlaceCursorAtCapture = cursorPlacementSafe; - let wroteFrontmatter = false; + // Set by a post-commit step that writes the file AGAIN, after `newFileContent` + // was compared against `priorContent` — whole-file Templater, or front-matter + // post-processing. Either makes an otherwise-identical write a real change. + let rewroteAfterCompare = false; // The formatted capture payload is empty/whitespace-only: the formatter // returns the file unchanged and editor insertion replaces the selection // with "" — i.e. a no-op. Surface a distinct notice instead of a false @@ -586,6 +588,10 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { if (this.choice.templater?.afterCapture === "wholeFile") { await overwriteTemplaterOnce(this.app, file); canPlaceCursorAtCapture = false; + // Templater rewrote the whole file AFTER the bytes we compared, so a + // note that still contained `<% %>` has changed even when the capture + // payload itself was a no-op. + rewroteAfterCompare = true; } const frontmatterPostProcessed = await this.applyCapturePropertyVars(file); @@ -594,7 +600,7 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { } // Post-processing writes front matter of its own, so it counts as a // change even when the capture body itself was a no-op. - wroteFrontmatter = frontmatterPostProcessed; + if (frontmatterPostProcessed) rewroteAfterCompare = true; expectedCursorContent = canPlaceCursorAtCapture ? newFileContent : null; } @@ -614,7 +620,7 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { : "changed" : !fileAlreadyExists ? "created" - : newFileContent !== priorContent || wroteFrontmatter + : newFileContent !== priorContent || rewroteAfterCompare ? "changed" : "unchanged"; this.outcome.success(file, effect); @@ -1209,12 +1215,6 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { `Folder ${folderPathSlash} is empty.`, ); - // Non-interactive run (CLI without `ui`): a format-syntax "Capture to" target - // resolves to a folder/vault scope at runtime (the requirement collector - // cannot pre-collect it), so this picker would hang. Abort with a clear error. - // Placed after the empty-folder check so a genuinely empty scope surfaces its - // own accurate error rather than the prompt message. - this.assertInteractiveCaptureTarget(); // Quick-Switcher-style ordering: recent first, excluded sunk, alphabetical tail. const orderedFiles = orderFilesForPicker( @@ -1232,35 +1232,50 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { const existingLabels = new Set( displayItems.map((label) => label.toLowerCase()), ); - let targetFilePath: string; - try { - targetFilePath = await InputSuggester.Suggest( - this.app, - displayItems, - filePaths, - { - placeholder: allowCreate - ? "Choose a note or type to create one" - : undefined, - emptyStateText: allowCreate - ? "Type a note name to create it" - : undefined, - renderItem: (path, el) => - renderNotePathSuggestion(el, path, this.app), - searchItems, - allowCustomValue: allowCreate, - customValueLabel: (value) => `Create new note: ${value}`, - valueExists: (value) => - existingLabels.has(value.toLowerCase()) || - this.captureTargetExists(folderPathSlash, value), + const placeholder = allowCreate + ? "Choose a note or type to create one" + : undefined; + const targetFilePath = String( + await routePrompt(this.choiceExecutor, { + // A custom reply needs no extra confinement here: every reply is + // re-prefixed into `folderPathSlash` below, which is exactly what + // `confinePreselectedToScope` does for a folder scope. + remote: (provider) => + promptEngineChoice(provider, { + items: filePaths.map((path, index) => ({ + value: path, + title: displayItems[index] ?? path, + })), + placeholder, + allowCustomInput: allowCreate, + what: "the capture-target picker", + }), + // Non-interactive run (CLI without `ui`): a format-syntax "Capture to" + // target resolves to a folder/vault scope at runtime (the requirement + // collector cannot pre-collect it), so this picker would hang. Abort with + // a clear error. Placed after the empty-folder check so a genuinely empty + // scope surfaces its own accurate error rather than the prompt message. + headless: () => { + this.assertInteractiveCaptureTarget(); + throw new Error("unreachable"); }, - ); - } catch (error) { - if (isCancellationError(error)) { - throw new UserCancelError("Input cancelled by user"); - } - throw error; - } + app: () => + InputSuggester.Suggest(this.app, displayItems, filePaths, { + placeholder, + emptyStateText: allowCreate + ? "Type a note name to create it" + : undefined, + renderItem: (path, el) => + renderNotePathSuggestion(el, path, this.app), + searchItems, + allowCustomValue: allowCreate, + customValueLabel: (value) => `Create new note: ${value}`, + valueExists: (value) => + existingLabels.has(value.toLowerCase()) || + this.captureTargetExists(folderPathSlash, value), + }), + }), + ); invariant( !!targetFilePath && targetFilePath.length > 0, @@ -1357,10 +1372,6 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { invariant(allowCreate || files.length > 0, notFoundMessage); - // See selectFileInFolder: a format-syntax tag/property capture target resolves - // to a runtime file picker the requirement collector can't pre-collect. Placed - // after the no-match check so an empty result surfaces its own accurate error. - this.assertInteractiveCaptureTarget(); // Quick-Switcher-style ordering; show note names (not raw paths). const orderedFiles = orderFilesForPicker( @@ -1384,35 +1395,63 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { const existingLabels = new Set( displayItems.map((label) => label.toLowerCase()), ); - let targetFilePath: string; - try { - targetFilePath = await InputSuggester.Suggest( - this.app, - displayItems, - filePaths, - { - placeholder: allowCreate - ? "Choose a note or type to create one" - : undefined, - emptyStateText: allowCreate - ? "Type a note name to create it" - : undefined, - renderItem: (path, el) => - renderNotePathSuggestion(el, path, this.app), - searchItems, - allowCustomValue: allowCreate, - customValueLabel: (value) => `Create new note: ${value}`, - valueExists: (value) => - existingLabels.has(value.toLowerCase()) || - this.captureTargetAlreadyExists(value, vaultBasenames), + const nameIsTaken = (value: string) => + existingLabels.has(value.toLowerCase()) || + this.captureTargetAlreadyExists(value, vaultBasenames); + const placeholder = allowCreate + ? "Choose a note or type to create one" + : undefined; + + const targetFilePath = String( + await routePrompt(this.choiceExecutor, { + remote: async (provider) => { + const reply = await promptEngineChoice(provider, { + items: filePaths.map((path, index) => ({ + value: path, + title: displayItems[index] ?? path, + })), + placeholder, + allowCustomInput: allowCreate, + what: "the capture-target picker", + }); + // Unlike the folder scope, nothing downstream re-confines this reply - + // it goes straight to formatFilePath. In Obsidian the picker enforces + // the rule structurally: `valueExists` suppresses the "Create new + // note" row for a name that already exists, so a typed value can only + // ever create a NEW note, never redirect the capture into an existing + // one the tag/property scope does not match. Enforce the same rule on + // a routed reply, mirroring `confinePreselectedToScope`. + if (!filePaths.includes(reply) && nameIsTaken(reply)) { + throw new Error( + `"${reply}" already exists but is not one of the notes this capture targets. ` + + `Pick one of the offered notes, or type a name that does not exist yet.`, + ); + } + return reply; }, - ); - } catch (error) { - if (isCancellationError(error)) { - throw new UserCancelError("Input cancelled by user"); - } - throw error; - } + // See selectFileInFolder: a format-syntax tag/property capture target + // resolves to a runtime file picker the requirement collector can't + // pre-collect. Placed after the no-match check so an empty result + // surfaces its own accurate error. + headless: () => { + this.assertInteractiveCaptureTarget(); + throw new Error("unreachable"); + }, + app: () => + InputSuggester.Suggest(this.app, displayItems, filePaths, { + placeholder, + emptyStateText: allowCreate + ? "Type a note name to create it" + : undefined, + renderItem: (path, el) => + renderNotePathSuggestion(el, path, this.app), + searchItems, + allowCustomValue: allowCreate, + customValueLabel: (value) => `Create new note: ${value}`, + valueExists: nameIsTaken, + }), + }), + ); invariant( !!targetFilePath && targetFilePath.length > 0, diff --git a/src/engine/TemplateChoiceEngine.ts b/src/engine/TemplateChoiceEngine.ts index 082de809..a60e6302 100644 --- a/src/engine/TemplateChoiceEngine.ts +++ b/src/engine/TemplateChoiceEngine.ts @@ -193,10 +193,13 @@ export class TemplateChoiceEngine extends TemplateEngine { let createdFile: TFile | null; let shouldAutoOpen = false; let createdNew = false; - // What this run did to the vault (#1615). Derived from the file-exists - // resolution the engine actually performed, which is exact for the two - // answers an automation acts on: "createNew" always writes a new note, and - // "reuseExisting" — the shipped "Do nothing" mode — writes nothing at all. + // What this run did to its target note (#1615). Derived from the file-exists + // resolution the engine actually performed rather than from a byte compare, + // which is exact for the two answers an automation acts on: "createNew" + // always writes a new note, and "reuseExisting" — the shipped "Do nothing" + // mode — writes nothing at all. Only "modifyExisting" (append/overwrite) is + // inferred: it always writes, so `changed` can in principle over-report a + // write whose bytes happened to match, which is the harmless direction. let effect: ChoiceEffect = "created"; if (await this.app.vault.adapter.exists(targetFilePath)) { const modeId = await this.getSelectedFileExistsMode(); diff --git a/src/engine/TemplateEngine.ts b/src/engine/TemplateEngine.ts index d74e2b04..e5253749 100644 --- a/src/engine/TemplateEngine.ts +++ b/src/engine/TemplateEngine.ts @@ -294,17 +294,25 @@ export abstract class TemplateEngine extends QuickAddEngine { // re-prompt is indistinguishable from the first one - an unbounded loop of an // identical question. Bound it there and end with the text the Notice carries. let remoteAttempts = executor.promptProvider ? MAX_REMOTE_FOLDER_ATTEMPTS : 0; + // The reason the LAST answer was refused, so the abort says what the Notice + // would have. The loop rejects for two different reasons - a disallowed root and + // a name Obsidian cannot use - and blaming the roots for an invalid name would + // send the client looking in the wrong place. + let lastRejection: string | null = null; // Keep prompting until the user provides an allowed selection or cancels. for (;;) { if (executor.promptProvider && remoteAttempts-- <= 0) { - throw new ChoiceAbortError(folderNotAllowedMessage(context.allowedRoots)); + throw new ChoiceAbortError( + lastRejection ?? folderNotAllowedMessage(context.allowedRoots), + ); } const raw = await this.promptForFolder(context, executor); const selection = await this.resolveSelection(raw, context); if (selection.isEmpty) { if (!selection.isAllowed) { + lastRejection = folderNotAllowedMessage(context.allowedRoots); this.showFolderNotAllowedNotice(context.allowedRoots); continue; } @@ -312,6 +320,7 @@ export abstract class TemplateEngine extends QuickAddEngine { } if (!selection.isAllowed) { + lastRejection = folderNotAllowedMessage(context.allowedRoots); this.showFolderNotAllowedNotice(context.allowedRoots); continue; } @@ -320,6 +329,7 @@ export abstract class TemplateEngine extends QuickAddEngine { this.validateFolderPath(selection.resolved); } catch (error) { if (error instanceof InvalidFolderPathError) { + lastRejection = error.message; new Notice(error.message); continue; } diff --git a/src/engine/choiceOutcomeRecorder.test.ts b/src/engine/choiceOutcomeRecorder.test.ts index e6047863..604ab98a 100644 --- a/src/engine/choiceOutcomeRecorder.test.ts +++ b/src/engine/choiceOutcomeRecorder.test.ts @@ -42,21 +42,23 @@ describe("ChoiceOutcomeRecorder (#1603)", () => { }, ); - // The close-guard exists to stop an automation retrying and DUPLICATING a side - // effect. An "unchanged" run left no side effect, so there is nothing to protect - // and a real post-commit failure must still reach the caller instead of being - // swallowed behind a benign "nothing to capture" (#1615). - it("stays open after an unchanged run, so a later failure is still reported", () => { + // An "unchanged" run wrote nothing to its TARGET, but the run is not over at the + // commit point: it goes on to append a link to a DIFFERENT note and to copy one to + // the clipboard. So it closes the outcome like any other success — letting a later + // append-link failure overwrite it would put the caller back to retrying a run that + // already had side effects (#1615). + it("closes the outcome on an unchanged run too, because the run had steps left", () => { const target = executor(); const recorder = new ChoiceOutcomeRecorder(target); recorder.success({ path: "Inbox.md" } as never, "unchanged"); recorder.failure("Append link target file not found."); - expect(target.recordExecutionResult).toHaveBeenCalledTimes(2); - expect(target.recordExecutionResult).toHaveBeenLastCalledWith({ - status: "error", - reason: "Append link target file not found.", + expect(target.recordExecutionResult).toHaveBeenCalledTimes(1); + expect(target.recordExecutionResult).toHaveBeenCalledWith({ + status: "success", + file: { path: "Inbox.md" }, + effect: "unchanged", }); }); diff --git a/src/engine/choiceOutcomeRecorder.ts b/src/engine/choiceOutcomeRecorder.ts index 35a2dd8b..2e7546ec 100644 --- a/src/engine/choiceOutcomeRecorder.ts +++ b/src/engine/choiceOutcomeRecorder.ts @@ -27,9 +27,11 @@ import type { ChoiceEffect } from "../types/ChoiceOutcome"; * (the note path and the canvas path), and the canvas path keeps going into link and * open-file steps whose throws unwind into `run()`'s catch. * - * An `unchanged` run is the exception, and for the same reason: it left nothing a retry - * could duplicate, so closing the outcome there would only hide a real post-commit - * failure behind a benign "nothing to capture" (#1615). + * That holds for an `unchanged` run too, even though it wrote nothing to its TARGET. + * The run is not over at the commit point: it goes on to the append-link step, which + * writes to a DIFFERENT note, and to the copy-link-to-clipboard step. So "nothing was + * duplicable" is false, and letting a later failure overwrite the recorded success + * would put the caller right back to retrying a run that already had side effects. */ export class ChoiceOutcomeRecorder { /** @@ -52,7 +54,7 @@ export class ChoiceOutcomeRecorder { * open-an-existing-note discovery path) commit nothing at all. */ success(file: TFile | undefined, effect: ChoiceEffect): void { - if (effect !== "unchanged") this.closed = true; + this.closed = true; this.executor.recordExecutionResult?.({ status: "success", file, effect }); } diff --git a/src/interactive/engineChoice.ts b/src/interactive/engineChoice.ts index 781dcf31..dbf32776 100644 --- a/src/interactive/engineChoice.ts +++ b/src/interactive/engineChoice.ts @@ -59,15 +59,18 @@ export async function promptEngineChoice( spec.allowCustomInput ?? false, ); - if (values.includes(answer as T)) return answer as T; - const text = answer == null ? "" : String(answer); - // An empty reply is the shape `suggester` produces for a missing value, and no - // engine picker offers an empty row. Treat it as the dismissal it almost certainly - // is rather than acting on it — a client that means to cancel has - // `{"cancelled": true}`, which has already rejected before we get here. + + // Checked BEFORE membership, so an absent value can never be read as picking an + // empty row. A folder list CAN contain "" (a `{{VALUE:sub}}` entry that resolved + // to nothing is the vault root), and that is precisely the reply whose two + // readings differ most: "the client sent nothing" vs "create the note at the vault + // root". A client that really means the root row replies with its index token, + // which is unambiguous; one that means to cancel sends `{"cancelled": true}`. if (text === "") throw new UserCancelError("Input cancelled by user"); + if (values.includes(answer as T)) return answer as T; + if (spec.allowCustomInput) return text; throw new Error( diff --git a/src/interactive/interactivePromptServer.ts b/src/interactive/interactivePromptServer.ts index a5040e8b..773aed25 100644 --- a/src/interactive/interactivePromptServer.ts +++ b/src/interactive/interactivePromptServer.ts @@ -453,10 +453,11 @@ class InteractivePromptServer { * and delivers its real outcome, which is more truthful than a fabricated one. * * The honest limit, documented at the wire: this interrupts prompts that were routed - * to the CLIENT. Prompts QuickAdd still opens in Obsidian on a Template/Capture run - * (the file-exists suggester, the folder chooser, the capture-target picker) do not - * go through this server and are unaffected, which is why the reply says how many - * pending prompts it actually interrupted. + * to the CLIENT. A run that is mid-work between prompts keeps going, which is why the + * reply says how many pending prompts it actually interrupted. The Template/Capture + * engine prompts that used to be outside that set - the file-exists chooser, the + * folder chooser, the capture-target and heading pickers - now travel this bridge and + * are interruptible (#1614); the AI tool-confirmation modal still is not. * * @returns the number of pending prompts rejected, or null if there is no live * session to abort. diff --git a/src/interactive/routePrompt.test.ts b/src/interactive/routePrompt.test.ts index 2609385a..392711ba 100644 --- a/src/interactive/routePrompt.test.ts +++ b/src/interactive/routePrompt.test.ts @@ -153,6 +153,21 @@ describe("promptEngineChoice enforces what the in-app modal enforces structurall ).resolves.toBe("A brand new note"); }); + // A folder list CAN legitimately contain "" (a `{{VALUE:sub}}` entry that resolved to + // nothing is the vault root). Membership must NOT win there, or an absent value silently + // becomes "create the note at the vault root" - the outcome the strict channel exists + // to prevent. + it("treats an empty reply as a dismissal even when '' is an offered row", async () => { + const withRoot = [ + { value: "", title: "/" }, + { value: "Archive", title: "Archive" }, + ]; + + await expect( + promptEngineChoice(providerReturning(""), { items: withRoot, what }), + ).rejects.toBeInstanceOf(UserCancelError); + }); + it("still refuses an empty reply where custom input is allowed", async () => { await expect( promptEngineChoice(providerReturning(""), { diff --git a/src/types/ChoiceOutcome.ts b/src/types/ChoiceOutcome.ts index cd23700d..881b176a 100644 --- a/src/types/ChoiceOutcome.ts +++ b/src/types/ChoiceOutcome.ts @@ -1,7 +1,12 @@ import type { TFile } from "obsidian"; /** - * What a successful run actually did to the vault. + * What a successful run did to the file it was aiming at. + * + * Scoped to the choice's TARGET, not to the whole vault: a run can also append a link + * to another note or copy one to the clipboard, and those steps are not described here. + * "Did the capture land / was the note created" is the question an automation asks, and + * it is the one this answers. * * `success` alone answers "did the run finish?", which is not the question an * automation asks — it asks "did anything land?". Those came apart in two shipped @@ -12,9 +17,9 @@ import type { TFile } from "obsidian"; * caller counting captures or writing an idempotency marker recorded work that never * happened (#1615). * - * `created` means the file did not exist before this run. `changed` means its bytes - * differ. `unchanged` means the vault is byte-identical — nothing failed, and nothing - * a retry could duplicate happened either. + * `created` means the target did not exist before this run. `changed` means its bytes + * differ. `unchanged` means the target is byte-identical — nothing failed, and the + * capture or template simply had nothing to write. * * The claim is about PERSISTED BYTES, never about the payload: a Capture with an empty * payload can still legitimately `create` a note (create-if-not-found, possibly with a From 09bc890b732c49285fa003bec0142aef008161e1 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Tue, 28 Jul 2026 01:57:49 +0200 Subject: [PATCH 7/8] fix: keep a picked row distinguishable from no answer at all, and mark the docs unreleased Review caught a real hole in the previous commit's reasoning. Its comment claimed "a client that really means the root row replies with its index token, which is unambiguous" - but the provider decodes that token back to the raw value BEFORE this module sees it. So picking the vault-root row and answering nothing at all both arrived here as `""`, and the empty-is-a-cancel rule made a legitimate selection abort the run. The channel now hands the provider its own opaque row handles instead of the real values, and maps a handle back afterwards. That makes "the client picked row N" structurally distinct from "the client said nothing", whatever row N's value happens to be - which is what the folder chooser needs, since "" really is one of its rows when a `{{VALUE:sub}}` entry resolves to nothing. Also adds the `:::note[Available in the next release]` callouts AGENTS.md requires for documented-but-unshipped behaviour: docs deploy from master ahead of releases, so `effect` and the forwarded engine prompts both need one. Refs #1614 Refs #1615 --- docs/src/content/docs/docs/Advanced/CLI.md | 12 ++++- src/interactive/engineChoice.ts | 35 +++++++----- src/interactive/routePrompt.test.ts | 63 ++++++++++++++++------ 3 files changed, 80 insertions(+), 30 deletions(-) diff --git a/docs/src/content/docs/docs/Advanced/CLI.md b/docs/src/content/docs/docs/Advanced/CLI.md index 51ad1ace..d8c69c7a 100644 --- a/docs/src/content/docs/docs/Advanced/CLI.md +++ b/docs/src/content/docs/docs/Advanced/CLI.md @@ -156,6 +156,11 @@ carries `error` instead and no `effect`, because there is no outcome to describe The `obsidian://quickadd` [x-callback](/docs/Advanced/TriggerQuickAddFromOutsideObsidian/) success callback carries the same `effect` value. +:::note[Available in the next release] +`effect` is new. `capabilities` in the `quickadd:interactive` handshake contains +`outcome-effect` on a build that has it. +::: + ## Answer run-time prompts from outside: `quickadd:interactive` {#interactive-runs-quickaddinteractive} Some choices prompt at *run time* for inputs that can't be gathered up front - @@ -181,7 +186,12 @@ Prompts a Template or Capture run opens itself - the "file already exists" choos the folder picker, the note-discovery picker, the heading picker, the capture-target picker - are forwarded like any other. (The AI assistant's tool-confirmation dialog is the one that is not: run such a choice at the desktop, or set tool confirmation to -"never".) They arrive as `suggester` prompts, and because the engine controls the +"never".) + +:::note[Available in the next release] +Forwarding the run's own pickers is new. Before it, a Template or Capture run opened +them in Obsidian and `/abort` could not reach them. +::: They arrive as `suggester` prompts, and because the engine controls the list, a reply that is not one of the offered `value` tokens is refused rather than acted on (unless the prompt sets `allowCustomInput`, as the folder and discovery pickers do so you can create something new). diff --git a/src/interactive/engineChoice.ts b/src/interactive/engineChoice.ts index dbf32776..009cc571 100644 --- a/src/interactive/engineChoice.ts +++ b/src/interactive/engineChoice.ts @@ -25,6 +25,13 @@ import { UserCancelError } from "../errors/UserCancelError"; import type { PromptProvider } from "./promptProvider"; +/** + * Prefix for this module's own row handles. The NUL char makes a collision with a + * value a user could type effectively impossible, mirroring the provider's own + * index token. + */ +const HANDLE_PREFIX = "\u0000qa-eng:"; + export interface EngineChoiceItem { /** The real item. Never crosses the wire — the provider tokenises by index. */ value: T; @@ -49,27 +56,31 @@ export async function promptEngineChoice( what: string; }, ): Promise { - const values = spec.items.map((item) => item.value); + // Hand the provider OUR OWN opaque handles rather than the real values, so a + // picked row is distinguishable from a typed one no matter what the row's value + // is. This matters for exactly one case, and it is a real one: a folder list can + // contain "" (a `{{VALUE:sub}}` entry that resolved to nothing is the vault root). + // The provider's own index token is decoded back to the raw value before this + // function sees it, so without a handle of our own, PICKING the root row and + // sending NO ANSWER AT ALL arrive here as the same empty string — and those two + // have to mean opposite things. + const handles = spec.items.map((_, index) => `${HANDLE_PREFIX}${index}`); const answer = await provider.suggester( spec.items.map((item) => item.title), - // The provider tokenises by index and hands back the ORIGINAL entry, so object - // identity survives even though only titles are sent. - values as unknown as string[], + handles, spec.placeholder, spec.allowCustomInput ?? false, ); const text = answer == null ? "" : String(answer); - // Checked BEFORE membership, so an absent value can never be read as picking an - // empty row. A folder list CAN contain "" (a `{{VALUE:sub}}` entry that resolved - // to nothing is the vault root), and that is precisely the reply whose two - // readings differ most: "the client sent nothing" vs "create the note at the vault - // root". A client that really means the root row replies with its index token, - // which is unambiguous; one that means to cancel sends `{"cancelled": true}`. - if (text === "") throw new UserCancelError("Input cancelled by user"); + const picked = handles.indexOf(text); + if (picked !== -1) return spec.items[picked].value; - if (values.includes(answer as T)) return answer as T; + // Not a handle, so the client either typed something or answered nothing. An + // empty reply is what `suggester` produces for a missing value; a client that + // means to cancel sends `{"cancelled": true}`, which rejects before we get here. + if (text === "") throw new UserCancelError("Input cancelled by user"); if (spec.allowCustomInput) return text; diff --git a/src/interactive/routePrompt.test.ts b/src/interactive/routePrompt.test.ts index 392711ba..d4ab5898 100644 --- a/src/interactive/routePrompt.test.ts +++ b/src/interactive/routePrompt.test.ts @@ -116,10 +116,10 @@ describe("promptEngineChoice enforces what the in-app modal enforces structurall const providerReturning = (answer: unknown) => ({ suggester: vi.fn(async () => answer) }) as unknown as PromptProvider; - it("returns the offered value when the client picks one", async () => { + it("refuses a reply that echoes a raw value instead of the row's handle", async () => { await expect( promptEngineChoice(providerReturning("doNothing"), { items, what }), - ).resolves.toBe("doNothing"); + ).rejects.toThrow(/has to be one of the offered options/); }); // GenericSuggester can only ever resolve an element of its list. Routing the prompt @@ -154,18 +154,33 @@ describe("promptEngineChoice enforces what the in-app modal enforces structurall }); // A folder list CAN legitimately contain "" (a `{{VALUE:sub}}` entry that resolved to - // nothing is the vault root). Membership must NOT win there, or an absent value silently - // becomes "create the note at the vault root" - the outcome the strict channel exists - // to prevent. - it("treats an empty reply as a dismissal even when '' is an offered row", async () => { + // nothing is the vault root). Picking that row and answering nothing at all must not + // collapse into the same thing: one creates the note at the vault root, the other has + // to abort. The row handles are what keep them apart. + describe("when '' is one of the offered rows", () => { const withRoot = [ { value: "", title: "/" }, { value: "Archive", title: "Archive" }, ]; - await expect( - promptEngineChoice(providerReturning(""), { items: withRoot, what }), - ).rejects.toBeInstanceOf(UserCancelError); + it("returns the root value when the client PICKS that row", async () => { + // The client echoes the row's opaque handle, as it does for any row. + const provider = { + suggester: vi.fn( + async (_display: unknown, handles: string[]) => handles[0], + ), + } as unknown as PromptProvider; + + await expect( + promptEngineChoice(provider, { items: withRoot, what }), + ).resolves.toBe(""); + }); + + it("cancels when the client answers NOTHING", async () => { + await expect( + promptEngineChoice(providerReturning(""), { items: withRoot, what }), + ).rejects.toBeInstanceOf(UserCancelError); + }); }); it("still refuses an empty reply where custom input is allowed", async () => { @@ -178,16 +193,30 @@ describe("promptEngineChoice enforces what the in-app modal enforces structurall ).rejects.toBeInstanceOf(UserCancelError); }); - it("sends titles, not the underlying values, and asks for a closed list by default", async () => { + it("sends titles and opaque handles, never the underlying values, and closes the list by default", async () => { const provider = providerReturning("appendBottom"); - await promptEngineChoice(provider, { items, what, placeholder: "Pick one" }); - - expect(provider.suggester).toHaveBeenCalledWith( - ["Append to bottom", "Do nothing"], - ["appendBottom", "doNothing"], - "Pick one", - false, + await promptEngineChoice(provider, { items, what, placeholder: "Pick one" }).catch( + () => undefined, ); + + const [displays, handles, placeholder, allowCustom] = ( + provider.suggester as ReturnType + ).mock.calls[0]; + expect(displays).toEqual(["Append to bottom", "Do nothing"]); + expect(handles).toHaveLength(2); + expect(handles[0]).not.toBe("appendBottom"); + expect(placeholder).toBe("Pick one"); + expect(allowCustom).toBe(false); + }); + + it("round-trips a picked row back to its original value", async () => { + const provider = { + suggester: vi.fn(async (_d: unknown, handles: string[]) => handles[1]), + } as unknown as PromptProvider; + + await expect( + promptEngineChoice(provider, { items, what }), + ).resolves.toBe("doNothing"); }); }); From a3004c52f5acf8ac1a10af065d6fb0a4c4a60fbd Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Tue, 28 Jul 2026 02:13:40 +0200 Subject: [PATCH 8/8] test: make the capture effect assertion rest on a real comparison The stub omitted `priorContent`, so `newFileContent !== priorContent` was `"updated" !== undefined` and the `effect: "changed"` assertion could never have failed. Stating it means the test exercises the compare it claims to. Refs #1615 --- src/engine/CaptureChoiceEngine.selection.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/engine/CaptureChoiceEngine.selection.test.ts b/src/engine/CaptureChoiceEngine.selection.test.ts index d34b16b1..9e10c7cf 100644 --- a/src/engine/CaptureChoiceEngine.selection.test.ts +++ b/src/engine/CaptureChoiceEngine.selection.test.ts @@ -1734,6 +1734,10 @@ describe("CaptureChoiceEngine capture target resolution", () => { file: linkedFile, newFileContent: "updated", captureContent: "capture", + // Stated, so the `effect: "changed"` assertion below rests on a real + // comparison. Left undefined, `newFileContent !== priorContent` is + // `"updated" !== undefined`, which can never yield "unchanged" (#1615). + priorContent: "existing", })); await engine.run();