From 1291d0baa621323c10cc3bba8a498c4301d21e03 Mon Sep 17 00:00:00 2001 From: ooiuuii Date: Wed, 7 Oct 2026 01:28:09 -0700 Subject: [PATCH 1/4] fix(image-paste): preserve attachment identity across draft restoration --- extensions/image-paste/index.ts | 64 ++++-- tests/extensions/image-paste/index.test.ts | 4 +- .../image-paste/native-editor.test.ts | 188 ++++++++++++++++++ 3 files changed, 235 insertions(+), 21 deletions(-) create mode 100644 tests/extensions/image-paste/native-editor.test.ts diff --git a/extensions/image-paste/index.ts b/extensions/image-paste/index.ts index 214da4224..6d33500e9 100644 --- a/extensions/image-paste/index.ts +++ b/extensions/image-paste/index.ts @@ -16,6 +16,7 @@ import { removeEditorLayer, } from "../shared/editor-layers.ts"; +const MAX_UNDO_ATTACHMENTS = 128; const IMAGE_PLACEHOLDER = /\[Image #(\d+)\]/g; const PI_CLIPBOARD_IMAGE = /^pi-clipboard-[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}\.(gif|jpe?g|png|webp)$/i; @@ -186,6 +187,7 @@ function expandWith(text: string, attachments: Iterable) { export class ImageAttachmentStore { private nextAttachmentId = 1; private readonly draft = new Map(); + private readonly removed = new Map(); get hasDraft() { return this.draft.size > 0; @@ -202,7 +204,7 @@ export class ImageAttachmentStore { return placeholder; } - reconcileDraft(text: string) { + reconcileDraft(text: string, undo = false) { const occurrenceCounts = new Map(); for (const occurrence of placeholderOccurrences(text)) { occurrenceCounts.set( @@ -210,15 +212,26 @@ export class ImageAttachmentStore { (occurrenceCounts.get(occurrence.id) ?? 0) + 1, ); } - for (const id of [...this.draft.keys()]) { - // Placeholder text is user-editable, so only an unambiguous single - // occurrence keeps its mapping. Duplicated or removed tokens degrade to - // ordinary text; the underlying file is left alone either way. - if (occurrenceCounts.get(id) === 1) continue; + for (const [id, attachment] of this.removed) { + const count = occurrenceCounts.get(id) ?? 0; + if (count === 0) continue; + // Only native Undo can recover ownership. Typing a matching literal or + // duplicating a token revokes it, even if that edit is later undone. + this.removed.delete(id); + if (undo && count === 1) this.draft.set(id, attachment); + } + for (const [id, attachment] of this.draft) { + const count = occurrenceCounts.get(id) ?? 0; + if (count === 1) continue; this.draft.delete(id); + // Undoing an insertion discards it; there is no native redo to recover. + if (count === 0 && !undo) this.removed.set(id, attachment); + } + // Keep only a bounded recovery window, not another editor undo stack. + while (this.removed.size > MAX_UNDO_ATTACHMENTS) { + const oldest = this.removed.keys().next().value; + if (oldest !== undefined) this.removed.delete(oldest); } - this.nextAttachmentId = - this.draft.size === 0 ? 1 : Math.max(...this.draft.keys()) + 1; } /** @@ -244,6 +257,7 @@ export class ImageAttachmentStore { clearDraft() { this.draft.clear(); + this.removed.clear(); this.nextAttachmentId = 1; } @@ -264,6 +278,7 @@ export class ImageAttachmentStore { if (matches.length === 0) return undefined; this.clearDraft(); + const reserved = new Set(placeholderOccurrences(text).map(({ id }) => id)); // Build replacements in reverse document order so earlier indices stay // valid while we splice. Each occurrence — even of the same path — gets // its own placeholder identity so that expandWith's single-occurrence @@ -272,6 +287,7 @@ export class ImageAttachmentStore { []; for (const match of matches) { const path = match[0]; + while (reserved.has(this.nextAttachmentId)) this.nextAttachmentId += 1; const id = this.nextAttachmentId++; const placeholder = `[Image #${id}]`; this.draft.set(id, { id, placeholder, path } satisfies Attachment); @@ -338,6 +354,7 @@ export class ImageAttachmentEditor extends BelowEditorNavigationEditor { private downstreamSubmit?: (text: string) => void; private submittedDraft?: readonly Attachment[]; private settingText = false; + private restoringUndo = false; constructor( base: EditorComponent, @@ -367,12 +384,11 @@ export class ImageAttachmentEditor extends BelowEditorNavigationEditor { this.downstreamChange = value; super.onChange = (text) => { if (!this.settingText) { - // submitValue() clears the buffer and reports it here before invoking - // onSubmit, so an empty buffer ends the draft and restarts numbering. - // Keep a snapshot so the submit that caused it can still expand. + // Native deletion and submit both report an empty buffer. Retain + // recoverable mappings until onSubmit confirms the draft ended. if (text.length === 0) { this.submittedDraft = this.attachments.snapshotDraft(); - this.attachments.clearDraft(); + this.attachments.reconcileDraft(text, this.restoringUndo); } else { this.submittedDraft = undefined; // History recall uses setTextInternal (bypasses setText) and only @@ -389,7 +405,7 @@ export class ImageAttachmentEditor extends BelowEditorNavigationEditor { this.settingText = false; } } else { - this.attachments.reconcileDraft(text); + this.attachments.reconcileDraft(text, this.restoringUndo); } } } @@ -411,11 +427,11 @@ export class ImageAttachmentEditor extends BelowEditorNavigationEditor { // an already expanded string is a no-op. const snapshot = this.submittedDraft; this.submittedDraft = undefined; - value( - snapshot - ? expandWith(text, snapshot) - : this.attachments.expandPlaceholders(text), - ); + const expanded = snapshot + ? expandWith(text, snapshot) + : this.attachments.expandPlaceholders(text); + this.attachments.clearDraft(); + value(expanded); } : undefined; } @@ -435,6 +451,7 @@ export class ImageAttachmentEditor extends BelowEditorNavigationEditor { // Pi restores queued messages and history entries as expanded paths. Adopt // them so the buffer shows compact tokens and the next submit can expand // again; ordinary text falls through to plain reconciliation. + this.submittedDraft = undefined; const adopted = text.length > 0 ? this.attachments.adoptExpandedText(text) : undefined; const next = adopted ?? text; @@ -498,7 +515,15 @@ export class ImageAttachmentEditor extends BelowEditorNavigationEditor { ) { return; } - super.handleInput(data); + this.restoringUndo = this.editorKeybindings.matches( + data, + "tui.editor.undo", + ); + try { + super.handleInput(data); + } finally { + this.restoringUndo = false; + } } } @@ -531,6 +556,7 @@ export default function imagePaste( ); pi.on("session_start", (_event, ctx) => { + attachments.cleanup(); installImagePasteEditor(pi, ctx, attachments); }); diff --git a/tests/extensions/image-paste/index.test.ts b/tests/extensions/image-paste/index.test.ts index 8c509c8ac..6b9ccdf31 100644 --- a/tests/extensions/image-paste/index.test.ts +++ b/tests/extensions/image-paste/index.test.ts @@ -222,7 +222,7 @@ test("duplicating a placeholder makes both copies ordinary text", () => { assert.equal(existsSync(path), true); }); -test("deleting the last image makes its number available to the next paste", () => { +test("deleted image numbers stay reserved for native undo", () => { const first = temporaryImage("png", "first"); const second = temporaryImage("png", "second"); const third = temporaryImage("png", "third"); @@ -239,7 +239,7 @@ test("deleting the last image makes its number available to the next paste", () base.cursor = editor.getText().length; editor.insertTextAtCursor(third); - assert.equal(editor.getText(), "[Image #1] [Image #2]"); + assert.equal(editor.getText(), "[Image #1] [Image #3]"); base.submit(); assert.equal(submitted[0], `${first} ${third}`); diff --git a/tests/extensions/image-paste/native-editor.test.ts b/tests/extensions/image-paste/native-editor.test.ts new file mode 100644 index 000000000..27ef47e08 --- /dev/null +++ b/tests/extensions/image-paste/native-editor.test.ts @@ -0,0 +1,188 @@ +import assert from "node:assert/strict"; +import { randomUUID } from "node:crypto"; +import { rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; +import { + InteractiveMode, + type KeybindingsManager, +} from "@earendil-works/pi-coding-agent"; +import { + Editor, + getKeybindings, + ProcessTerminal, + TuiMainScreen, +} from "@earendil-works/pi-tui"; +import { + ImageAttachmentEditor, + ImageAttachmentStore, +} from "../../../extensions/image-paste/index.ts"; + +const files: string[] = []; +test.after(() => { + for (const path of files) rmSync(path, { force: true }); +}); +function image() { + const path = join(tmpdir(), `pi-clipboard-${randomUUID()}.png`); + writeFileSync( + path, + Buffer.from( + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+a7X8AAAAASUVORK5CYII=", + "base64", + ), + ); + files.push(path); + return path; +} +function harness() { + const identity = (text: string) => text; + const base = new Editor(new TuiMainScreen(new ProcessTerminal()), { + borderColor: identity, + selectList: { + selectedPrefix: identity, + selectedText: identity, + description: identity, + scrollInfo: identity, + noMatch: identity, + }, + }); + const store = new ImageAttachmentStore(); + const editor = new ImageAttachmentEditor( + base, + getKeybindings() as unknown as KeybindingsManager, + store, + ); + const submitted: string[] = []; + editor.onSubmit = (text) => submitted.push(text); + return { editor, store, submitted }; +} +async function roundTrip(editor: ImageAttachmentEditor) { + const queue: string[] = []; + const host = { + editor, + ui: { requestRender() {} }, + session: { + isCompacting: false, + isStreaming: true, + async prompt(text: string) { + queue.push(text); + }, + }, + updatePendingMessagesDisplay() {}, + clearAllQueues() { + return { steering: [], followUp: queue.splice(0) }; + }, + }; + // Exercise the locked native private hooks without constructing a provider session. + const hooks = InteractiveMode.prototype as unknown as { + handleFollowUp(this: typeof host): Promise; + restoreQueuedMessagesToEditor(this: typeof host): number; + }; + await hooks.handleFollowUp.call(host); + const queued = queue[0]; + assert.equal(editor.getText(), ""); + assert.equal(hooks.restoreQueuedMessagesToEditor.call(host), 1); + return queued; +} + +test("native queue restoration preserves literal tokens and repeated image paths", async () => { + const { editor, submitted } = harness(); + const path = image(); + editor.handleInput("[Image #1] [Image #3] "); + editor.insertTextAtCursor(path); + editor.handleInput(" and "); + editor.insertTextAtCursor(path); + const queued = await roundTrip(editor); + assert.equal(queued, `[Image #1] [Image #3] ${path} and ${path}`); + assert.equal( + editor.getText(), + "[Image #1] [Image #3] [Image #2] and [Image #4]", + ); + editor.handleInput("\r"); + assert.equal(submitted[0], queued); +}); + +test("native Undo recovers a removed attachment without reusing its identity", () => { + const { editor, submitted } = harness(); + const first = image(), + second = image(); + editor.insertTextAtCursor(first); + editor.handleInput("\x15"); // Ctrl+U + assert.equal(editor.getText(), ""); + editor.insertTextAtCursor(second); + assert.equal(editor.getText(), "[Image #2]"); + editor.handleInput("\x1f"); // Undo second insertion + editor.handleInput("\x1f"); // Undo Ctrl+U + assert.equal(editor.getExpandedText(), first); + editor.handleInput("\r"); + assert.deepEqual(submitted, [first]); +}); + +test("typing a removed token remains literal even after native Undo", () => { + const { editor, submitted } = harness(); + editor.insertTextAtCursor(image()); + editor.handleInput("\x15"); + editor.handleInput("[Image #1]"); + assert.equal(editor.getExpandedText(), "[Image #1]"); + editor.handleInput("\x15"); + editor.handleInput("\x1f"); + editor.handleInput("\r"); + assert.deepEqual(submitted, ["[Image #1]"]); +}); + +test("explicit clear and cleanup cannot resurrect attachments through native Undo", () => { + for (const reset of ["clear", "cleanup"] as const) { + const { editor, store, submitted } = harness(); + editor.insertTextAtCursor(image()); + if (reset === "clear") editor.setText(""); + else { + editor.handleInput("\x15"); + store.cleanup(); + } + editor.handleInput("\x1f"); + editor.handleInput("\r"); + assert.deepEqual(submitted, ["[Image #1]"]); + } +}); + +test("submission releases removed mappings before the next native draft", () => { + const { editor, submitted } = harness(); + editor.insertTextAtCursor(image()); + editor.handleInput("\x15"); + editor.handleInput("ordinary"); + editor.handleInput("\r"); + editor.handleInput("[Image #1]"); + editor.handleInput("\r"); + assert.deepEqual(submitted, ["ordinary", "[Image #1]"]); +}); + +test("attachment recovery keeps a bounded window of removed identities", () => { + const store = new ImageAttachmentStore(); + const path = image(); + const first = store.attachClipboardPath(path, ""); + let last = first; + for (let i = 0; i < 256; i += 1) { + store.reconcileDraft(""); + last = store.attachClipboardPath(path, ""); + } + store.reconcileDraft(""); + assert(first && last); + store.reconcileDraft(`${first} ${last}`, true); + assert.equal( + store.expandPlaceholders(`${first} ${last}`), + `${first} ${path}`, + ); +}); + +test("undoing a new insertion cannot bind an older cleared token to that image", () => { + const { editor, submitted } = harness(); + editor.insertTextAtCursor(image()); + editor.handleInput("\x15"); + editor.setText(""); + editor.insertTextAtCursor(image()); + editor.handleInput("\x1f"); + editor.handleInput("\x1f"); + editor.handleInput("\r"); + assert.deepEqual(submitted, ["[Image #1]"]); +}); From e17086ff9d3d467e44fb141a8da1d9f29cb8e7d3 Mon Sep 17 00:00:00 2001 From: tt-a1i Date: Thu, 15 Oct 2026 15:21:22 +0800 Subject: [PATCH 2/4] fix(image-paste): reconcile bounded Undo references with file ownership --- extensions/image-paste/index.ts | 56 ++++- .../image-paste/native-editor.test.ts | 194 +++++++++++++++++- 2 files changed, 237 insertions(+), 13 deletions(-) diff --git a/extensions/image-paste/index.ts b/extensions/image-paste/index.ts index bc553dbd5..05be14f6f 100644 --- a/extensions/image-paste/index.ts +++ b/extensions/image-paste/index.ts @@ -182,8 +182,8 @@ function expandWith(text: string, attachments: Iterable) { * message Pi sends is byte-identical to unmodified Pi. * * Submitted clipboard files remain Pi-owned so the read tool can open them - * later. Direct files that lose their final placeholder before submission are - * reclaimed as abandoned draft state. + * later. Direct files remain referenced by live mappings or the bounded native + * Undo window, and are reclaimed only after their final reference is released. */ export class ImageAttachmentStore { private nextAttachmentId = 1; @@ -197,17 +197,23 @@ export class ImageAttachmentStore { attachClipboardPath(path: string, editorText: string) { if (!isPiClipboardImage(path)) return undefined; + const pathIdentity = normalizedPath(path); + const alreadyReferenced = [ + ...this.draft.values(), + ...this.removed.values(), + ].some((attachment) => normalizedPath(attachment.path) === pathIdentity); let id = this.nextAttachmentId; while (this.draft.has(id) || editorText.includes(`[Image #${id}]`)) id += 1; const placeholder = `[Image #${id}]`; this.nextAttachmentId = id + 1; this.draft.set(id, { id, placeholder, path } satisfies Attachment); - this.directDraftPaths.add(path); + // Re-inserting a known adopted path must not claim a Pi-owned file. + if (!alreadyReferenced) this.directDraftPaths.add(pathIdentity); return placeholder; } - reconcileDraft(text: string, undo = false) { + reconcileDraft(text: string, undo = false, clearingForSubmit = false) { const occurrenceCounts = new Map(); for (const occurrence of placeholderOccurrences(text)) { occurrenceCounts.set( @@ -235,7 +241,10 @@ export class ImageAttachmentStore { const oldest = this.removed.keys().next().value; if (oldest !== undefined) this.removed.delete(oldest); } - this.reclaimUnreferencedDirectDraftPaths(); + // Native Enter reports the cleared buffer before onSubmit can hand off + // the snapshot. The input wrapper retires this provisional state if no + // submission follows, including when removal evicted an Undo reference. + if (!clearingForSubmit) this.reclaimUnreferencedDirectDraftPaths(); } /** @@ -260,17 +269,30 @@ export class ImageAttachmentStore { } clearDraft() { + // An active draft may be queued or handed to Pi through setText(""). Keep + // main's conservative handoff boundary, but retire older Undo-only files. + for (const attachment of this.draft.values()) { + this.directDraftPaths.delete(normalizedPath(attachment.path)); + } this.draft.clear(); this.removed.clear(); - // A cleared draft can have been submitted, queued, or handed to another - // Pi delivery path. Release ownership without deleting those files. - this.directDraftPaths.clear(); + this.reclaimUnreferencedDirectDraftPaths(); this.nextAttachmentId = 1; } + releaseDeliveredPaths(text: string) { + for (const match of text.matchAll(CLIPBOARD_PATH_IN_TEXT)) { + if (isPiClipboardPath(match[0])) { + this.directDraftPaths.delete(normalizedPath(match[0])); + } + } + } + private reclaimUnreferencedDirectDraftPaths() { const referencedPaths = new Set( - [...this.draft.values()].map((attachment) => attachment.path), + [...this.draft.values(), ...this.removed.values()].map((attachment) => + normalizedPath(attachment.path), + ), ); for (const path of this.directDraftPaths) { if (referencedPaths.has(path)) continue; @@ -300,6 +322,9 @@ export class ImageAttachmentStore { ); if (matches.length === 0) return undefined; + // A recalled/queued path can also occur in the previous Undo window. + // Release the incoming Pi-owned paths before retiring that window. + this.releaseDeliveredPaths(text); this.clearDraft(); const reserved = new Set(placeholderOccurrences(text).map(({ id }) => id)); // Build replacements in reverse document order so earlier indices stay @@ -411,7 +436,11 @@ export class ImageAttachmentEditor extends BelowEditorNavigationEditor { // recoverable mappings until onSubmit confirms the draft ended. if (text.length === 0) { this.submittedDraft = this.attachments.snapshotDraft(); - this.attachments.reconcileDraft(text, this.restoringUndo); + this.attachments.reconcileDraft( + text, + this.restoringUndo, + this.submittedDraft !== undefined, + ); } else { this.submittedDraft = undefined; // History recall uses setTextInternal (bypasses setText) and only @@ -453,6 +482,9 @@ export class ImageAttachmentEditor extends BelowEditorNavigationEditor { const expanded = snapshot ? expandWith(text, snapshot) : this.attachments.expandPlaceholders(text); + // Native Enter clears the buffer before this callback, so the paths + // being delivered may now be referenced only by removed mappings. + this.attachments.releaseDeliveredPaths(expanded); this.attachments.clearDraft(); value(expanded); } @@ -545,6 +577,10 @@ export class ImageAttachmentEditor extends BelowEditorNavigationEditor { try { super.handleInput(data); } finally { + if (this.submittedDraft) { + this.submittedDraft = undefined; + this.attachments.reconcileDraft(this.getText(), this.restoringUndo); + } this.restoringUndo = false; } } diff --git a/tests/extensions/image-paste/native-editor.test.ts b/tests/extensions/image-paste/native-editor.test.ts index 27ef47e08..164901a1c 100644 --- a/tests/extensions/image-paste/native-editor.test.ts +++ b/tests/extensions/image-paste/native-editor.test.ts @@ -1,11 +1,13 @@ import assert from "node:assert/strict"; import { randomUUID } from "node:crypto"; -import { rmSync, writeFileSync } from "node:fs"; +import { existsSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import test from "node:test"; import { InteractiveMode, + type ExtensionAPI, + type ExtensionContext, type KeybindingsManager, } from "@earendil-works/pi-coding-agent"; import { @@ -14,7 +16,7 @@ import { ProcessTerminal, TuiMainScreen, } from "@earendil-works/pi-tui"; -import { +import imagePaste, { ImageAttachmentEditor, ImageAttachmentStore, } from "../../../extensions/image-paste/index.ts"; @@ -107,16 +109,21 @@ test("native Undo recovers a removed attachment without reusing its identity", ( const { editor, submitted } = harness(); const first = image(), second = image(); + const bytes = readFileSync(first); editor.insertTextAtCursor(first); editor.handleInput("\x15"); // Ctrl+U assert.equal(editor.getText(), ""); + assert.deepEqual(readFileSync(first), bytes); editor.insertTextAtCursor(second); assert.equal(editor.getText(), "[Image #2]"); editor.handleInput("\x1f"); // Undo second insertion + assert.equal(existsSync(second), false); editor.handleInput("\x1f"); // Undo Ctrl+U assert.equal(editor.getExpandedText(), first); + assert.deepEqual(readFileSync(first), bytes); editor.handleInput("\r"); assert.deepEqual(submitted, [first]); + assert.deepEqual(readFileSync(first), bytes); }); test("typing a removed token remains literal even after native Undo", () => { @@ -134,12 +141,43 @@ test("typing a removed token remains literal even after native Undo", () => { test("explicit clear and cleanup cannot resurrect attachments through native Undo", () => { for (const reset of ["clear", "cleanup"] as const) { const { editor, store, submitted } = harness(); - editor.insertTextAtCursor(image()); + const path = image(); + editor.insertTextAtCursor(path); if (reset === "clear") editor.setText(""); else { editor.handleInput("\x15"); store.cleanup(); } + // Native setText("") can also mean queue handoff; only the proven + // removed-only cleanup window permits unlinking the file here. + assert.equal(existsSync(path), reset === "clear"); + editor.handleInput("\x1f"); + editor.handleInput("\r"); + assert.deepEqual(submitted, ["[Image #1]"]); + } +}); + +test("native Undo cannot revive a removed PNG across extension Session boundaries", () => { + for (const boundary of ["session_start", "session_shutdown"]) { + const { editor, store, submitted } = harness(); + const hooks = new Map unknown>(); + imagePaste( + { + registerMarkdownTransformer() {}, + on(event: string, handler: (...args: unknown[]) => unknown) { + hooks.set(event, handler); + }, + events: { emit() {} }, + } as unknown as ExtensionAPI, + store, + ); + const path = image(); + const bytes = readFileSync(path); + editor.insertTextAtCursor(path); + editor.handleInput("\x15"); + assert.deepEqual(readFileSync(path), bytes); + hooks.get(boundary)?.({}, { mode: "rpc" } as ExtensionContext); + assert.equal(existsSync(path), false); editor.handleInput("\x1f"); editor.handleInput("\r"); assert.deepEqual(submitted, ["[Image #1]"]); @@ -186,3 +224,153 @@ test("undoing a new insertion cannot bind an older cleared token to that image", editor.handleInput("\r"); assert.deepEqual(submitted, ["[Image #1]"]); }); + +test("querying expanded text does not hand off a native insertion undone before delivery", () => { + const { editor } = harness(); + const path = image(); + editor.insertTextAtCursor(path); + assert.equal(editor.getExpandedText(), path); + assert.equal(editor.getExpandedText(), path); + editor.handleInput("\x1f"); + assert.equal(existsSync(path), false); +}); + +test("128 removed references retain PNG bytes and eviction reclaims only the oldest file", () => { + const store = new ImageAttachmentStore(); + const paths = Array.from({ length: 129 }, () => image()); + const bytes = readFileSync(paths[0]!); + const tokens: string[] = []; + for (const path of paths) { + tokens.push(store.attachClipboardPath(path, "")!); + store.reconcileDraft(""); + } + assert.equal(existsSync(paths[0]!), false); + for (const path of paths.slice(1)) + assert.deepEqual(readFileSync(path), bytes); + store.reconcileDraft(`${tokens[0]} ${tokens.at(-1)}`, true); + assert.equal( + store.expandPlaceholders(`${tokens[0]} ${tokens.at(-1)}`), + `${tokens[0]} ${paths.at(-1)}`, + ); + store.clearDraft(); + assert.equal(existsSync(paths[1]!), false); + assert.deepEqual(readFileSync(paths.at(-1)!), bytes); +}); + +test("shared physical paths survive until their last eligible removed reference is revoked", () => { + const store = new ImageAttachmentStore(); + const path = image(); + const bytes = readFileSync(path); + const first = store.attachClipboardPath(path, "")!; + const second = store.attachClipboardPath(path, first)!; + store.reconcileDraft(second); + store.reconcileDraft(`${first} ${second}`); + assert.deepEqual(readFileSync(path), bytes); + store.reconcileDraft(""); + assert.deepEqual(readFileSync(path), bytes); + store.reconcileDraft(`${second} ${second}`); + assert.equal(existsSync(path), false); +}); + +test("plain Enter transfers snapshot paths before reclaiming the old Undo window", () => { + const { editor, submitted } = harness(); + const abandoned = image(); + const delivered = image(); + const bytes = readFileSync(delivered); + editor.insertTextAtCursor(abandoned); + editor.handleInput("\x15"); + editor.insertTextAtCursor(delivered); + editor.handleInput("\r"); + assert.deepEqual(submitted, [delivered]); + assert.deepEqual(readFileSync(delivered), bytes); + assert.equal(existsSync(abandoned), false); +}); + +test("native queue and history adoption preserve handed-off PNGs and reclaim unrelated removed files", async () => { + const { editor } = harness(); + const abandoned = image(); + const path = image(); + const bytes = readFileSync(path); + editor.insertTextAtCursor(abandoned); + editor.handleInput("\x15"); + editor.insertTextAtCursor(path); + assert.equal(await roundTrip(editor), path); + assert.equal(existsSync(abandoned), false); + assert.deepEqual(readFileSync(path), bytes); + editor.handleInput("\x15"); + editor.handleInput("[Image #1]"); + assert.deepEqual(readFileSync(path), bytes); + editor.setText(path); + editor.addToHistory(path); + editor.setText(""); + editor.handleInput("\x1b[A"); + assert.equal(editor.getExpandedText(), path); + editor.handleInput("\x15"); + editor.handleInput("[Image #1]"); + assert.deepEqual(readFileSync(path), bytes); +}); + +test("adoption protects an incoming path still referenced by the old removed window", () => { + const { editor } = harness(); + const path = image(); + const bytes = readFileSync(path); + editor.insertTextAtCursor(path); + editor.handleInput("\x15"); + editor.setText(path); + editor.insertTextAtCursor(path); + editor.handleInput("\x15"); + editor.setText(""); + assert.deepEqual(readFileSync(path), bytes); +}); + +test("native typed and duplicate tokens revoke file ownership without Undo revival", () => { + for (const duplicate of [false, true]) { + const { editor } = harness(); + const path = image(); + editor.insertTextAtCursor(path); + if (!duplicate) editor.handleInput("\x15"); + editor.handleInput("[Image #1]"); + assert.equal(existsSync(path), false); + editor.handleInput("\x1f"); + assert.doesNotMatch(editor.getExpandedText(), /pi-clipboard-/); + } +}); + +test("normalized aliases of the same physical PNG share direct-file ownership", () => { + const store = new ImageAttachmentStore(); + const path = image(); + const bytes = readFileSync(path); + const alias = path.replace(/([\\/])([^\\/]+)$/, "$1.$1$2"); + const first = store.attachClipboardPath(path, "")!; + const second = store.attachClipboardPath(alias, first)!; + store.reconcileDraft(`${first}${first} ${second}`); + assert.deepEqual(readFileSync(alias), bytes); + store.reconcileDraft(`${second}${second}`); + assert.equal(existsSync(path), false); +}); + +test("native Enter hands off every active PNG even when the draft exceeds the Undo window", () => { + const { editor, submitted } = harness(); + const paths = Array.from({ length: 129 }, () => image()); + const bytes = readFileSync(paths[0]!); + for (const path of paths) editor.insertTextAtCursor(path); + editor.handleInput("\r"); + assert.deepEqual(submitted, [paths.join("")]); + for (const path of paths) assert.deepEqual(readFileSync(path), bytes); +}); + +test("native non-submit clearing still retires references beyond the 128 Undo window", () => { + const { editor } = harness(); + const paths = Array.from({ length: 129 }, () => image()); + const bytes = readFileSync(paths[0]!); + for (const path of paths) editor.insertTextAtCursor(path); + editor.handleInput("\x15"); + assert.equal(existsSync(paths[0]!), false); + for (const path of paths.slice(1)) + assert.deepEqual(readFileSync(path), bytes); + editor.handleInput("\x1f"); + assert.equal( + editor.getExpandedText(), + `[Image #1]${paths.slice(1).join("")}`, + ); +}); From 8c59d4619fdcf1c16d60459a2213b2af48cb6ded Mon Sep 17 00:00:00 2001 From: tt-a1i Date: Thu, 15 Oct 2026 15:40:25 +0800 Subject: [PATCH 3/4] fix(image-paste): verify physical identity before reclaiming files --- extensions/image-paste/index.ts | 101 +++++++++++++++--- .../image-paste/native-editor.test.ts | 98 ++++++++++++++++- 2 files changed, 182 insertions(+), 17 deletions(-) diff --git a/extensions/image-paste/index.ts b/extensions/image-paste/index.ts index 05be14f6f..d38d06b2f 100644 --- a/extensions/image-paste/index.ts +++ b/extensions/image-paste/index.ts @@ -1,6 +1,7 @@ -import { rmSync, statSync } from "node:fs"; +import { lstatSync, realpathSync, rmSync, statSync } from "node:fs"; import { tmpdir } from "node:os"; import { basename, dirname, extname, resolve } from "node:path"; +import { isDeepStrictEqual } from "node:util"; import type { ExtensionAPI, ExtensionContext, @@ -47,6 +48,47 @@ function normalizedPath(path: string) { return process.platform === "win32" ? normalized.toLowerCase() : normalized; } +function physicalPath(path: string) { + try { + return normalizedPath(realpathSync(path)); + } catch { + return undefined; + } +} + +function directFileSnapshot(path: string) { + try { + // Directory aliases are legitimate, but a foreign leaf symlink does not + // grant ownership of its target. Keep submission paths unchanged. + const initial = lstatSync(path); + if (!initial.isFile()) return undefined; + const canonical = physicalPath(path); + if (!canonical) return undefined; + const stat = lstatSync(canonical); + if ( + !stat.isFile() || + initial.dev !== stat.dev || + initial.ino !== stat.ino || + initial.birthtimeMs !== stat.birthtimeMs || + initial.ctimeMs !== stat.ctimeMs || + initial.mtimeMs !== stat.mtimeMs || + initial.size !== stat.size + ) + return undefined; + return { + path: canonical, + dev: stat.dev, + ino: stat.ino, + birthtimeMs: stat.birthtimeMs, + ctimeMs: stat.ctimeMs, + mtimeMs: stat.mtimeMs, + size: stat.size, + }; + } catch { + return undefined; + } +} + /** * Directories a Pi clipboard image can legitimately come from. * @@ -189,7 +231,10 @@ export class ImageAttachmentStore { private nextAttachmentId = 1; private readonly draft = new Map(); private readonly removed = new Map(); - private readonly directDraftPaths = new Set(); + private readonly directDraftPaths = new Map< + string, + NonNullable> + >(); get hasDraft() { return this.draft.size > 0; @@ -197,11 +242,13 @@ export class ImageAttachmentStore { attachClipboardPath(path: string, editorText: string) { if (!isPiClipboardImage(path)) return undefined; - const pathIdentity = normalizedPath(path); - const alreadyReferenced = [ - ...this.draft.values(), - ...this.removed.values(), - ].some((attachment) => normalizedPath(attachment.path) === pathIdentity); + const pathIdentity = physicalPath(path); + const references = [...this.draft.values(), ...this.removed.values()].map( + (attachment) => physicalPath(attachment.path), + ); + const alreadyReferenced = + !pathIdentity || + references.some((value) => !value || value === pathIdentity); let id = this.nextAttachmentId; while (this.draft.has(id) || editorText.includes(`[Image #${id}]`)) id += 1; @@ -209,7 +256,10 @@ export class ImageAttachmentStore { this.nextAttachmentId = id + 1; this.draft.set(id, { id, placeholder, path } satisfies Attachment); // Re-inserting a known adopted path must not claim a Pi-owned file. - if (!alreadyReferenced) this.directDraftPaths.add(pathIdentity); + const owned = !alreadyReferenced ? directFileSnapshot(path) : undefined; + if (owned && !this.directDraftPaths.has(owned.path)) { + this.directDraftPaths.set(owned.path, owned); + } return placeholder; } @@ -272,7 +322,12 @@ export class ImageAttachmentStore { // An active draft may be queued or handed to Pi through setText(""). Keep // main's conservative handoff boundary, but retire older Undo-only files. for (const attachment of this.draft.values()) { - this.directDraftPaths.delete(normalizedPath(attachment.path)); + const identity = physicalPath(attachment.path); + if (!identity) { + this.directDraftPaths.clear(); + break; + } + this.directDraftPaths.delete(identity); } this.draft.clear(); this.removed.clear(); @@ -283,20 +338,34 @@ export class ImageAttachmentStore { releaseDeliveredPaths(text: string) { for (const match of text.matchAll(CLIPBOARD_PATH_IN_TEXT)) { if (isPiClipboardPath(match[0])) { - this.directDraftPaths.delete(normalizedPath(match[0])); + const identity = physicalPath(match[0]); + if (!identity) { + this.directDraftPaths.clear(); + return; + } + this.directDraftPaths.delete(identity); } } } private reclaimUnreferencedDirectDraftPaths() { - const referencedPaths = new Set( - [...this.draft.values(), ...this.removed.values()].map((attachment) => - normalizedPath(attachment.path), - ), - ); - for (const path of this.directDraftPaths) { + const referencedPaths = new Set(); + for (const attachment of [ + ...this.draft.values(), + ...this.removed.values(), + ]) { + const identity = physicalPath(attachment.path); + // An unresolved reference might alias an owned path. It cannot authorize + // reclamation, and an unresolved handoff relinquishes ownership above. + if (!identity) return; + referencedPaths.add(identity); + } + for (const [path, owned] of this.directDraftPaths) { if (referencedPaths.has(path)) continue; this.directDraftPaths.delete(path); + // A move, replacement or external modification makes ownership uncertain. + // Never unlink a new file or a substituted leaf symlink at an old name. + if (!isDeepStrictEqual(directFileSnapshot(path), owned)) continue; try { rmSync(path, { force: true }); } catch { diff --git a/tests/extensions/image-paste/native-editor.test.ts b/tests/extensions/image-paste/native-editor.test.ts index 164901a1c..9b35a4a23 100644 --- a/tests/extensions/image-paste/native-editor.test.ts +++ b/tests/extensions/image-paste/native-editor.test.ts @@ -1,6 +1,14 @@ import assert from "node:assert/strict"; import { randomUUID } from "node:crypto"; -import { existsSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + existsSync, + readFileSync, + realpathSync, + renameSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import test from "node:test"; @@ -374,3 +382,91 @@ test("native non-submit clearing still retires references beyond the 128 Undo wi `[Image #1]${paths.slice(1).join("")}`, ); }); + +test("macOS physical temp aliases share active, adopted, and delivered PNG ownership", { + skip: process.platform !== "darwin", +}, () => { + for (const mode of ["active", "adopt", "deliver"]) { + const store = new ImageAttachmentStore(); + const path = image(); + const bytes = readFileSync(path); + const alias = path.startsWith("/private/") + ? path.slice(8) + : `/private${path}`; + assert.equal(realpathSync(path), realpathSync(alias)); + const first = store.attachClipboardPath(path, "")!; + if (mode === "active") { + const second = store.attachClipboardPath(alias, first)!; + store.reconcileDraft(`${first}${first} ${second}`); + assert.equal(store.expandPlaceholders(second), alias); + } else { + store.reconcileDraft(""); + if (mode === "adopt") { + const token = store.adoptExpandedText(alias)!; + assert.equal(store.expandPlaceholders(token), alias); + } else { + store.releaseDeliveredPaths(alias); + store.clearDraft(); + } + } + assert.deepEqual(readFileSync(alias), bytes); + } +}); + +test("cleanup preserves a replaced or externally modified clipboard file", () => { + for (const replacement of [true, false]) { + const store = new ImageAttachmentStore(); + const path = image(); + const token = store.attachClipboardPath(path, "")!; + if (replacement) { + const moved = `${path}.moved`; + files.push(moved); + renameSync(path, moved); + } + const foreignBytes = Buffer.from("foreign replacement, not an owned draft"); + writeFileSync(path, foreignBytes); + store.reconcileDraft(`${token}${token}`); + assert.deepEqual(readFileSync(path), foreignBytes); + } +}); + +test("an unresolved live file identity cannot authorize another draft file's deletion", () => { + const store = new ImageAttachmentStore(); + const missing = image(); + const other = image(); + const bytes = readFileSync(other); + const first = store.attachClipboardPath(missing, "")!; + const second = store.attachClipboardPath(other, first)!; + const moved = `${missing}.moved`; + files.push(moved); + renameSync(missing, moved); + store.reconcileDraft(`${first} ${second}${second}`); + assert.deepEqual(readFileSync(other), bytes); + store.clearDraft(); + assert.deepEqual(readFileSync(other), bytes); +}); + +test("a foreign leaf symlink never grants ownership of its target", { + skip: process.platform === "win32", +}, () => { + const store = new ImageAttachmentStore(); + const target = image(); + const bytes = readFileSync(target); + const link = join(tmpdir(), `pi-clipboard-${randomUUID()}.png`); + files.push(link); + symlinkSync(target, link); + const token = store.attachClipboardPath(link, "")!; + assert.ok(token); + store.reconcileDraft(`${token}${token}`); + assert.deepEqual(readFileSync(target), bytes); + assert.equal(existsSync(link), true); + const ownedPath = image(); + const ownedToken = store.attachClipboardPath(ownedPath, "")!; + const moved = `${ownedPath}.moved`; + files.push(moved); + renameSync(ownedPath, moved); + symlinkSync(target, ownedPath); + store.reconcileDraft(`${ownedToken}${ownedToken}`); + assert.deepEqual(readFileSync(target), bytes); + assert.equal(existsSync(ownedPath), true); +}); From 43884820e865e439e0e4231f5c5f787ca2d77627 Mon Sep 17 00:00:00 2001 From: tt-a1i Date: Thu, 15 Oct 2026 15:45:17 +0800 Subject: [PATCH 4/4] fix(image-paste): key ownership by physical file identity --- extensions/image-paste/index.ts | 53 ++++++++++--------- .../image-paste/native-editor.test.ts | 44 ++++++++++++++- 2 files changed, 72 insertions(+), 25 deletions(-) diff --git a/extensions/image-paste/index.ts b/extensions/image-paste/index.ts index d38d06b2f..ceffd5973 100644 --- a/extensions/image-paste/index.ts +++ b/extensions/image-paste/index.ts @@ -48,9 +48,13 @@ function normalizedPath(path: string) { return process.platform === "win32" ? normalized.toLowerCase() : normalized; } -function physicalPath(path: string) { +function physicalIdentity(path: string) { try { - return normalizedPath(realpathSync(path)); + const stat = statSync(path, { bigint: true }); + // Device/inode identify the existing file even when realpath preserves an + // alias's spelling. Birth time distinguishes subsequent inode generations. + if (!stat.isFile() || stat.ino === 0n) return undefined; + return `${stat.dev}:${stat.ino}:${stat.birthtimeNs}`; } catch { return undefined; } @@ -60,28 +64,29 @@ function directFileSnapshot(path: string) { try { // Directory aliases are legitimate, but a foreign leaf symlink does not // grant ownership of its target. Keep submission paths unchanged. - const initial = lstatSync(path); + const initial = lstatSync(path, { bigint: true }); if (!initial.isFile()) return undefined; - const canonical = physicalPath(path); - if (!canonical) return undefined; - const stat = lstatSync(canonical); + const canonical = realpathSync(path); + const stat = lstatSync(canonical, { bigint: true }); if ( !stat.isFile() || + stat.ino === 0n || initial.dev !== stat.dev || initial.ino !== stat.ino || - initial.birthtimeMs !== stat.birthtimeMs || - initial.ctimeMs !== stat.ctimeMs || - initial.mtimeMs !== stat.mtimeMs || + initial.birthtimeNs !== stat.birthtimeNs || + initial.ctimeNs !== stat.ctimeNs || + initial.mtimeNs !== stat.mtimeNs || initial.size !== stat.size ) return undefined; return { path: canonical, + identity: `${stat.dev}:${stat.ino}:${stat.birthtimeNs}`, dev: stat.dev, ino: stat.ino, - birthtimeMs: stat.birthtimeMs, - ctimeMs: stat.ctimeMs, - mtimeMs: stat.mtimeMs, + birthtimeNs: stat.birthtimeNs, + ctimeNs: stat.ctimeNs, + mtimeNs: stat.mtimeNs, size: stat.size, }; } catch { @@ -242,9 +247,9 @@ export class ImageAttachmentStore { attachClipboardPath(path: string, editorText: string) { if (!isPiClipboardImage(path)) return undefined; - const pathIdentity = physicalPath(path); + const pathIdentity = physicalIdentity(path); const references = [...this.draft.values(), ...this.removed.values()].map( - (attachment) => physicalPath(attachment.path), + (attachment) => physicalIdentity(attachment.path), ); const alreadyReferenced = !pathIdentity || @@ -257,8 +262,8 @@ export class ImageAttachmentStore { this.draft.set(id, { id, placeholder, path } satisfies Attachment); // Re-inserting a known adopted path must not claim a Pi-owned file. const owned = !alreadyReferenced ? directFileSnapshot(path) : undefined; - if (owned && !this.directDraftPaths.has(owned.path)) { - this.directDraftPaths.set(owned.path, owned); + if (owned && !this.directDraftPaths.has(owned.identity)) { + this.directDraftPaths.set(owned.identity, owned); } return placeholder; } @@ -322,7 +327,7 @@ export class ImageAttachmentStore { // An active draft may be queued or handed to Pi through setText(""). Keep // main's conservative handoff boundary, but retire older Undo-only files. for (const attachment of this.draft.values()) { - const identity = physicalPath(attachment.path); + const identity = physicalIdentity(attachment.path); if (!identity) { this.directDraftPaths.clear(); break; @@ -338,7 +343,7 @@ export class ImageAttachmentStore { releaseDeliveredPaths(text: string) { for (const match of text.matchAll(CLIPBOARD_PATH_IN_TEXT)) { if (isPiClipboardPath(match[0])) { - const identity = physicalPath(match[0]); + const identity = physicalIdentity(match[0]); if (!identity) { this.directDraftPaths.clear(); return; @@ -354,20 +359,20 @@ export class ImageAttachmentStore { ...this.draft.values(), ...this.removed.values(), ]) { - const identity = physicalPath(attachment.path); + const identity = physicalIdentity(attachment.path); // An unresolved reference might alias an owned path. It cannot authorize // reclamation, and an unresolved handoff relinquishes ownership above. if (!identity) return; referencedPaths.add(identity); } - for (const [path, owned] of this.directDraftPaths) { - if (referencedPaths.has(path)) continue; - this.directDraftPaths.delete(path); + for (const [identity, owned] of this.directDraftPaths) { + if (referencedPaths.has(identity)) continue; + this.directDraftPaths.delete(identity); // A move, replacement or external modification makes ownership uncertain. // Never unlink a new file or a substituted leaf symlink at an old name. - if (!isDeepStrictEqual(directFileSnapshot(path), owned)) continue; + if (!isDeepStrictEqual(directFileSnapshot(owned.path), owned)) continue; try { - rmSync(path, { force: true }); + rmSync(owned.path, { force: true }); } catch { // Editor input remains usable if a temporary file disappeared or the // operating system declines the cleanup. diff --git a/tests/extensions/image-paste/native-editor.test.ts b/tests/extensions/image-paste/native-editor.test.ts index 9b35a4a23..93a6fd6f5 100644 --- a/tests/extensions/image-paste/native-editor.test.ts +++ b/tests/extensions/image-paste/native-editor.test.ts @@ -2,15 +2,17 @@ import assert from "node:assert/strict"; import { randomUUID } from "node:crypto"; import { existsSync, + linkSync, readFileSync, realpathSync, renameSync, rmSync, + statSync, symlinkSync, writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { basename, dirname, join } from "node:path"; import test from "node:test"; import { InteractiveMode, @@ -413,6 +415,46 @@ test("macOS physical temp aliases share active, adopted, and delivered PNG owner } }); +for (const aliasKind of ["case", "hardlink"] as const) { + test(`${aliasKind} aliases share active, adopted, and delivered physical ownership`, (t) => { + for (const mode of ["active", "adopt", "deliver"]) { + const store = new ImageAttachmentStore(); + const path = image(); + const bytes = readFileSync(path); + const alias = + aliasKind === "case" + ? join(dirname(path), basename(path).toUpperCase()) + : join(tmpdir(), `pi-clipboard-${randomUUID()}.png`); + if (aliasKind === "hardlink") { + linkSync(path, alias); + files.push(alias); + } else if (!existsSync(alias)) { + t.skip("filesystem is case sensitive"); + return; + } + assert.equal(statSync(path).dev, statSync(alias).dev); + assert.equal(statSync(path).ino, statSync(alias).ino); + const first = store.attachClipboardPath(path, "")!; + if (mode === "active") { + const second = store.attachClipboardPath(alias, first)!; + store.reconcileDraft(`${first}${first} ${second}`); + assert.equal(store.expandPlaceholders(second), alias); + } else { + store.reconcileDraft(""); + if (mode === "adopt") { + const token = store.adoptExpandedText(alias)!; + assert.equal(store.expandPlaceholders(token), alias); + } else { + store.releaseDeliveredPaths(alias); + store.clearDraft(); + } + } + assert.deepEqual(readFileSync(path), bytes); + assert.deepEqual(readFileSync(alias), bytes); + } + }); +} + test("cleanup preserves a replaced or externally modified clipboard file", () => { for (const replacement of [true, false]) { const store = new ImageAttachmentStore();