From 1e49c777765287a14f52336b327d324d2f474811 Mon Sep 17 00:00:00 2001 From: ketan0 Date: Thu, 24 Sep 2026 17:02:45 -0700 Subject: [PATCH] Investigate first-run folder collision and add safe filesystem reproduction Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 --- .../scripts/repro-folder-collision.mjs | 113 ++++++++++++++++++ docs/folder-collision-investigation.md | 80 +++++++++++++ 2 files changed, 193 insertions(+) create mode 100644 apps/review-desktop/scripts/repro-folder-collision.mjs create mode 100644 docs/folder-collision-investigation.md diff --git a/apps/review-desktop/scripts/repro-folder-collision.mjs b/apps/review-desktop/scripts/repro-folder-collision.mjs new file mode 100644 index 000000000..3199bea69 --- /dev/null +++ b/apps/review-desktop/scripts/repro-folder-collision.mjs @@ -0,0 +1,113 @@ +// Run from apps/review-desktop after installing workspace dependencies: +// TSX_TSCONFIG_PATH=tsconfig.test.json node --import tsx scripts/repro-folder-collision.mjs +// This is an investigation harness, not a regression test for an implemented fix. +import assert from "node:assert/strict"; +import * as fs from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { Event } from "../code-oss/src/vs/base/common/event.ts"; +import { URI } from "../code-oss/src/vs/base/common/uri.ts"; +import { + FileSystemProviderCapabilities, + FileSystemProviderErrorCode, + FileType, + createFileSystemProviderError, +} from "../code-oss/src/vs/platform/files/common/files.ts"; +import { FileService } from "../code-oss/src/vs/platform/files/common/fileService.ts"; +import { NullLogService } from "../code-oss/src/vs/platform/log/common/log.ts"; + +// Keep the real FileService and real filesystem, adapting only the provider +// interface to avoid native Electron/watch dependencies in this CLI harness. +async function operation(action) { + try { + return await action(); + } catch (error) { + const codes = { + ENOENT: FileSystemProviderErrorCode.FileNotFound, + ENOTDIR: FileSystemProviderErrorCode.FileNotADirectory, + EEXIST: FileSystemProviderErrorCode.FileExists, + }; + + throw createFileSystemProviderError( + error, + codes[error.code] ?? FileSystemProviderErrorCode.Unknown, + ); + } +} + +const provider = { + capabilities: FileSystemProviderCapabilities.PathCaseSensitive, + onDidChangeCapabilities: Event.None, + onDidChangeFile: Event.None, + async stat(resource) { + const stat = await operation(() => fs.stat(resource.fsPath)); + + return { + type: stat.isDirectory() ? FileType.Directory : FileType.File, + ctime: stat.ctimeMs, + mtime: stat.mtimeMs, + size: stat.size, + }; + }, + mkdir: (resource) => operation(() => fs.mkdir(resource.fsPath)), + async readdir(resource) { + const entries = await operation(() => + fs.readdir(resource.fsPath, { withFileTypes: true }), + ); + + return entries.map((entry) => [ + entry.name, + entry.isDirectory() ? FileType.Directory : FileType.File, + ]); + }, +}; + +const root = await fs.mkdtemp(join(tmpdir(), "review-folder-collision-")); + +const service = new FileService(new NullLogService()); + +const registration = service.registerProvider("file", provider); + +let creates = 0; + +const listener = service.onDidRunOperation(() => creates++); + +try { + const target = join(root, "folder with spaces"); + const content = "Existing user file: keep these bytes.\n"; + await fs.writeFile(target, content); + await assert.rejects( + service.createFolder(URI.file(target)), + /already exists but is not a directory/, + ); + assert.equal(await fs.readFile(target, "utf8"), content); + assert.equal(creates, 0); + console.log( + "Reproduced the reported FileService collision; file bytes preserved.", + ); + + await assert.rejects(service.createFolder(URI.file(join(target, "child")))); + assert.equal(await fs.readFile(target, "utf8"), content); + assert.equal(creates, 0); + console.log( + "An ancestor file also prevents folder creation without losing data.", + ); + + // Simulate explicit user recovery only inside this disposable fixture. + const backup = join(root, "preserved-file"); + await fs.rename(target, backup); + const created = await service.createFolder(URI.file(join(target, "child"))); + assert.equal(created.isDirectory, true); + assert.equal(await fs.readFile(backup, "utf8"), content); + await service.createFolder(URI.file(join(target, "child"))); + assert.equal((await fs.stat(join(target, "child"))).isDirectory(), true); + console.log( + "Creation recovers after explicit fixture rename; existing directories are accepted.", + ); +} finally { + listener.dispose(); + registration.dispose(); + service.dispose(); + await fs.rm(root, { recursive: true, force: true }); +} diff --git a/docs/folder-collision-investigation.md b/docs/folder-collision-investigation.md new file mode 100644 index 000000000..1860f0239 --- /dev/null +++ b/docs/folder-collision-investigation.md @@ -0,0 +1,80 @@ +# First-run folder collision investigation + +Status: filesystem guard reproduced; production cause and implementation remain unresolved. + +PostHog issue `01a0d49f-9dc7-7db1-87aa-60419d73d04d` contains one reported +exception on September 24, 2026 in Whiteboard 0.1.1. The message says an existing +path is not a directory. Both the path and its category are unavailable in the +redacted event. No user paths or telemetry identities are retained here. + +The two bundle frames are `vs/review/review.desktop.main.js:1886:81871` and +`:1886:81605`. Comparing those positions against the installed 0.1.1 bundle +(commit `c14db218df246b305d1c2a391eb64435b902e83e`) identifies +`FileService.mkdirp` and `FileService.createFolder`. There is no calling feature +in the captured stack. The current source retains the same guard in +`apps/review-desktop/code-oss/src/vs/platform/files/common/fileService.ts`. + +## Timeline + +An analytics query of the same telemetry identity, restricted to 18:10–18:17 UTC, +returned this sequence. `review_client_error` and `$exception` are the paired +reports of the same exception, not two separate incidents. + +| UTC time | Event | +| --- | --- | +| 18:13:39.031 | `review_installation_created` | +| 18:13:39.071 | `review_client_error`, folder collision | +| 18:13:39.093 | `$exception`, same folder collision | +| 18:13:39.116 | `review_home_empty_state_viewed` | +| 18:13:40.278 | `review_app_ready` | +| 18:14:00.119 / .125 | command started / succeeded | +| 18:16:15.706 / .752 | command started / succeeded | + +This suggests a first-run startup operation. It does not establish that startup +failed: the ready event and successful commands follow. User-snippet initialization +is one startup caller of `createFolder`, but the evidence cannot distinguish it +from other callers or identify the colliding folder. Treating snippets, settings +import, or a cache as the proven cause would be speculative. + +## Reproduction + +From `apps/review-desktop` after installing workspace dependencies: + +```sh +TSX_TSCONFIG_PATH=tsconfig.test.json node --import tsx scripts/repro-folder-collision.mjs +``` + +The harness calls the actual `FileService.createFolder` implementation through a +small provider backed by Node filesystem operations. It uses a fresh temporary +directory and removes only that fixture afterward. It verifies that: + +- A regular file occupying the requested directory produces the reported error, + keeps its bytes, and emits no successful create operation. +- An ancestor that is a file also rejects creation and preserves the file. +- Explicitly renaming the fixture file permits creation without losing its bytes. +- Calling creation on an existing directory succeeds. + +All assertions passed on September 24, 2026 against base `cc43318e`, using an +isolated installation of tsx 4.21.0. This reproduces the low-level condition, not +the user's first-run sequence. No Electron build or end-to-end reproduction was +performed. This is a standalone investigation harness, not a test of a new fix. + +## Why there is no production patch yet + +Rejecting an existing regular file is correct and protects user data. Swallowing +the error, replacing the file, or choosing an arbitrary alternative directory +could break the feature or destroy data. Broad telemetry changes would not +resolve the unknown caller. Existing `createFolder` callers have different +recovery requirements, so a feature-specific fix needs the path category or a +caller stack first. + +The next useful evidence is the calling feature and a safe category for the +colliding path (profile snippets, logs, configuration cache, or another location), +obtained from a local reproduction/debugger or a user-approved diagnostic. +Only then should recovery be implemented at that caller. Do not auto-delete or +overwrite the colliding file. Error-tracking issue APIs were unavailable with the +current key; permitted analytics SQL provided the frames and timeline above. + +The workspace root and code-oss AGENTS.md instructions were read. The latter +references `.github/copilot-instructions.md`, which is absent from this checkout. +No README files were changed.