Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 122 additions & 2 deletions src/engine/CaptureChoiceEngine.selection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1256,6 +1256,122 @@ describe("CaptureChoiceEngine capture target resolution", () => {
});
});

it("refuses an impossible target before prompting or creating (#1591)", async () => {
// The refusal used to arrive from vault.create, after the user had
// answered the whole prompt chain.
const app = createApp();
const engine = new CaptureChoiceEngine(
app,
{
settings: {
useSelectionAsCaptureValue: false,
showCaptureNotification: true,
},
} as any,
createChoice({
captureTo: "Bad: Title.md",
createFileIfItDoesntExist: {
enabled: true,
createWithTemplate: false,
template: "",
},
format: { enabled: true, format: "{{VALUE}}" },
}),
createExecutor(),
);

const createFileWithInput = vi.fn();
(engine as any).createFileWithInput = createFileWithInput;

await engine.run();

expect(createFileWithInput).not.toHaveBeenCalled();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// And the capture's own {{VALUE}} prompt never opened: the whole point of
// #1591 is that the user does not answer a prompt chain for a note that
// cannot be created.
expect(promptHydratedValues).toHaveLength(0);
});

it("refuses before the heading picker, not after it (#1591)", async () => {
// Placement, not just presence: the "Choose heading when capturing" picker
// runs between the existence check and the create dispatch, so a guard
// sited at the sink would still have asked the user to pick a heading for
// a note that cannot be created. This case fails if the guard moves down.
const suggestSpy = vi.fn(async () => "## Today");
(InputSuggester as any).Suggest = suggestSpy;

const engine = new CaptureChoiceEngine(
createApp(),
{
settings: {
useSelectionAsCaptureValue: false,
showCaptureNotification: true,
},
} as any,
createChoice({
captureTo: "Bad: Title.md",
createFileIfItDoesntExist: {
enabled: true,
createWithTemplate: false,
template: "",
},
insertAfter: {
enabled: true,
after: "",
insertAtEnd: false,
considerSubsections: false,
createIfNotFound: false,
createIfNotFoundLocation: "",
promptHeading: true,
},
}),
createExecutor(),
);
const createFileWithInput = vi.fn();
(engine as any).createFileWithInput = createFileWithInput;

await engine.run();

expect(suggestSpy).not.toHaveBeenCalled();
expect(createFileWithInput).not.toHaveBeenCalled();
});

it("still appends to an EXISTING file whose name carries a colon (#1591)", async () => {
// ":" is legal on macOS/Linux at the filesystem level, so a note made
// outside Obsidian can have one - and capturing to it works today,
// because nothing ever asks Obsidian to accept the name.
const app = createApp();
const engine = new CaptureChoiceEngine(
app,
{
settings: {
useSelectionAsCaptureValue: false,
showCaptureNotification: true,
},
} as any,
createChoice({
captureTo: "Bad: Title.md",
createFileIfItDoesntExist: {
enabled: true,
createWithTemplate: false,
template: "",
},
}),
createExecutor(),
);
(engine as any).fileExists = vi.fn(async () => true);
const onFileExists = vi.fn(async () => ({
file: { path: "Bad: Title.md", basename: "Bad: Title" },
newFileContent: "",
captureContent: "x",
}));
(engine as any).onFileExists = onFileExists;

await engine.run();

expect(onFileExists).toHaveBeenCalled();
});

