From fec8ef406a22fbf3ea121b6261b3831dae7284a9 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:54:09 -0700 Subject: [PATCH] fix(editor): keep the unparseable editor path visible after the URI guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review remediation. Blocking: the guard turned the only observable instance of an unidentified input class into a silent rewrite plus a silent catch. Both paths now leave a shape-only breadcrumb through the existing `recordRendererCrashBreadcrumb` leaf API, so the class still lands in the crash report's "Recent activity" block: - `editor_model_path_uri_rejected` from the fallback rewrite - `editor_model_dispose_path_unkeyable` from the disposal catch Fields are length, colon count, has-backslash, and the character classes of the scheme prefix — never the path itself (a test asserts the payload cannot contain the host name). Also fixed: - The disposal catch's justification comment was stale. It argued no model could leak because @monaco-editor/react keyed with the identical parse; Part 2 makes exactly those paths produce a model. It now says what the branch actually guards: `Uri.file()` itself rejecting the path. - Test coupling gap: MonacoEditor.model-path.test.tsx now feeds the `path` prop Monaco actually received into `disposeClosedEditorTabs`, so create/ dispose key agreement fails if MonacoEditor stops using the helper. The fake registry moved to monaco-model-registry-test-fixture.ts instead of being duplicated. - Windows form pinned: monaco-edit-model-path.windows.test.ts pins `process.platform` before monaco loads and asserts the exact UNC-aware fallback (`file://wsl.localhost/.../notes%3A2026.md`) real users get, plus that it stays a stable dispose key. - `ClosedEditorTabMonacoRegistry` uses `Omit` rather than intersecting a second `Uri` onto a type that declares one, and is exported so tests stop redeclaring it. Widening `MonacoModelRegistry['Uri']` in place was rejected: diff disposal never calls `Uri.file`, so that would force unused members onto its fakes. Correction to the previous write-up, not the code: "that is the producer: a WSL/Linux-produced name joined onto a `\\wsl.localhost` Windows root" was stated as fact and is an inference. The probe shows only that WSL-side git can emit an ASCII colon, and it refutes the Win32-enumeration route (U+F03A does not throw). The honest claim is: mechanism narrowed, producing string not recovered — which is exactly why the breadcrumb above is needed. Verification: 199 files / 1331 tests pass in the editor folder; typecheck exit 0; oxlint on the editor folder exit 0; oxfmt clean; changed-code quality gate reports 0 new findings across 8 files. RED confirmed for the new crumb by removing the fallback record call (1 failed). --- .../editor/MonacoEditor.model-path.test.tsx | 17 ++++ .../editor/closed-editor-tab-disposal.test.ts | 12 +-- .../editor/closed-editor-tab-disposal.ts | 13 ++- .../editor/monaco-edit-model-path.test.ts | 98 +++++++++++-------- .../editor/monaco-edit-model-path.ts | 41 ++++++++ .../monaco-edit-model-path.windows.test.ts | 38 +++++++ .../monaco-model-registry-test-fixture.ts | 44 +++++++++ 7 files changed, 213 insertions(+), 50 deletions(-) create mode 100644 src/renderer/src/components/editor/monaco-edit-model-path.windows.test.ts create mode 100644 src/renderer/src/components/editor/monaco-model-registry-test-fixture.ts diff --git a/src/renderer/src/components/editor/MonacoEditor.model-path.test.tsx b/src/renderer/src/components/editor/MonacoEditor.model-path.test.tsx index b9f3a7aed22..9129eb6bf37 100644 --- a/src/renderer/src/components/editor/MonacoEditor.model-path.test.tsx +++ b/src/renderer/src/components/editor/MonacoEditor.model-path.test.tsx @@ -2,6 +2,9 @@ import { cleanup, render } from '@testing-library/react' import { afterEach, describe, expect, it, vi } from 'vitest' import { Uri } from 'monaco-editor' +import type { OpenFile } from '@/store/slices/editor' +import { disposeClosedEditorTabs } from './closed-editor-tab-disposal' +import { createMonacoModelRegistryWithRealUri } from './monaco-model-registry-test-fixture' const editorProps = vi.hoisted(() => ({ current: null as Record | null })) @@ -72,4 +75,18 @@ describe('MonacoEditor model path', () => { expect(() => renderEditor(WSL_COLON_PATH)).not.toThrow() expect(() => Uri.parse(String(editorProps.current?.path))).not.toThrow() }) + + // Why through the rendered prop, not the helper: this is the only assertion that fails if + // MonacoEditor stops routing `path` through the helper and the close-all key drifts. + it('keys the model on a path close-all can still dispose', () => { + renderEditor(WSL_COLON_PATH) + const renderedPath = String(editorProps.current?.path) + const registry = createMonacoModelRegistryWithRealUri([renderedPath]) + + disposeClosedEditorTabs(registry, [ + { id: 'poisoned', mode: 'edit', filePath: WSL_COLON_PATH } as OpenFile + ]) + + expect(registry.disposed).toEqual([renderedPath]) + }) }) diff --git a/src/renderer/src/components/editor/closed-editor-tab-disposal.test.ts b/src/renderer/src/components/editor/closed-editor-tab-disposal.test.ts index 80cee7751f8..4b63f8b3538 100644 --- a/src/renderer/src/components/editor/closed-editor-tab-disposal.test.ts +++ b/src/renderer/src/components/editor/closed-editor-tab-disposal.test.ts @@ -6,13 +6,14 @@ import { scrollTopCache } from '@/lib/scroll-cache' import type { OpenFile } from '@/store/slices/editor' -import { disposeClosedEditorTabs } from './closed-editor-tab-disposal' +import { + disposeClosedEditorTabs, + type ClosedEditorTabMonacoRegistry +} from './closed-editor-tab-disposal' import { getDiffViewerMonacoModelPaths, - getDiffViewerMonacoModelPathPrefixes, - type MonacoModelRegistry + getDiffViewerMonacoModelPathPrefixes } from './diff-monaco-model-disposal' -import type { MonacoUriNamespace } from './monaco-edit-model-path' const CLOSED_DIFF_TAB_COUNT = 100 const RETAINED_MODEL_COUNT = 320 @@ -26,8 +27,7 @@ type FakeModel = { uri: { toString: (skipEncoding?: boolean) => string } } -type FakeRegistry = MonacoModelRegistry & { - Uri: MonacoUriNamespace +type FakeRegistry = ClosedEditorTabMonacoRegistry & { models: FakeModel[] counters: { getModelsCalls: number; uriToStringCalls: number } } diff --git a/src/renderer/src/components/editor/closed-editor-tab-disposal.ts b/src/renderer/src/components/editor/closed-editor-tab-disposal.ts index c93dba5a0bd..ecb4f766d4f 100644 --- a/src/renderer/src/components/editor/closed-editor-tab-disposal.ts +++ b/src/renderer/src/components/editor/closed-editor-tab-disposal.ts @@ -14,9 +14,15 @@ import { deletePaneScopedCacheEntries, sweepClosedPdfViewPositions } from './closed-editor-tab-cache-sweep' -import { toMonacoEditModelPath, type MonacoUriNamespace } from './monaco-edit-model-path' +import { + recordUnparseableModelPathShape, + toMonacoEditModelPath, + type MonacoUriNamespace +} from './monaco-edit-model-path' -type ClosedEditorTabMonacoRegistry = MonacoModelRegistry & { Uri: MonacoUriNamespace } +export type ClosedEditorTabMonacoRegistry = Omit & { + Uri: MonacoUriNamespace +} function disposeEditTabModel( monacoRegistry: ClosedEditorTabMonacoRegistry, @@ -26,7 +32,8 @@ function disposeEditTabModel( try { modelUri = monacoRegistry.Uri.parse(toMonacoEditModelPath(monacoRegistry.Uri, filePath)) } catch { - // Why safe: @monaco-editor/react keys the model with this identical parse, so a path that throws here never produced a model to leak. + // Reached only if Uri.file() itself rejects the path: no key exists, so nothing to dispose. + recordUnparseableModelPathShape('editor_model_dispose_path_unkeyable', filePath) return } monacoRegistry.editor.getModel(modelUri)?.dispose() diff --git a/src/renderer/src/components/editor/monaco-edit-model-path.test.ts b/src/renderer/src/components/editor/monaco-edit-model-path.test.ts index b2be1a3da8c..193c6d49a66 100644 --- a/src/renderer/src/components/editor/monaco-edit-model-path.test.ts +++ b/src/renderer/src/components/editor/monaco-edit-model-path.test.ts @@ -1,11 +1,22 @@ // @vitest-environment happy-dom -import { describe, expect, it } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { Uri } from 'monaco-editor' import type { OpenFile } from '@/store/slices/editor' import { disposeClosedEditorTabs } from './closed-editor-tab-disposal' import { toMonacoEditModelPath } from './monaco-edit-model-path' -import type { MonacoModelRegistry } from './diff-monaco-model-disposal' -import type { MonacoUriNamespace } from './monaco-edit-model-path' +import { + createMonacoModelRegistryWithRealUri, + type RecordingMonacoModelRegistry +} from './monaco-model-registry-test-fixture' + +const crashBreadcrumbs = vi.hoisted(() => ({ record: vi.fn() })) +vi.mock('@/lib/crash-breadcrumb-recorder', () => ({ + recordRendererCrashBreadcrumb: crashBreadcrumbs.record +})) + +beforeEach(() => { + crashBreadcrumbs.record.mockClear() +}) // Why this exact shape: the field crash (`[UriError]: Scheme contains illegal characters.`) came // from a Windows/WSL workspace. `Uri.parse` reads `\\wsl.localhost\...\notes` as the scheme @@ -13,42 +24,6 @@ import type { MonacoUriNamespace } from './monaco-edit-model-path' const WSL_COLON_PATH = '\\\\wsl.localhost\\Ubuntu-26.04\\home\\mj\\projects\\acp-client\\notes:2026.md' -type RecordingRegistry = MonacoModelRegistry & { - Uri: MonacoUriNamespace - disposed: string[] -} - -function createRegistryWithRealUri(modelPaths: readonly string[]): RecordingRegistry { - const disposed: string[] = [] - const modelsByUri = new Map>() - - function createModel(modelPath: string): { - dispose: () => void - isAttachedToEditor: () => boolean - uri: { toString: (skipEncoding?: boolean) => string } - } { - const key = Uri.parse(modelPath).toString() - return { - dispose: () => disposed.push(modelPath), - isAttachedToEditor: () => false, - uri: { toString: () => key } - } - } - - for (const modelPath of modelPaths) { - modelsByUri.set(Uri.parse(modelPath).toString(), createModel(modelPath)) - } - - return { - disposed, - Uri, - editor: { - getModel: (uri: unknown) => modelsByUri.get(String(uri)) ?? null, - getModels: () => [...modelsByUri.values()] - } - } -} - function editTab(id: string, filePath: string): OpenFile { return { id, mode: 'edit', filePath } as OpenFile } @@ -71,11 +46,32 @@ describe('toMonacoEditModelPath', () => { expect(modelPath).not.toBe(WSL_COLON_PATH) expect(() => Uri.parse(modelPath)).not.toThrow() }) + + // Why: the rewrite is now the only trace of an input class whose producer is unidentified. + it('leaves a shape-only crumb describing the rejected path', () => { + toMonacoEditModelPath(Uri, WSL_COLON_PATH) + + expect(crashBreadcrumbs.record).toHaveBeenCalledWith('editor_model_path_uri_rejected', { + length: WSL_COLON_PATH.length, + colons: 1, + hasBackslash: true, + schemePrefixLength: WSL_COLON_PATH.indexOf(':'), + schemePrefixCharset: 'alpha|backslash|digit|schemeSafePunct' + }) + const [, data] = crashBreadcrumbs.record.mock.calls[0] as [string, Record] + expect(JSON.stringify(data)).not.toContain('wsl.localhost') + }) + + it('records nothing for a path Monaco already accepts', () => { + toMonacoEditModelPath(Uri, '/repo/file.py') + + expect(crashBreadcrumbs.record).not.toHaveBeenCalled() + }) }) describe('disposeClosedEditorTabs with the real Monaco URI parser', () => { it('does not throw the workbench down on the unparseable path class', () => { - const registry = createRegistryWithRealUri([]) + const registry = createMonacoModelRegistryWithRealUri([]) expect(() => disposeClosedEditorTabs(registry, [editTab('poisoned', WSL_COLON_PATH)]) @@ -85,7 +81,7 @@ describe('disposeClosedEditorTabs with the real Monaco URI parser', () => { it('keeps disposing the rest of the close-all batch behind a poisoned tab', () => { const beforePath = '/repo/before.ts' const afterPath = '/repo/after.ts' - const registry = createRegistryWithRealUri([ + const registry = createMonacoModelRegistryWithRealUri([ beforePath, toMonacoEditModelPath(Uri, WSL_COLON_PATH), afterPath @@ -102,4 +98,24 @@ describe('disposeClosedEditorTabs with the real Monaco URI parser', () => { // Why: the poisoned tab's model is reachable too, because open and close now key it the same way. expect(registry.disposed).toContain(toMonacoEditModelPath(Uri, WSL_COLON_PATH)) }) + + it('crumbs instead of throwing when even the fallback URI form is unbuildable', () => { + const registry: RecordingMonacoModelRegistry = { + ...createMonacoModelRegistryWithRealUri([]), + Uri: { + parse: (value: string) => Uri.parse(value), + file: () => { + throw new Error('unbuildable') + } + } + } + + expect(() => + disposeClosedEditorTabs(registry, [editTab('poisoned', WSL_COLON_PATH)]) + ).not.toThrow() + expect(crashBreadcrumbs.record).toHaveBeenCalledWith( + 'editor_model_dispose_path_unkeyable', + expect.objectContaining({ hasBackslash: true }) + ) + }) }) diff --git a/src/renderer/src/components/editor/monaco-edit-model-path.ts b/src/renderer/src/components/editor/monaco-edit-model-path.ts index d895b99c81c..5d643604d32 100644 --- a/src/renderer/src/components/editor/monaco-edit-model-path.ts +++ b/src/renderer/src/components/editor/monaco-edit-model-path.ts @@ -1,8 +1,48 @@ +import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder' + export type MonacoUriNamespace = { parse(value: string): unknown file(path: string): { toString(): string } } +const SCHEME_PREFIX_CHAR_CLASSES: readonly (readonly [string, RegExp])[] = [ + ['alpha', /[a-z]/i], + ['digit', /\d/], + ['backslash', /\\/], + ['slash', /\//], + ['schemeSafePunct', /[.+-]/], + ['space', / /] +] + +/** Character classes before the first `:`, so a crumb names the shape without carrying the path. */ +function schemePrefixCharset(prefix: string): string { + const classes = new Set( + [...prefix].map( + (char) => SCHEME_PREFIX_CHAR_CLASSES.find(([, pattern]) => pattern.test(char))?.[0] ?? 'other' + ) + ) + return [...classes].sort().join('|') +} + +/** + * Shape-only crumb for a path Monaco's URI parser rejects. + * + * Why at all: the guards below turn the field crash into a silent rewrite, and the crash was the + * only signal that ever surfaced this input class — its producer is still unidentified. Shape + * fields only; the crash pipeline redacts raw paths anyway. + */ +export function recordUnparseableModelPathShape(name: string, filePath: string): void { + const path = String(filePath) + const firstColon = path.indexOf(':') + recordRendererCrashBreadcrumb(name, { + length: path.length, + colons: path.split(':').length - 1, + hasBackslash: path.includes('\\'), + schemePrefixLength: firstColon, + schemePrefixCharset: schemePrefixCharset(firstColon === -1 ? '' : path.slice(0, firstColon)) + }) +} + /** * The path an edit tab's Monaco model is keyed by, in a form `Uri.parse` always accepts. * @@ -17,6 +57,7 @@ export function toMonacoEditModelPath(uri: MonacoUriNamespace, filePath: string) uri.parse(filePath) return filePath } catch { + recordUnparseableModelPathShape('editor_model_path_uri_rejected', filePath) return uri.file(filePath).toString() } } diff --git a/src/renderer/src/components/editor/monaco-edit-model-path.windows.test.ts b/src/renderer/src/components/editor/monaco-edit-model-path.windows.test.ts new file mode 100644 index 00000000000..8d22a00c7df --- /dev/null +++ b/src/renderer/src/components/editor/monaco-edit-model-path.windows.test.ts @@ -0,0 +1,38 @@ +// @vitest-environment happy-dom +import { describe, expect, it, vi } from 'vitest' + +// Why a dedicated file: monaco reads `process.platform` once at module load, so the Windows URI +// form — the one real users of this crash get — is only reachable by pinning it before the import. +vi.hoisted(() => { + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) +}) + +import { Uri } from 'monaco-editor' +import { isWindows } from 'monaco-editor/esm/vs/base/common/platform.js' +import type { OpenFile } from '@/store/slices/editor' +import { disposeClosedEditorTabs } from './closed-editor-tab-disposal' +import { toMonacoEditModelPath } from './monaco-edit-model-path' +import { createMonacoModelRegistryWithRealUri } from './monaco-model-registry-test-fixture' + +const WSL_COLON_PATH = + '\\\\wsl.localhost\\Ubuntu-26.04\\home\\mj\\projects\\acp-client\\notes:2026.md' + +describe('toMonacoEditModelPath on Windows', () => { + it('pins the UNC-aware fallback form monaco builds when the renderer is Windows', () => { + expect(isWindows).toBe(true) + expect(toMonacoEditModelPath(Uri, WSL_COLON_PATH)).toBe( + 'file://wsl.localhost/Ubuntu-26.04/home/mj/projects/acp-client/notes%3A2026.md' + ) + }) + + it('keeps the Windows form a stable dispose key', () => { + const modelPath = toMonacoEditModelPath(Uri, WSL_COLON_PATH) + const registry = createMonacoModelRegistryWithRealUri([modelPath]) + + disposeClosedEditorTabs(registry, [ + { id: 'poisoned', mode: 'edit', filePath: WSL_COLON_PATH } as OpenFile + ]) + + expect(registry.disposed).toEqual([modelPath]) + }) +}) diff --git a/src/renderer/src/components/editor/monaco-model-registry-test-fixture.ts b/src/renderer/src/components/editor/monaco-model-registry-test-fixture.ts new file mode 100644 index 00000000000..365981edd39 --- /dev/null +++ b/src/renderer/src/components/editor/monaco-model-registry-test-fixture.ts @@ -0,0 +1,44 @@ +import { Uri } from 'monaco-editor' +import type { ClosedEditorTabMonacoRegistry } from './closed-editor-tab-disposal' + +export type RecordingMonacoModelRegistry = ClosedEditorTabMonacoRegistry & { + disposed: string[] +} + +/** + * A model registry backed by the real Monaco URI parser. + * + * Why real: the whole point of these tests is which paths `Uri.parse` accepts, so a fake parser + * that echoes its input would assert nothing. + */ +export function createMonacoModelRegistryWithRealUri( + modelPaths: readonly string[] +): RecordingMonacoModelRegistry { + const disposed: string[] = [] + const modelsByUri = new Map< + string, + { + dispose: () => void + isAttachedToEditor: () => boolean + uri: { toString: (skipEncoding?: boolean) => string } + } + >() + + for (const modelPath of modelPaths) { + const key = Uri.parse(modelPath).toString() + modelsByUri.set(key, { + dispose: () => disposed.push(modelPath), + isAttachedToEditor: () => false, + uri: { toString: () => key } + }) + } + + return { + disposed, + Uri, + editor: { + getModel: (uri: unknown) => modelsByUri.get(String(uri)) ?? null, + getModels: () => [...modelsByUri.values()] + } + } +}