diff --git a/docs/src/content/docs/docs/Advanced/CLI.md b/docs/src/content/docs/docs/Advanced/CLI.md index 6c099551..d8c69c7a 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._ @@ -125,6 +125,42 @@ 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 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. + +:::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 - @@ -136,7 +172,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 @@ -146,11 +182,26 @@ 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, 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".) + +:::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). + 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 @@ -171,12 +222,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] 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/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/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..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(); @@ -1742,6 +1746,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..614e1053 100644 --- a/src/engine/CaptureChoiceEngine.ts +++ b/src/engine/CaptureChoiceEngine.ts @@ -50,11 +50,14 @@ import { templaterParseTemplate, waitForTemplaterTriggerOnCreateToComplete, } from "../utilityObsidian"; -import { isCancellationError, reportError } from "../utils/errorUtils"; +import { reportError } from "../utils/errorUtils"; import { ChoiceOutcomeRecorder, 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, @@ -84,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 { @@ -114,6 +116,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 +521,17 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { file, newFileContent, captureContent, + priorContent, cursorEndOffset, cursorPlacementSafe = true, } = await getFileAndAddContentFn(filePath, content); let expectedCursorContent: string | null = null; let canPlaceCursorAtCapture = cursorPlacementSafe; + // 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 @@ -574,19 +588,42 @@ 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); 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. + if (frontmatterPostProcessed) rewroteAfterCompare = true; 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 || rewroteAfterCompare + ? "changed" + : "unchanged"; + this.outcome.success(file, effect); // Show success notification if (this.plugin.settings.showCaptureNotification) { @@ -745,16 +782,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) { @@ -849,15 +892,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); @@ -868,28 +902,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, @@ -1168,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( @@ -1191,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, @@ -1316,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( @@ -1343,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, @@ -1439,6 +1519,7 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { file, newFileContent, captureContent: formatted, + priorContent: secondReadFileContent, cursorEndOffset: cursorEndOffset ?? undefined, cursorPlacementSafe, }; @@ -1556,6 +1637,7 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine { file, newFileContent, captureContent: formattedCaptureContent, + priorContent: updatedFileContent, cursorEndOffset: cursorEndOffset ?? undefined, cursorPlacementSafe: true, }; 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.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..a60e6302 100644 --- a/src/engine/TemplateChoiceEngine.ts +++ b/src/engine/TemplateChoiceEngine.ts @@ -17,6 +17,9 @@ import { shouldRunTemplateNoteDiscovery, } 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, @@ -28,7 +31,7 @@ import { openExistingFileTab, openFile, } from "../utilityObsidian"; -import { isCancellationError, reportError } from "../utils/errorUtils"; +import { reportError } from "../utils/errorUtils"; import { ChoiceOutcomeRecorder, failureReason, @@ -48,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"; @@ -113,10 +115,14 @@ export class TemplateChoiceEngine extends TemplateEngine { const discovery = await promptForTemplateNoteDiscovery( this.app, this.choice, + this.choiceExecutor, ); 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 +193,23 @@ export class TemplateChoiceEngine extends TemplateEngine { let createdFile: TFile | null; let shouldAutoOpen = false; let createdNew = false; + // 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(); const mode = getFileExistsMode(modeId); + effect = + mode.resolutionKind === "reuseExisting" + ? "unchanged" + : mode.resolutionKind === "createNew" + ? "created" + : "changed"; const existingFile = mode.requiresExistingFile ? this.findExistingFile(targetFilePath) : null; @@ -250,7 +270,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 @@ -417,32 +437,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( @@ -678,9 +706,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 && @@ -702,7 +730,7 @@ export class TemplateChoiceEngine extends TemplateEngine { allowCreate: true, allowedRoots: folders, topItems, - interactive, + executor, }); } @@ -713,7 +741,7 @@ export class TemplateChoiceEngine extends TemplateEngine { return await this.getOrCreateFolder(allFoldersInVault, { allowCreate: true, topItems, - interactive, + executor, }); } @@ -730,7 +758,7 @@ export class TemplateChoiceEngine extends TemplateEngine { return await this.getOrCreateFolder([activeFile.parent.path], { allowCreate: true, topItems, - interactive, + executor, }); } @@ -738,7 +766,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..e5253749 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,14 +287,32 @@ 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; + // 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 (;;) { - const raw = await this.promptForFolder(context); + if (executor.promptProvider && remoteAttempts-- <= 0) { + 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; } @@ -263,6 +320,7 @@ export abstract class TemplateEngine extends QuickAddEngine { } if (!selection.isAllowed) { + lastRejection = folderNotAllowedMessage(context.allowedRoots); this.showFolderNotAllowedNotice(context.allowedRoots); continue; } @@ -271,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; } @@ -381,12 +440,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/choiceOutcomeRecorder.test.ts b/src/engine/choiceOutcomeRecorder.test.ts index 367d7300..604ab98a 100644 --- a/src/engine/choiceOutcomeRecorder.test.ts +++ b/src/engine/choiceOutcomeRecorder.test.ts @@ -22,17 +22,43 @@ 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, + }); + }, + ); + + // 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: "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" }, + file: { path: "Inbox.md" }, + effect: "unchanged", }); }); diff --git a/src/engine/choiceOutcomeRecorder.ts b/src/engine/choiceOutcomeRecorder.ts index bfc3d27f..2e7546ec 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,42 @@ 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. + * + * 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 { - 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 { + this.closed = true; + this.executor.recordExecutionResult?.({ status: "success", file, effect }); } /** @@ -44,7 +63,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/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)) { diff --git a/src/interactive/engineChoice.ts b/src/interactive/engineChoice.ts new file mode 100644 index 00000000..009cc571 --- /dev/null +++ b/src/interactive/engineChoice.ts @@ -0,0 +1,91 @@ +/** + * 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"; + +/** + * 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; + /** 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 { + // 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), + handles, + spec.placeholder, + spec.allowCustomInput ?? false, + ); + + const text = answer == null ? "" : String(answer); + + const picked = handles.indexOf(text); + if (picked !== -1) return spec.items[picked].value; + + // 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; + + 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..773aed25 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); @@ -438,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. @@ -478,7 +494,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 +535,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..d4ab5898 --- /dev/null +++ b/src/interactive/routePrompt.test.ts @@ -0,0 +1,222 @@ +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("refuses a reply that echoes a raw value instead of the row's handle", async () => { + await expect( + promptEngineChoice(providerReturning("doNothing"), { items, what }), + ).rejects.toThrow(/has to be one of the offered options/); + }); + + // 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"); + }); + + // A folder list CAN legitimately contain "" (a `{{VALUE:sub}}` entry that resolved to + // 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" }, + ]; + + 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 () => { + await expect( + promptEngineChoice(providerReturning(""), { + items, + what, + allowCustomInput: true, + }), + ).rejects.toBeInstanceOf(UserCancelError); + }); + + 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" }).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"); + }); +}); 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; + } +} 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); diff --git a/src/types/ChoiceOutcome.ts b/src/types/ChoiceOutcome.ts index b444c2a1..881b176a 100644 --- a/src/types/ChoiceOutcome.ts +++ b/src/types/ChoiceOutcome.ts @@ -1,12 +1,42 @@ import type { TFile } from "obsidian"; +/** + * 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 + * 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 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 + * 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 +46,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 };