it("keeps submitted VALUE prompt draft after failed target creation and clears it after success", async () => {
promptResponses.push("Capture body to preserve");
const store = InputPromptDraftStore.getInstance();
Expand All @@ -1274,7 +1390,11 @@ describe("CaptureChoiceEngine capture target resolution", () => {
},
} as any,
createChoice({
captureTo: "Bad:Title.md",
// A LEGAL target: an impossible one is now refused before the
// prompt is ever opened (#1591), which is a different behaviour
// with its own test below. What this case is about is a create
// that fails for some other reason after the answer is in.
captureTo: "Notes/Target.md",
createFileIfItDoesntExist: {
enabled: true,
createWithTemplate: false,
Expand All @@ -1289,7 +1409,7 @@ describe("CaptureChoiceEngine capture target resolution", () => {
);

(engine as any).createFileWithInput = vi.fn(async () => {
throw new Error("File name cannot contain ':'");
throw new Error("Disk is on fire");
});

const clipboardWriteText = vi.fn(async () => {});
Expand Down
18 changes: 18 additions & 0 deletions src/engine/CaptureChoiceEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ import {
shouldPostProcessFrontMatter,
} 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";
Expand Down Expand Up @@ -434,6 +435,18 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine {
let getFileAndAddContentFn: GetFileAndAddContentFn;
const fileAlreadyExists = await this.fileExists(filePath);

// Refuse an impossible target here rather than at vault.create (#1591).
// Gated character-for-character on the dispatch condition below, so a
// missing target with creation OFF still gets its own message - and an
// EXISTING file with a colon in its name (legal on macOS/Linux, so a note
// made outside Obsidian can have one) keeps appending as it does today.
// Placed above the heading picker rather than inside
// onCreateFileIfItDoesntExist so the user is not asked to choose a
// heading for a note that cannot be created.
if (!fileAlreadyExists && this.choice?.createFileIfItDoesntExist?.enabled) {
assertCreatableFilePath(filePath);
}

// "Choose heading when capturing" (After line…): prompt for a heading from the resolved
// target note and feed the picked line to the formatter as an insert-after
// override. Runs after the target file is known and before any formatting/write.
Expand Down Expand Up @@ -1436,6 +1449,11 @@ export class CaptureChoiceEngine extends QuickAddChoiceEngine {
captureContent: string,
linkOptions?: AppendLinkOptions,
): Promise<CaptureWriteResult> {
// Re-asserted at the sink, so the invariant is local to the one function
// that creates the file, not only to its caller's branch. It is an
// `includes` scan over a short string and costs nothing (#1591).
assertCreatableFilePath(filePath);

// Extract filename without extension from the full path.
const fileBasename = basenameWithoutMdOrCanvas(filePath);
this.formatter.setTitle(fileBasename);
Expand Down
111 changes: 111 additions & 0 deletions src/engine/TemplateEngine-1591-refuse-name.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import { describe, expect, it, vi } from "vitest";
import { ChoiceAbortError } from "../errors/ChoiceAbortError";

/**
* Issue #1591. `createFileWithTemplate` reads the template, formats the ENTIRE
* body - real `{{VALUE}}` prompts, macros, and inline `js quickadd` fences with
* their side effects - and only then hands the path to `createFileWithInput`,
* which creates the target FOLDER before `vault.create` refuses the name.
*
* Measured on Obsidian 1.13.0 (isolated e2e vault) before the fix: a Template
* choice named `Bad: {{VALUE:title}}` reported "no file was created" AND left
* `Repro1591Folder` behind AND had run the template's inline script once.
*
* The guard is the first statement of `createFileWithTemplate`, so this asserts
* on the observable consequence: nothing downstream of it is reached.
*/

vi.mock("obsidian", async () => {
const actual = await import("../../tests/obsidian-stub");
return actual;
});
vi.mock("../formatters/completeFormatter", () => ({
CompleteFormatter: class {
setTitle() {}
setTargetFolderPath() {}
setPromptRunContext() {}
setTemplateInclusionState() {}
},
}));
vi.mock("../utilityObsidian", () => ({
getTemplateFile: vi.fn(),
getTemplater: vi.fn(() => null),
overwriteTemplaterOnce: vi.fn(),
templaterParseTemplate: vi.fn(),
}));
vi.mock("../logger/logManager", () => ({
log: { logWarning: vi.fn(), logError: vi.fn(), logMessage: vi.fn() },
}));

const { TemplateEngine } = await import("./TemplateEngine");

class ProbeEngine extends (TemplateEngine as never as new (
...args: never[]
) => Record<string, unknown>) {
public readTemplate = vi.fn();
public created = vi.fn();

constructor() {
super(
{ vault: {}, workspace: {} } as never,
{ settings: {} } as never,
);
}

run() {
return Promise.resolve();
}

// Everything the guard must run BEFORE.
protected getTemplateContent(path: string) {
this.readTemplate(path);
return Promise.resolve("body");
}
protected createFileWithInput(path: string) {
this.created(path);
return Promise.resolve({ path });
}
}

function makeEngine() {
return new ProbeEngine() as unknown as ProbeEngine & {
createFileWithTemplate: (
filePath: string,
templatePath: string,
) => Promise<unknown>;
};
}

describe("createFileWithTemplate refuses an impossible name first (#1591)", () => {
it("aborts without reading the template or creating anything", async () => {
const engine = makeEngine();

await expect(
engine.createFileWithTemplate("Notes/Bad: My Note.md", "T.md"),
).rejects.toBeInstanceOf(ChoiceAbortError);

expect(engine.readTemplate).not.toHaveBeenCalled();
expect(engine.created).not.toHaveBeenCalled();
});

it("aborts for a colon in a FOLDER segment, before the folder is created", async () => {
const engine = makeEngine();

await expect(
engine.createFileWithTemplate("Meetings: 2026/Note.md", "T.md"),
).rejects.toBeInstanceOf(ChoiceAbortError);

expect(engine.readTemplate).not.toHaveBeenCalled();
});

it("leaves a legal path alone", async () => {
// Getting as far as reading the template is what proves the guard let it
// through; the rest of the create path needs the real formatter and is
// covered by TemplateChoiceEngine's own suites.
const engine = makeEngine();

await engine.createFileWithTemplate("Notes/My Note.md", "T.md");

expect(engine.readTemplate).toHaveBeenCalledWith("T.md");
});
});
17 changes: 17 additions & 0 deletions src/engine/TemplateEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import { ChoiceAbortError } from "../errors/ChoiceAbortError";
import { isCancellationError } from "../utils/errorUtils";
import type { IChoiceExecutor } from "../IChoiceExecutor";
import { log } from "../logger/logManager";
import { assertCreatableFilePath } from "./assertCreatableFilePath";

type FolderChoiceOptions = {
allowCreate?: boolean;
Expand Down Expand Up @@ -598,7 +599,23 @@ export abstract class TemplateEngine extends QuickAddEngine {
filePath: string,
resolvedTemplatePath: string
) {
// Clear the previous run's swallowed cause FIRST, so the reset is
// unconditional even when the guard below aborts (#1617).
this.lastTemplateFileFailure = null;

// Then, before the template is read and long before its body is formatted
// (#1591). Both production callers - TemplateChoiceEngine's `else` branch of
// `vault.adapter.exists`, and its freshly incremented collision name - are
// paths where nothing is at `filePath` yet, so a name Obsidian refuses can
// only end in a failed `vault.create`. Letting it get that far means the
// user answers the whole prompt chain and their inline scripts run first,
// and QuickAddEngine.createFileWithInput leaves the target folder behind.
//
// It THROWS rather than recording into lastTemplateFileFailure: that field
// is for helpers that report-and-return-null, while this aborts the choice
// with a typed ChoiceAbortError carrying its own actionable message (#1606).
assertCreatableFilePath(filePath);

try {
const templateContent: string = await this.getTemplateContent(
resolvedTemplatePath
Expand Down
Loading