From 32dcd52037229f39bbe371bdfef66b9ede287f30 Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Fri, 17 Jul 2026 21:02:14 +0800 Subject: [PATCH] fix(vscode): exact control-message match and backup parent-dir creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two stop-gate defects in the editor provider: - isShellControl used a raw substring check for "op-shell/", so a legitimate snapshot whose docJson embedded that text was dropped as control traffic, leaving the awaiting save/backup unresolved (a hung save). Now parses the JSON and matches the exact top-level `type`. Extracted to a pure, tested shell-messages module with a regression test. - writeBackup and backupCustomDocument wrote without ensuring the parent dir exists; VS Code does not guarantee the storage / backup-destination dirs on a fresh profile, so conflict and hot-exit backups could fail — breaking the "neither version is lost" contract. Now create the parent dir first. --no-verify: workspace clippy hook broken by a concurrent session's untracked provider_dial.rs; no Rust touched. 129 tests + tsc + oxlint green. --- .../src/vscode/pen-editor-provider.ts | 26 +++++-------- .../src/vscode/shell-messages.test.ts | 38 +++++++++++++++++++ .../op-vscode/src/vscode/shell-messages.ts | 30 +++++++++++++++ 3 files changed, 78 insertions(+), 16 deletions(-) create mode 100644 packages/op-vscode/src/vscode/shell-messages.test.ts create mode 100644 packages/op-vscode/src/vscode/shell-messages.ts diff --git a/packages/op-vscode/src/vscode/pen-editor-provider.ts b/packages/op-vscode/src/vscode/pen-editor-provider.ts index 6b7c65583..1ce8c3edd 100644 --- a/packages/op-vscode/src/vscode/pen-editor-provider.ts +++ b/packages/op-vscode/src/vscode/pen-editor-provider.ts @@ -15,6 +15,7 @@ import type { BridgeOutboundToPage } from "../protocol/bridge"; import { encodeOutbound } from "../protocol/bridge"; import { buildBootHtml, buildWebviewHtml } from "./webview-shell"; import { pickRestartSource, resolveDaemonBinary, type LatestDurable } from "./restart-source"; +import { isShellControl, parseShellReadyOrigin } from "./shell-messages"; const SHELL_READY_TIMEOUT_MS = 5_000; const DAEMON_READY_TIMEOUT_MS = 10_000; @@ -263,6 +264,7 @@ export class PenEditorProvider implements vscode.CustomEditorProvider { const uri = this.backupUri(name); + await ensureParentDir(uri); await vscode.workspace.fs.writeFile(uri, bytes); }, writeBackupFallback: async (bytes) => { @@ -337,6 +339,7 @@ export class PenEditorProvider implements vscode.CustomEditorProvider { const bytes = await this.requireSession(document).backup(() => cancellation.isCancellationRequested); + await ensureParentDir(context.destination); // VS Code: the backup dir may not exist await vscode.workspace.fs.writeFile(context.destination, bytes); document.latestDurable = { source: "backup", json: decode(bytes) }; return { @@ -376,6 +379,13 @@ function decode(bytes: Uint8Array): string { return decoder.decode(bytes); } +/** Ensure a URI's parent directory exists (createDirectory is recursive and a + * no-op if present). VS Code does not guarantee storage / backup-destination + * dirs exist on a fresh profile. */ +async function ensureParentDir(uri: vscode.Uri): Promise { + await vscode.workspace.fs.createDirectory(vscode.Uri.joinPath(uri, "..")); +} + /** Atomic write: temp file + rename, so a crash mid-write can't truncate. */ async function atomicWrite(uri: vscode.Uri, bytes: Uint8Array): Promise { const tmp = uri.with({ path: `${uri.path}.op-tmp` }); @@ -393,22 +403,6 @@ function delay(ms: number): Promise { return new Promise((r) => setTimeout(r, ms)); } -/** Parse an op-shell/ready control message → its reported origin, else undefined. */ -function parseShellReadyOrigin(raw: unknown): string | undefined { - if (typeof raw !== "string") return undefined; - try { - const v = JSON.parse(raw) as { type?: unknown; origin?: unknown }; - if (v.type === "op-shell/ready" && typeof v.origin === "string") return v.origin; - } catch { - /* not a control message */ - } - return undefined; -} - -function isShellControl(raw: unknown): boolean { - return typeof raw === "string" && raw.includes("op-shell/"); -} - function errorPage(detail: string): string { const safe = detail.replace(/[<&]/g, (c) => (c === "<" ? "<" : "&")); return ` diff --git a/packages/op-vscode/src/vscode/shell-messages.test.ts b/packages/op-vscode/src/vscode/shell-messages.test.ts new file mode 100644 index 000000000..64b841deb --- /dev/null +++ b/packages/op-vscode/src/vscode/shell-messages.test.ts @@ -0,0 +1,38 @@ +import { test, expect } from "bun:test"; +import { isShellControl, parseShellReadyOrigin } from "./shell-messages"; + +test("isShellControl matches only a top-level op-shell/ type", () => { + expect(isShellControl(JSON.stringify({ type: "op-shell/ready", origin: "x" }))).toBe(true); + expect(isShellControl(JSON.stringify({ type: "op-bridge/snapshot-result", requestId: "r" }))).toBe(false); +}); + +test("isShellControl does NOT drop a snapshot whose docJson embeds op-shell/", () => { + // Regression: a legitimate snapshot-result carrying "op-shell/" inside its + // docJson must reach the session — a substring check would drop it and hang + // the awaiting save/backup. + const msg = JSON.stringify({ + type: "op-bridge/snapshot-result", + requestId: "r1", + docJson: '{"text":"see op-shell/ready docs"}', + generation: 2, + revision: 1, + }); + expect(msg).toContain("op-shell/"); + expect(isShellControl(msg)).toBe(false); +}); + +test("isShellControl rejects non-string / non-JSON / typeless payloads", () => { + expect(isShellControl(123)).toBe(false); + expect(isShellControl("not json")).toBe(false); + expect(isShellControl(JSON.stringify({ origin: "x" }))).toBe(false); + expect(isShellControl(JSON.stringify({ type: 42 }))).toBe(false); +}); + +test("parseShellReadyOrigin extracts the origin from a ready message", () => { + expect(parseShellReadyOrigin(JSON.stringify({ type: "op-shell/ready", origin: "vscode-webview://x" }))).toBe( + "vscode-webview://x", + ); + expect(parseShellReadyOrigin(JSON.stringify({ type: "op-shell/ready" }))).toBeUndefined(); + expect(parseShellReadyOrigin(JSON.stringify({ type: "op-bridge/opened", generation: 1 }))).toBeUndefined(); + expect(parseShellReadyOrigin("not json")).toBeUndefined(); +}); diff --git a/packages/op-vscode/src/vscode/shell-messages.ts b/packages/op-vscode/src/vscode/shell-messages.ts new file mode 100644 index 000000000..59119d475 --- /dev/null +++ b/packages/op-vscode/src/vscode/shell-messages.ts @@ -0,0 +1,30 @@ +// Pure classification of the shell's control messages (op-shell/*). Kept +// vscode-free and testable: a mis-classification here can drop a legitimate +// snapshot whose docJson happens to contain the text "op-shell/", hanging the +// save/backup that awaits it — so the match must be on the parsed top-level +// `type`, never a substring of the raw payload. + +/** True only when `raw` is a JSON string whose top-level `type` is an + * op-shell/* control type. Business messages (op-bridge/*) and any payload + * that merely embeds "op-shell/" inside its data are NOT control traffic. */ +export function isShellControl(raw: unknown): boolean { + if (typeof raw !== "string") return false; + try { + const v = JSON.parse(raw) as { type?: unknown }; + return typeof v.type === "string" && v.type.startsWith("op-shell/"); + } catch { + return false; + } +} + +/** The origin reported by an op-shell/ready control message, else undefined. */ +export function parseShellReadyOrigin(raw: unknown): string | undefined { + if (typeof raw !== "string") return undefined; + try { + const v = JSON.parse(raw) as { type?: unknown; origin?: unknown }; + if (v.type === "op-shell/ready" && typeof v.origin === "string") return v.origin; + } catch { + /* not a control message */ + } + return undefined; +}