diff --git a/src/fix/fix-executor.ts b/src/fix/fix-executor.ts index 0b2b161..9258944 100644 --- a/src/fix/fix-executor.ts +++ b/src/fix/fix-executor.ts @@ -1,66 +1,96 @@ import { App, TFile } from "obsidian"; import type { FixAction } from "../scanner/Issue"; +import type { MutationFence } from "./metadata-write-fence"; import { markdownLinks, wikiLinkRanges } from "../utils/markdown-source"; -export async function executeFixAction(app: App, action: FixAction): Promise { +export async function executeFixAction( + app: App, + action: FixAction, + fence?: MutationFence, +): Promise { switch (action.kind) { case "trash-file": - return trashFiles(app, action.targetPaths); + return trashFiles(app, action.targetPaths, fence); case "remove-link-text": { const source = action.targetPaths[0]; if (action.original !== undefined) { - return replaceLinkText(app, source, action.original, action.replacement ?? ""); + return replaceLinkText(app, source, action.original, action.replacement ?? "", undefined, fence); } - return removeLinkText(app, source, action.linkText!); + return removeLinkText(app, source, action.linkText!, fence); } default: return 0; } } -async function trashFiles(app: App, paths: string[]): Promise { +async function trashFiles(app: App, paths: string[], fence?: MutationFence): Promise { let count = 0; for (const path of paths) { + if (fence && !fence.ready) break; const file = app.vault.getAbstractFileByPath(path); - if (file) { - await app.fileManager.trashFile(file); + if (file instanceof TFile) { + const ready = fence + ? await fence.mutate(file, null, () => app.fileManager.trashFile(file)) + : (await app.fileManager.trashFile(file), true); count++; + if (!ready) break; } } return count; } -async function removeLinkText(app: App, sourcePath: string, linkText: string): Promise { - return replaceLinkText(app, sourcePath, undefined, "", linkText); +async function removeLinkText( + app: App, + sourcePath: string, + linkText: string, + fence?: MutationFence, +): Promise { + return replaceLinkText(app, sourcePath, undefined, "", linkText, fence); } -/** Parse once per action, then splice only complete, matching source ranges. */ +/** + * Parse once per action, then splice only complete, matching source ranges. + * The atomic read/transform/write happens inside vault.process; when a fence + * is provided, expected content is armed synchronously before the write + * returns so cache events can be correlated with this exact mutation. + */ async function replaceLinkText( app: App, sourcePath: string, original: string | undefined, replacement: string, legacyLinkText?: string, + fence?: MutationFence, ): Promise { const file = app.vault.getAbstractFileByPath(sourcePath); - if (!(file instanceof TFile)) return 0; - const content = await app.vault.read(file); - const wiki = original === undefined || /^!?\[\[/.test(original); - const ranges = (wiki ? wikiLinkRanges(content) : markdownLinks(content)).filter(({ start, end }) => { - const source = content.slice(start, end); - return original !== undefined - ? source === original - : source === `[[${legacyLinkText}]]` || source === `![[${legacyLinkText}]]`; - }).sort((left, right) => left.start - right.start); - let cursor = 0; - let updated = ""; - for (const { start, end } of ranges) { - if (start < cursor) continue; - updated += content.slice(cursor, start) + replacement; - cursor = end; - } - updated += content.slice(cursor); - if (updated === content) return 0; - await app.vault.modify(file, updated); - return 1; + if (!(file instanceof TFile) || (fence && !fence.ready)) return 0; + let affectedCount = 0; + const write = async (expectContent: (updated: string) => void) => { + await app.vault.process(file, content => { + const wiki = original === undefined || /^!?\[\[/.test(original); + const ranges = (wiki ? wikiLinkRanges(content) : markdownLinks(content)) + .filter(({ start, end }) => { + const source = content.slice(start, end); + return original !== undefined + ? source === original + : source === `[[${legacyLinkText}]]` || source === `![[${legacyLinkText}]]`; + }).sort((left, right) => left.start - right.start); + let cursor = 0; + let updated = ""; + for (const { start, end } of ranges) { + if (start < cursor) continue; + updated += content.slice(cursor, start) + replacement; + cursor = end; + } + updated += content.slice(cursor); + if (updated !== content) { + affectedCount = 1; + expectContent(updated); + } + return updated; + }); + }; + if (fence) await fence.mutate(file, undefined, write); + else await write(() => {}); + return affectedCount; } diff --git a/src/fix/fix-runner.ts b/src/fix/fix-runner.ts index 216a52d..df35ef1 100644 --- a/src/fix/fix-runner.ts +++ b/src/fix/fix-runner.ts @@ -6,13 +6,23 @@ import { isBlockedFromExecution, type FixDecision, } from "./fix-decisions"; +import { METADATA_NOT_READY } from "./metadata-write-fence"; export type FixRunnerDependencies = { /** Read live settings once; the batch clones and freezes the value for every scan. */ settings: () => InspectorSettings; /** Receives a clone of the frozen settings on every call (preflights + final verification). */ scan: (settings: InspectorSettings) => Promise; - execute: (action: FixAction) => Promise; + /** Executors may return a number (legacy) or a result carrying verification readiness. */ + execute: (action: FixAction) => Promise; + /** Optional batch-wide guard; false stops all further scans and mutations. */ + canScan?: () => boolean; +}; + +export type FixExecutionResult = { + affectedCount: number; + verificationReady: boolean; + verificationMessage?: string; }; export type FixBatchResult = { @@ -33,7 +43,13 @@ export async function runFixBatch( dependencies: FixRunnerDependencies, ): Promise { const frozenSettings = structuredClone(dependencies.settings()); - const scanOnce = () => dependencies.scan(structuredClone(frozenSettings)); + // Set when a write succeeded but its cache synchronization could not be + // confirmed: later preflights would read stale metadata and must not run. + let verificationProblem: string | undefined; + const scanOnce = () => { + if (verificationProblem || dependencies.canScan?.() === false) return Promise.resolve(null); + return dependencies.scan(structuredClone(frozenSettings)); + }; const decisionsByFingerprint = new Map( decisions.map((decision) => [decision.fingerprint, decision]), @@ -43,6 +59,10 @@ export async function runFixBatch( let scannedDuringBatch = false; for (const [index, issue] of issues.entries()) { + if (verificationProblem || dependencies.canScan?.() === false) { + outcomes[index] = skipped(issue, METADATA_NOT_READY); + continue; + } if (isBlockedFromExecution(issue)) { outcomes[index] = skipped( issue, @@ -86,12 +106,19 @@ export async function runFixBatch( } try { + const raw = await dependencies.execute(freshAction); + const execution = typeof raw === "number" + ? { affectedCount: raw, verificationReady: true } + : raw; pending.push({ index, fingerprint: issue.fingerprint, affectedPaths: [...freshAction.targetPaths], - affectedCount: await dependencies.execute(freshAction), + affectedCount: execution.affectedCount, }); + if (!execution.verificationReady) { + verificationProblem = execution.verificationMessage ?? METADATA_NOT_READY; + } } catch (error) { outcomes[index] = { fingerprint: issue.fingerprint, @@ -114,7 +141,11 @@ export async function runFixBatch( fingerprint: action.fingerprint, outcome: "failed", phase: "verification", - message: "The final verification scan did not complete.", + message: verificationProblem ?? ( + dependencies.canScan?.() === false + ? METADATA_NOT_READY + : "The final verification scan did not complete." + ), affectedPaths: action.affectedPaths, }; } diff --git a/src/fix/metadata-write-fence.ts b/src/fix/metadata-write-fence.ts new file mode 100644 index 0000000..0755a42 --- /dev/null +++ b/src/fix/metadata-write-fence.ts @@ -0,0 +1,97 @@ +import type { App, EventRef, TFile } from "obsidian"; + +export type MutationFence = { + readonly ready: boolean; + mutate(file: TFile, content: string | null | undefined, + write: (expectContent: (updated: string) => void) => Promise): Promise; +}; + +export const METADATA_NOT_READY = + "Changes may already be saved, but metadata synchronization did not complete. " + + "Remaining fixes in this batch were skipped. Run a fresh scan and review saved contents before retrying."; + +export class MetadataWriteFence implements MutationFence { + private poisoned = false; + private disposed = false; + private readonly cancel = new Set<() => void>(); + + constructor(private readonly app: App, private readonly timeoutMs = 10000) {} + + get ready(): boolean { return !this.poisoned && !this.disposed; } + + async mutate(file: TFile, content: string | null | undefined, + write: (expectContent: (updated: string) => void) => Promise): Promise { + if (!this.ready) return false; + const path = file.path; + let relevant = false; + let resolved = false; + let writeDone = false; + let done = false; + let settle!: (value: boolean) => void; + const waiting = new Promise(resolve => { settle = resolve; }); + const metadataRefs: EventRef[] = []; + const vaultRefs: EventRef[] = []; + let timer: number | undefined; + const finish = (success: boolean) => { + if (done) return; + done = true; + if (!success) this.poisoned = true; + for (const ref of metadataRefs) this.app.metadataCache.offref(ref); + for (const ref of vaultRefs) this.app.vault.offref(ref); + if (timer !== undefined) window.clearTimeout(timer); + this.cancel.delete(cancel); + settle(success); + }; + const cancel = () => finish(false); + const check = () => { + if (!writeDone || !relevant || !resolved) return; + if (content === null && this.app.vault.getAbstractFileByPath(path)) return; + finish(true); + }; + this.cancel.add(cancel); + metadataRefs.push(this.app.metadataCache.on("changed", (changed, data) => { + if (content === null || changed.path !== path) return; + // A later change with other content invalidates earlier proof too. + relevant = data === content; + resolved = false; + })); + metadataRefs.push(this.app.metadataCache.on("deleted", deleted => { + if (deleted.path !== path) return; + relevant = content === null; + resolved = false; + })); + if (content === null && file.extension !== "md") { + vaultRefs.push(this.app.vault.on("delete", deleted => { + if (deleted.path !== path) return; + relevant = true; + resolved = false; + })); + } + metadataRefs.push(this.app.metadataCache.on("resolved", () => { + if (!relevant) return; + resolved = true; + check(); + })); + timer = window.setTimeout(cancel, this.timeoutMs); + try { + await write(updated => { + content = updated; + relevant = false; + resolved = false; + }); + writeDone = true; + // process callbacks that return unchanged content do not schedule a cache event. + if (content === undefined) finish(true); + else check(); + return await waiting; + } catch (error) { + finish(false); + throw error; + } + } + + dispose(): void { + this.disposed = true; + for (const cancel of [...this.cancel]) cancel(); + } +} diff --git a/src/main.ts b/src/main.ts index 2ce3a40..93c2620 100644 --- a/src/main.ts +++ b/src/main.ts @@ -15,6 +15,7 @@ import { MAX_SAFE_VAULT_REPORT_BYTES, } from "./report/report-export"; import { executeFixAction } from "./fix/fix-executor"; +import { METADATA_NOT_READY, MetadataWriteFence } from "./fix/metadata-write-fence"; import { showConfirmModal } from "./fix/confirm-modal"; import { runFixBatch } from "./fix/fix-runner"; import type { DispositionOutcome } from "./fix/action-outcomes"; @@ -92,7 +93,12 @@ export default class VaultInspectorPlugin extends Plugin { this.addRibbonIcon("shield-check", "Run scan", () => this.runScan()); } - onunload() {} + private activeMetadataFences = new Set(); + + onunload() { + for (const fence of this.activeMetadataFences) fence.dispose(); + this.activeMetadataFences.clear(); + } async loadSettings() { const parsed = parsePluginData(await this.loadData()); @@ -240,11 +246,25 @@ export default class VaultInspectorPlugin extends Plugin { await this.enqueueOperation(async () => { const fixSettings = structuredClone(this.settings); const scanProfile = await createScanProfile(fixSettings); - const batch = await runFixBatch(issues, decisions, { - settings: () => fixSettings, - scan: (batchSettings) => this.scan(view, batchSettings), - execute: (action) => executeFixAction(this.app, action), - }); + const fence = new MetadataWriteFence(this.app); + this.activeMetadataFences.add(fence); + const batch = await (async () => { + try { + return await runFixBatch(issues, decisions, { + settings: () => fixSettings, + scan: (batchSettings) => this.scan(view, batchSettings), + canScan: () => fence.ready, + execute: async (action) => ({ + affectedCount: await executeFixAction(this.app, action, fence), + verificationReady: fence.ready, + verificationMessage: fence.ready ? undefined : METADATA_NOT_READY, + }), + }); + } finally { + fence.dispose(); + this.activeMetadataFences.delete(fence); + } + })(); let acceptanceFailed = false; let acceptanceError: unknown; if (batch.verificationResult) { diff --git a/src/scanner/scanners/broken-links.ts b/src/scanner/scanners/broken-links.ts index b5ea0fe..1a65e4c 100644 --- a/src/scanner/scanners/broken-links.ts +++ b/src/scanner/scanners/broken-links.ts @@ -184,15 +184,40 @@ function resolveLinkIssues( } if (headingPart) { - const targetCache = ctx.metadataCache.getFileCache( - ctx.markdownFiles.find((file) => file.path === resolvedPath)!, - ); + const targetFile = ctx.markdownFiles.find((file) => file.path === resolvedPath); + const targetCache = targetFile ? ctx.metadataCache.getFileCache(targetFile) : null; const isBlock = headingPart.startsWith("^"); + if (!targetCache) { + // A null cache means Obsidian has not indexed the target yet; a + // miss against no evidence at all must not authorize a fix. An + // empty cache object is still real evidence of a missing target. + const issue = makeIssue( + sourcePath, + candidate, + resolvedPath, + "info", + `Target metadata not available yet for "#${headingPart}" in ${resolvedPath}`, + candidate.isEmbed + ? "embed" + : candidate.isMarkdown + ? "markdown-link" + : "heading", + isBlock ? "block" : "heading", + { + title: "Link could not be verified", + why: "The target exists, but its heading and block metadata could not be read.", + nextStep: "Wait for indexing to finish, then run the scan again.", + }, + ); + issue.evidence.reason = "target-metadata-unavailable"; + issues.push(issue); + return issues; + } const found = isBlock - ? Object.keys(targetCache?.blocks ?? {}).some( + ? Object.keys(targetCache.blocks ?? {}).some( (id) => id.toLowerCase() === headingPart.slice(1).toLowerCase(), ) - : (targetCache?.headings ?? []).some( + : (targetCache.headings ?? []).some( (heading) => slugifyHeading(heading.heading) === slugifyHeading(headingPart), ); if (!found && isBlock) { diff --git a/src/snapshot/scan-snapshot.ts b/src/snapshot/scan-snapshot.ts index 4389d30..cc54e7b 100644 --- a/src/snapshot/scan-snapshot.ts +++ b/src/snapshot/scan-snapshot.ts @@ -10,6 +10,11 @@ import { export const SNAPSHOT_SCHEMA_VERSION = 1; /** + * 4 — Link-reference findings whose target metadata is unavailable, and + * block-id misses, are unverified and cannot authorize link fixes; heading + * anchors match Obsidian's replace-with-space normalization. Older baselines + * must not fabricate resolved/new findings against the changed semantics. + * * 3 — audited link, reference, and YAML detection corrections change which * findings exist. Older baselines must not fabricate resolved/new findings. * @@ -18,7 +23,7 @@ export const SNAPSHOT_SCHEMA_VERSION = 1; * error). Fingerprints for the reclassified findings changed identity, so * pre-2 snapshots cannot be compared without false resolved/new claims. */ -export const COMPARISON_VERSION = 3; +export const COMPARISON_VERSION = 4; export type SnapshotIssue = { fingerprint: string; diff --git a/src/tests/broken-links.test.ts b/src/tests/broken-links.test.ts index a2f54a1..4d6eead 100644 --- a/src/tests/broken-links.test.ts +++ b/src/tests/broken-links.test.ts @@ -930,6 +930,82 @@ describe("heading anchors", () => { }); }); +describe("unavailable target metadata", () => { + it.each(["Existing", "^block-id"])("does not authorize fixes without target cache: %s", async (fragment) => { + const ctx = makeScanContext({ + scanner: "broken-links", + files: [{ path: "Source.md" }, { path: "Target.md" }], + metadataByPath: { + "Source.md": { links: [{ + link: `Target#${fragment}`, + original: `[[Target#${fragment}]]`, + position: {} as any, + }] }, + "Target.md": null, + }, + }); + expect(ctx.metadataCache.getFileCache(ctx.markdownFiles[1])).toBeNull(); + const issues = await brokenLinksScanner.scan(ctx); + expect(issues).toHaveLength(1); + expect(issues[0]).toMatchObject({ + classification: "unverified", + severity: "info", + evidence: { reason: "target-metadata-unavailable" }, + }); + expect(issues[0].fixAction).toBeUndefined(); + }); + + it("recovers once the target cache becomes available", async () => { + const base = { + scanner: "broken-links" as const, + files: [{ path: "Source.md" }, { path: "Target.md" }], + }; + const links = [{ + link: "Target#Existing", + original: "[[Target#Existing]]", + position: {} as any, + }]; + const before = makeScanContext({ + ...base, + metadataByPath: { + "Source.md": { links }, + "Target.md": null, + }, + }); + expect(await brokenLinksScanner.scan(before)).toHaveLength(1); + const after = makeScanContext({ + ...base, + metadataByPath: { + "Source.md": { links }, + "Target.md": { headings: [{ heading: "Existing", level: 1, position: {} as any }] }, + }, + }); + expect(await brokenLinksScanner.scan(after)).toEqual([]); + }); + + it("treats an empty-but-present cache as available evidence", async () => { + const ctx = makeScanContext({ + scanner: "broken-links", + files: [{ path: "Source.md" }, { path: "Target.md" }], + metadataByPath: { + "Source.md": { links: [{ + link: "Target#Missing", + original: "[[Target#Missing]]", + position: {} as any, + }] }, + "Target.md": {}, + }, + }); + const issues = await brokenLinksScanner.scan(ctx); + expect(issues).toHaveLength(1); + expect(issues[0]).toMatchObject({ + classification: "confirmed", + severity: "warning", + message: 'Heading "#Missing" not found in Target.md', + }); + }); +}); + describe("block references", () => { it.each([ ["[[Target#^Known-id]]", "Target#^Known-id", false], diff --git a/src/tests/cli.test.ts b/src/tests/cli.test.ts index 69a132a..0512e28 100644 --- a/src/tests/cli.test.ts +++ b/src/tests/cli.test.ts @@ -5,6 +5,7 @@ import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { afterEach, describe, expect, it, vi } from "vitest"; import { runCli } from "../../cli/cli"; +import { COMPARISON_VERSION } from "../snapshot/scan-snapshot"; import { createLocalApp } from "../../cli/local-vault"; import { EXTERNAL_LINK_TIMEOUT_MS } from "../scanner/scanners/external-links"; @@ -230,7 +231,7 @@ describe("runCli", () => { persistingIssues: 0, resolvedIssues: 0, scanProfile: expect.any(String), - comparisonVersion: 3, + comparisonVersion: COMPARISON_VERSION, fingerprints: expect.any(Array), }); // The identity set is the complete unfiltered set, not just the @@ -589,7 +590,7 @@ describe("runCli", () => { persistingIssues: 0, resolvedIssues: 0, scanProfile: expect.any(String), - comparisonVersion: 3, + comparisonVersion: COMPARISON_VERSION, fingerprints: expect.any(Array), }); }); @@ -1376,7 +1377,7 @@ describe("runCli", () => { persistingIssues: 1, resolvedIssues: 0, scanProfile: expect.any(String), - comparisonVersion: 3, + comparisonVersion: COMPARISON_VERSION, fingerprints: expect.any(Array), }); // The identity set is the complete unfiltered set. @@ -1423,7 +1424,7 @@ describe("runCli", () => { persistingIssues: 1, resolvedIssues: 1, scanProfile: expect.any(String), - comparisonVersion: 3, + comparisonVersion: COMPARISON_VERSION, fingerprints: expect.any(Array), }); // The identity set is the complete unfiltered set: sorted and unique. @@ -1483,7 +1484,7 @@ describe("runCli", () => { persistingIssues: 1, resolvedIssues: 1, scanProfile: expect.any(String), - comparisonVersion: 3, + comparisonVersion: COMPARISON_VERSION, fingerprints: expect.any(Array), }); expect(payload.issues.find( @@ -1596,7 +1597,7 @@ describe("runCli", () => { persistingIssues: 0, resolvedIssues: 0, scanProfile: expect.any(String), - comparisonVersion: 3, + comparisonVersion: COMPARISON_VERSION, fingerprints: expect.any(Array), }); // No lifecycle annotations are fabricated from an incompatible baseline. @@ -1643,7 +1644,7 @@ describe("runCli", () => { persistingIssues: 0, resolvedIssues: 0, scanProfile: expect.any(String), - comparisonVersion: 3, + comparisonVersion: COMPARISON_VERSION, fingerprints: expect.any(Array), }); expect(payload.issues.every( diff --git a/src/tests/fix-executor.test.ts b/src/tests/fix-executor.test.ts index 132d856..718e240 100644 --- a/src/tests/fix-executor.test.ts +++ b/src/tests/fix-executor.test.ts @@ -49,15 +49,22 @@ async function makeAliasedHeadingFixAction(): Promise { function makeApp(content: string) { const file = Object.assign(new TFile(), { path: "Source.md" }); - const modify = vi.fn(async () => {}); + let disk = content; + const process = vi.fn(async (_file: TFile, transform: (text: string) => string) => { + disk = transform(disk); + return disk; + }); + const read = vi.fn(async () => disk); + const modify = vi.fn(async (_file: TFile, text: string) => { disk = text; }); const app = { vault: { getAbstractFileByPath: vi.fn(() => file), - read: vi.fn(async () => content), + read, modify, + process, }, }; - return { app, file, modify }; + return { app, file, process, modify, getContent: () => disk }; } describe("executeFixAction", () => { @@ -69,7 +76,7 @@ describe("executeFixAction", () => { "[[Target|plain]]", "![[Target#Missing heading|missing]]", ].join("\n"); - const { app, file, modify } = makeApp(content); + const { app, file, getContent } = makeApp(content); const fixed = await executeFixAction(app as any, action); @@ -77,8 +84,7 @@ describe("executeFixAction", () => { expect(action.linkText).toBe("Target#Missing heading|missing"); expect(action.original).toBe("[[Target#Missing heading|missing]]"); expect(action.replacement).toBe("missing"); - expect(modify).toHaveBeenCalledWith( - file, + expect(getContent()).toBe( [ "missing", "[[Target#Other heading|other]]", @@ -102,13 +108,12 @@ describe("executeFixAction", () => { "Prefix [Readable Markdown](missing-target.md) suffix.", "![Readable Markdown](missing-target.md)", ].join("\n"); - const { app, file, modify } = makeApp(content); + const { app, file, getContent } = makeApp(content); const fixed = await executeFixAction(app as any, action); expect(fixed).toBe(1); - expect(modify).toHaveBeenCalledWith( - file, + expect(getContent()).toBe( [ "Prefix Readable Markdown suffix.", "![Readable Markdown](missing-target.md)", @@ -126,12 +131,12 @@ describe("executeFixAction", () => { replacement: "", }; const content = "Before ![[missing-embed.png]] after"; - const { app, file, modify } = makeApp(content); + const { app, file, getContent } = makeApp(content); const fixed = await executeFixAction(app as any, action); expect(fixed).toBe(1); - expect(modify).toHaveBeenCalledWith(file, "Before after"); + expect(getContent()).toBe("Before after"); }); it("still supports the legacy linkText wiki path", async () => { @@ -143,12 +148,12 @@ describe("executeFixAction", () => { linkText: "Legacy|Alias", }; const content = "Keep [[Legacy|Alias]] here"; - const { app, file, modify } = makeApp(content); + const { app, file, getContent } = makeApp(content); const fixed = await executeFixAction(app as any, action); expect(fixed).toBe(1); - expect(modify).toHaveBeenCalledWith(file, "Keep here"); + expect(getContent()).toBe("Keep here"); }); it("returns 0 when the original syntax is no longer present", async () => { @@ -160,11 +165,38 @@ describe("executeFixAction", () => { original: "[[Gone]]", replacement: "Gone", }; - const { app, modify } = makeApp("Nothing to see"); + const { app, getContent } = makeApp("Nothing to see"); const fixed = await executeFixAction(app as any, action); expect(fixed).toBe(0); + expect(getContent()).toBe("Nothing to see"); + }); + + it("preserves an edit committed before the atomic transformation", async () => { + const file = Object.assign(new TFile(), { path: "Source.md" }); + let disk = "[label](missing)\nOriginal paragraph"; + const appended = "\nConcurrent user edit"; + const read = vi.fn(async () => { + const stale = disk; + disk += appended; + return stale; + }); + const modify = vi.fn(async (_file: TFile, text: string) => { disk = text; }); + const process = vi.fn(async (_file: TFile, transform: (text: string) => string) => { + disk += appended; + disk = transform(disk); + return disk; + }); + const app = { vault: { getAbstractFileByPath: () => file, read, modify, process } }; + const count = await executeFixAction(app as any, { + kind: "remove-link-text", label: "Remove link", description: "", + targetPaths: [file.path], original: "[label](missing)", replacement: "label", + }); + expect(count).toBe(1); + expect(disk).toBe("label\nOriginal paragraph\nConcurrent user edit"); + expect(process).toHaveBeenCalledTimes(1); + expect(read).not.toHaveBeenCalled(); expect(modify).not.toHaveBeenCalled(); }); @@ -179,13 +211,12 @@ describe("executeFixAction", () => { "```", "", ].join("\n"); - const { app, file, modify } = makeApp(content); + const { app, file, getContent } = makeApp(content); const fixed = await executeFixAction(app as any, action); expect(fixed).toBe(1); - expect(modify).toHaveBeenCalledWith( - file, + expect(getContent()).toBe( [ "Before missing after", "`[[Target#Missing heading|missing]]`", @@ -210,37 +241,81 @@ describe("parsed source safety", () => { ].join("\n\n"); const prefix = `\uFEFF---\r\nref: '${original}'\r\n---\r\n`; const content = prefix + original + "\n\n" + protectedText + "\n\n" + original; - const { app, file, modify } = makeApp(content); + const { app, file, getContent } = makeApp(content); expect(await executeFixAction(app as any, { kind: "remove-link-text", label: "Remove", description: "", targetPaths: ["Source.md"], original, replacement: "Shown", })).toBe(1); - expect(modify).toHaveBeenCalledWith(file, prefix + "Shown\n\n" + protectedText + "\n\nShown"); + expect(getContent()).toBe(prefix + "Shown\n\n" + protectedText + "\n\nShown"); }); it("restricts legacy wiki removal to parsed ranges", async () => { const content = "[[Missing]]\n\n [[Missing]]\n\n\\[[Missing]]"; - const { app, file, modify } = makeApp(content); + const { app, file, getContent } = makeApp(content); expect(await executeFixAction(app as any, { kind: "remove-link-text", label: "Remove", description: "", targetPaths: ["Source.md"], linkText: "Missing", })).toBe(1); - expect(modify).toHaveBeenCalledWith(file, "\n\n [[Missing]]\n\n\\[[Missing]]"); + expect(getContent()).toBe("\n\n [[Missing]]\n\n\\[[Missing]]"); }); }); it.each(["[Missing](missing.md)", "[[Missing]]"])("replaces links after even backslashes: %s", async (original) => { const content = "\\\\" + original + "\r\n"; - const { app, file, modify } = makeApp(content); + const { app, file, getContent } = makeApp(content); expect(await executeFixAction(app as any, { kind: "remove-link-text", label: "Remove", description: "", targetPaths: ["Source.md"], original, replacement: "Shown", })).toBe(1); - expect(modify).toHaveBeenCalledWith(file, "\\\\Shown\r\n"); + expect(getContent()).toBe("\\\\Shown\r\n"); }); it.each(["[[Missing|**bold**]]", "plain text", " [[Missing]]"])("fails closed for unsupported or non-link source: %s", async (content) => { - const { app, modify } = makeApp(content); + const { app, getContent } = makeApp(content); expect(await executeFixAction(app as any, { kind: "remove-link-text", label: "Remove", description: "", targetPaths: ["Source.md"], original: content.trim(), replacement: "Shown", })).toBe(0); - expect(modify).not.toHaveBeenCalled(); + expect(getContent()).toBe(content); +}); + +it("propagates a failed write instead of returning a false success", async () => { + const { app, process } = makeApp("[[Missing]]"); + process.mockRejectedValueOnce(new Error("write failed")); + await expect(executeFixAction(app as any, { + kind: "remove-link-text", label: "Remove", description: "", targetPaths: ["Source.md"], + original: "[[Missing]]", replacement: "Shown", + })).rejects.toThrow("write failed"); +}); + +it("returns 0 without restoring old text when the link vanished from latest content", async () => { + const { app, getContent } = makeApp("[[Kept]] and [[Missing]]"); + const process = (app as any).vault.process; + process.mockImplementationOnce(async (_file: unknown, transform: (text: string) => string) => + transform("[[Kept]]")); + expect(await executeFixAction(app as any, { + kind: "remove-link-text", label: "Remove", description: "", targetPaths: ["Source.md"], + original: "[[Missing]]", replacement: "Shown", + })).toBe(0); + expect(getContent()).toBe("[[Kept]] and [[Missing]]"); +}); + +it("arms expected metadata inside the atomic process callback and preserves change count on timeout", async () => { + const file = Object.assign(new TFile(), { path: "Source.md" }); + const expectContent = vi.fn(); + const process = vi.fn(async (_file: TFile, transform: (content: string) => string) => { + const updated = transform("[[Missing]]"); + expect(expectContent).toHaveBeenCalledWith(updated); + expect(updated).toBe("Shown"); + return updated; + }); + const app = { vault: { getAbstractFileByPath: () => file, process } }; + const mutate = vi.fn(async (_file: unknown, _content: unknown, + write: (expect: (content: string) => void) => Promise) => { + await write(expectContent); + return false; + }); + expect(await executeFixAction(app as any, { + kind: "remove-link-text", label: "Remove", description: "", targetPaths: ["Source.md"], + original: "[[Missing]]", replacement: "Shown", + }, { ready: true, mutate })).toBe(1); + expect(mutate).toHaveBeenCalledWith(file, undefined, expect.any(Function)); + expect(process).toHaveBeenCalledWith(file, expect.any(Function)); }); diff --git a/src/tests/fix-runner.test.ts b/src/tests/fix-runner.test.ts index 032b86b..405a085 100644 --- a/src/tests/fix-runner.test.ts +++ b/src/tests/fix-runner.test.ts @@ -124,6 +124,33 @@ describe("runFixBatch", () => { expect(execute).not.toHaveBeenCalled(); }); + it("skips execution when preflight re-evaluates the finding as unverified without a fix", async () => { + const requested = issue("unverified"); + const reEvaluated = issue("unverified"); + // The same fingerprint resurfaces with unavailable target metadata: + // unverified classification and no authorizable fix action. + const { fixAction: _withdrawn, ...withoutFix } = reEvaluated; + void _withdrawn; + const stale = { ...withoutFix, classification: "unverified" as const }; + const execute = vi.fn(); + const scan = vi.fn() + .mockResolvedValueOnce(result([stale])) + .mockResolvedValueOnce(result([])); + + const batch = await runFixBatch( + [requested], + [{ fingerprint: requested.fingerprint }], + { settings: () => DEFAULT_SETTINGS, scan, execute }, + ); + + expect(batch.outcomes[0]).toMatchObject({ + fingerprint: "unverified", + outcome: "skipped", + phase: "preflight", + }); + expect(execute).not.toHaveBeenCalled(); + }); + it("reports a missing confirmed decision in its original outcome slot", async () => { const missing = issue("missing"); const confirmed = issue("confirmed"); @@ -392,19 +419,54 @@ describe("runFixBatch", () => { }); -it("verifies a parsed link fix without changing identical code examples", async () => { - const original = "[Missing](missing.md)"; - let content = `${original}\n\n ${original}\n\n\\${original}`; - const file = Object.assign(new TFile(), { path: "Source.md" }); - const modify = vi.fn(async (_file: TFile, updated: string) => { content = updated; }); - const app = { vault: { getAbstractFileByPath: () => file, read: async () => content, modify } }; - const requested = issue("link", action("Source.md", { kind: "remove-link-text", original, replacement: "Missing" })); - const scan = vi.fn().mockResolvedValueOnce(result([requested])).mockResolvedValueOnce(result([])); - const batch = await runFixBatch([requested], [{ fingerprint: "link" }], { - settings: () => DEFAULT_SETTINGS, scan, execute: (fix) => executeFixAction(app as any, fix), + it("verifies a parsed link fix without changing identical code examples", async () => { + const original = "[Missing](missing.md)"; + let content = `${original}\n\n ${original}\n\n\\${original}`; + const file = Object.assign(new TFile(), { path: "Source.md" }); + const modify = vi.fn(async (_file: TFile, updated: string) => { content = updated; }); + const process = vi.fn(async (_file: TFile, transform: (text: string) => string) => { + content = transform(content); + return content; + }); + const app = { vault: { getAbstractFileByPath: () => file, read: async () => content, modify, process } }; + const requested = issue("link", action("Source.md", { kind: "remove-link-text", original, replacement: "Missing" })); + const scan = vi.fn().mockResolvedValueOnce(result([requested])).mockResolvedValueOnce(result([])); + const batch = await runFixBatch([requested], [{ fingerprint: "link" }], { + settings: () => DEFAULT_SETTINGS, scan, execute: (fix) => executeFixAction(app as any, fix), + }); + expect(content).toBe(`Missing\n\n ${original}\n\n\\${original}`); + expect(batch.outcomes[0].outcome).toBe("fixed"); + expect(scan).toHaveBeenCalledTimes(2); + expect(process).toHaveBeenCalledTimes(1); + }); + +describe("runFixBatch metadata readiness", () => { + it("reports successful writes with cache timeouts in verification phase and stops subsequent preflights", async () => { + const first = issue("first"); + const second = issue("second"); + const scan = vi.fn().mockResolvedValue(result([first, second])); + const execute = vi.fn().mockResolvedValue({ + affectedCount: 1, verificationReady: false, verificationMessage: "Metadata timed out", + }); + const batch = await runFixBatch([first, second], [ + { fingerprint: first.fingerprint }, { fingerprint: second.fingerprint }, + ], { settings: () => DEFAULT_SETTINGS, scan, execute }); + expect(scan).toHaveBeenCalledOnce(); + expect(execute).toHaveBeenCalledOnce(); + expect(batch.verificationResult).toBeNull(); + expect(batch.outcomes[0]).toMatchObject({ outcome: "failed", phase: "verification", message: "Metadata timed out" }); + expect(batch.outcomes[1]).toMatchObject({ outcome: "skipped", phase: "preflight" }); + }); + + it("does not run any preflight when this batch fence is already unavailable", async () => { + const requested = issue("first"); + const scan = vi.fn(); + const execute = vi.fn(); + const batch = await runFixBatch([requested], [{ fingerprint: requested.fingerprint }], { + settings: () => DEFAULT_SETTINGS, scan, execute, canScan: () => false, + }); + expect(scan).not.toHaveBeenCalled(); + expect(execute).not.toHaveBeenCalled(); + expect(batch.outcomes[0]).toMatchObject({ outcome: "skipped", phase: "preflight" }); }); - expect(content).toBe(`Missing\n\n ${original}\n\n\\${original}`); - expect(batch.outcomes[0].outcome).toBe("fixed"); - expect(scan).toHaveBeenCalledTimes(2); - expect(modify).toHaveBeenCalledTimes(1); }); diff --git a/src/tests/helpers/scan-context.ts b/src/tests/helpers/scan-context.ts index c2e5a52..6bff7e4 100644 --- a/src/tests/helpers/scan-context.ts +++ b/src/tests/helpers/scan-context.ts @@ -57,7 +57,12 @@ export function makeScanContext(options: TestContextOptions = {}): ScanContext { return { app: {} as any, metadataCache: { - getFileCache: (file: TFile) => metadataByPath[file.path] ?? {}, + // An explicitly-null cache (metadata unavailable) must stay null; only + // omitted files get the empty default. + getFileCache: (file: TFile) => + Object.prototype.hasOwnProperty.call(metadataByPath, file.path) + ? metadataByPath[file.path] + : {}, resolvedLinks: options.resolvedLinks ?? {}, unresolvedLinks: options.unresolvedLinks ?? {}, } as any, diff --git a/src/tests/main.test.ts b/src/tests/main.test.ts index 27c5aeb..3f6037d 100644 --- a/src/tests/main.test.ts +++ b/src/tests/main.test.ts @@ -1322,6 +1322,7 @@ describe("VaultInspectorPlugin", () => { expect.objectContaining({ targetPaths: ["a.md", "b.md"], }), + expect.any(MetadataWriteFence), ); expect(view.setOperationOutcomes).toHaveBeenCalledWith([ expect.objectContaining({ fingerprint: "duplicates", outcome: "fixed" }), @@ -1869,3 +1870,4 @@ describe("migrateExcalidrawFrontmatterKey", () => { }); }); }); +import { MetadataWriteFence } from "../fix/metadata-write-fence"; diff --git a/src/tests/metadata-write-fence.test.ts b/src/tests/metadata-write-fence.test.ts new file mode 100644 index 0000000..da3c49f --- /dev/null +++ b/src/tests/metadata-write-fence.test.ts @@ -0,0 +1,143 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { App, TFile } from "obsidian"; +import { MetadataWriteFence } from "../fix/metadata-write-fence"; + +function fixture() { + vi.stubGlobal("window", globalThis); + type Callback = (...args: unknown[]) => void; + const listeners = new Map(); + const events = { + on(name: string, callback: Callback) { + const ref = {}; + listeners.set(ref, { name, callback }); + return ref; + }, + offref(ref: object) { listeners.delete(ref); }, + }; + const emit = (name: string, ...args: unknown[]) => { + for (const entry of [...listeners.values()]) { + if (entry.name === name) entry.callback(...args); + } + }; + const file = { path: "Source.md", extension: "md" } as TFile; + let exists = true; + const app = { + metadataCache: events, + vault: { ...events, getAbstractFileByPath: () => exists ? file : null }, + } as unknown as App; + const fence = new MetadataWriteFence(app, 100); + return { file, fence, emit, listeners, remove: () => { exists = false; } }; +} + +afterEach(() => { vi.useRealTimers(); vi.unstubAllGlobals(); }); + +describe("metadata write fence", () => { + it("rejects early resolved and wrong content, then accepts the exact changed content plus resolved", async () => { + const { file, fence, emit, listeners } = fixture(); + let finished = false; + const waiting = fence.mutate(file, "new", async () => {}).then(value => { + finished = true; + return value; + }); + emit("resolved"); + emit("changed", file, "old", {}); + emit("resolved"); + await Promise.resolve(); + expect(finished).toBe(false); + emit("changed", file, "new", {}); + await Promise.resolve(); + expect(finished).toBe(false); + emit("resolved"); + expect(await waiting).toBe(true); + expect(listeners.size).toBe(0); + }); + + it("registers before write and accepts events fired inside modify", async () => { + const { file, fence, emit, listeners } = fixture(); + expect(await fence.mutate(file, "new", async () => { + expect(listeners.size).toBeGreaterThan(0); + emit("changed", file, "new", {}); + emit("resolved"); + })).toBe(true); + expect(listeners.size).toBe(0); + }); + + it("times out without throwing a successful write and removes listeners", async () => { + vi.useFakeTimers(); + const { file, fence, listeners } = fixture(); + const write = vi.fn(async () => {}); + const waiting = fence.mutate(file, "new", write); + await vi.advanceTimersByTimeAsync(100); + expect(await waiting).toBe(false); + expect(write).toHaveBeenCalledOnce(); + expect(fence.ready).toBe(false); + expect(listeners.size).toBe(0); + const secondWrite = vi.fn(async () => {}); + expect(await fence.mutate(file, "next", secondWrite)).toBe(false); + expect(secondWrite).not.toHaveBeenCalled(); + }); + + it("unload releases the waiter and removes all listeners", async () => { + const { file, fence, listeners } = fixture(); + const waiting = fence.mutate(file, "new", async () => {}); + fence.dispose(); + expect(await waiting).toBe(false); + expect(listeners.size).toBe(0); + }); + + it("preserves real write errors and still poisons the cache session", async () => { + const { file, fence, listeners } = fixture(); + const error = new Error("write failed"); + await expect(fence.mutate(file, "new", async () => { throw error; })).rejects.toBe(error); + expect(fence.ready).toBe(false); + expect(listeners.size).toBe(0); + }); + + it("arms expected content in process before synchronous events", async () => { + const { file, fence, emit } = fixture(); + expect(await fence.mutate(file, undefined, async expectContent => { + expectContent("new"); + emit("changed", file, "new", {}); + emit("resolved"); + })).toBe(true); + }); + + it("does not await an event for an unchanged process result", async () => { + const { file, fence, listeners } = fixture(); + expect(await fence.mutate(file, undefined, async () => {})).toBe(true); + expect(listeners.size).toBe(0); + }); + + it("ignores other files and invalidates proof after conflicting source content", async () => { + vi.useFakeTimers(); + const { file, fence, emit, listeners } = fixture(); + const waiting = fence.mutate(file, "new", async () => {}); + emit("changed", { path: "Other.md" }, "new", {}); + emit("resolved"); + emit("changed", file, "new", {}); + emit("changed", file, "different", {}); + emit("resolved"); + await vi.advanceTimersByTimeAsync(100); + expect(await waiting).toBe(false); + expect(listeners.size).toBe(0); + }); + + it("requires relevant deletion followed by resolution and actual path absence", async () => { + const { file, fence, emit, remove, listeners } = fixture(); + let finished = false; + const waiting = fence.mutate(file, null, async () => {}).then(value => { + finished = true; + return value; + }); + emit("resolved"); + emit("deleted", { path: "Other.md" }, null); + emit("resolved"); + await Promise.resolve(); + expect(finished).toBe(false); + remove(); + emit("deleted", file, null); + emit("resolved"); + expect(await waiting).toBe(true); + expect(listeners.size).toBe(0); + }); +}); diff --git a/src/tests/scan-history.test.ts b/src/tests/scan-history.test.ts index 75b49cc..d0a7f37 100644 --- a/src/tests/scan-history.test.ts +++ b/src/tests/scan-history.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import type { Issue, ScanResult } from "../scanner/Issue"; import type { LifecycleComparison } from "../scanner/result-diff"; -import { createScanSnapshot } from "../snapshot/scan-snapshot"; +import { COMPARISON_VERSION, createScanSnapshot } from "../snapshot/scan-snapshot"; import { appendScanHistoryEntry, createScanHistoryEntry, @@ -116,7 +116,7 @@ describe("createScanHistoryEntry", () => { createdAt: 1_725_000_000_000, toolVersion: "0.7.0", scanProfile: "profile-abc", - comparisonVersion: 3, + comparisonVersion: COMPARISON_VERSION, trigger: "manual", filesScanned: 3, scannersRun: ["broken-links", "empty-notes"], diff --git a/src/tests/scan-snapshot.test.ts b/src/tests/scan-snapshot.test.ts index a024ba3..f2b386a 100644 --- a/src/tests/scan-snapshot.test.ts +++ b/src/tests/scan-snapshot.test.ts @@ -63,10 +63,10 @@ describe("scan snapshots", () => { ); expect(SNAPSHOT_SCHEMA_VERSION).toBe(1); - expect(COMPARISON_VERSION).toBe(3); + expect(COMPARISON_VERSION).toBe(4); expect(snapshot).toEqual({ schemaVersion: 1, - comparisonVersion: 3, + comparisonVersion: 4, toolVersion: "0.5.0", createdAt: 1_725_000_000_000, scanProfile: "profile-abc",