-
Notifications
You must be signed in to change notification settings - Fork 26.2k
fix(windows): keep project paths stable across aliases #18709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
fabbaef
c3a8420
f69c7cb
687e738
a4e53b1
1b44f4c
fdbcf91
4bb7659
20ba5b9
2270ad4
82d4bd7
42da404
9a8dfd8
12a113e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { getAdaptor } from "./adaptors" | |
| import { WorkspaceInfo } from "./types" | ||
| import { WorkspaceID } from "./schema" | ||
| import { parseSSE } from "./sse" | ||
| import { Path } from "@/path/path" | ||
|
|
||
| export namespace Workspace { | ||
| export const Event = { | ||
|
|
@@ -40,7 +41,7 @@ export namespace Workspace { | |
| type: row.type, | ||
| branch: row.branch, | ||
| name: row.name, | ||
| directory: row.directory, | ||
| directory: row.directory ? Path.stored(row.directory) : null, | ||
| extra: row.extra, | ||
| projectID: row.project_id, | ||
| } | ||
|
|
@@ -65,7 +66,7 @@ export namespace Workspace { | |
| type: config.type, | ||
| branch: config.branch ?? null, | ||
| name: config.name ?? null, | ||
| directory: config.directory ?? null, | ||
| directory: config.directory ? Path.stored(config.directory) : null, | ||
| extra: config.extra ?? null, | ||
|
Comment on lines
68
to
70
|
||
| projectID: input.projectID, | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import { Filesystem } from "@/util/filesystem" | ||
|
|
||
| /** | ||
| * Any legal path text we ingest from the outside world. | ||
| * | ||
| * You might see: | ||
| * - `C:\Users\RUNNER~1\repo` | ||
| * - `C:/Users/runneradmin/repo` | ||
| * - `/c/Users/runneradmin/repo` | ||
| * - `/cygdrive/c/Users/runneradmin/repo` | ||
| * | ||
| * You should not assume: | ||
| * - native separators | ||
| * - canonical casing | ||
| * - long Windows names | ||
| * - symlinks resolved | ||
| * - safe to persist directly | ||
| */ | ||
| export type RawPath = string & { readonly __raw: unique symbol } | ||
|
|
||
| /** | ||
| * Canonical path value we keep in storage and long-lived runtime state. | ||
| * | ||
| * You might see: | ||
| * - `C:\Users\runneradmin\repo` | ||
| * - `/Users/luke/repo` | ||
| * | ||
| * You should not see: | ||
| * - `RUNNER~1` | ||
| * - `/cygdrive/c/...` | ||
| * - slash-only key forms used just for comparison | ||
| */ | ||
| export type StoredPath = string & { readonly __stored: unique symbol } | ||
|
|
||
| export namespace Path { | ||
| export function stored(input: RawPath | string): StoredPath { | ||
| if (!input || input === "/") return input as StoredPath | ||
| return Filesystem.resolve(input) as StoredPath | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { disposeInstance } from "@/effect/instance-registry" | |
| import { Filesystem } from "@/util/filesystem" | ||
| import { iife } from "@/util/iife" | ||
| import { Log } from "@/util/log" | ||
| import { Path, type StoredPath } from "@/path/path" | ||
| import { Context } from "../util/context" | ||
| import { Project } from "./project" | ||
| import { State } from "./state" | ||
|
|
@@ -13,7 +14,7 @@ export interface Shape { | |
| project: Project.Info | ||
| } | ||
| const context = Context.create<Shape>("instance") | ||
| const cache = new Map<string, Promise<Shape>>() | ||
| const cache = new Map<StoredPath, Promise<Shape>>() | ||
|
|
||
| const disposal = { | ||
| all: undefined as Promise<void> | undefined, | ||
|
|
@@ -52,18 +53,18 @@ function boot(input: { directory: string; init?: () => Promise<any>; project?: P | |
| }) | ||
| } | ||
|
|
||
| function track(directory: string, next: Promise<Shape>) { | ||
| function track(dir: StoredPath, next: Promise<Shape>) { | ||
| const task = next.catch((error) => { | ||
| if (cache.get(directory) === task) cache.delete(directory) | ||
| if (cache.get(dir) === task) cache.delete(dir) | ||
| throw error | ||
| }) | ||
| cache.set(directory, task) | ||
| cache.set(dir, task) | ||
| return task | ||
| } | ||
|
|
||
| export const Instance = { | ||
| async provide<R>(input: { directory: string; init?: () => Promise<any>; fn: () => R }): Promise<R> { | ||
| const directory = Filesystem.resolve(input.directory) | ||
| const directory = Path.stored(input.directory) | ||
| let existing = cache.get(directory) | ||
| if (!existing) { | ||
| Log.Default.info("creating instance", { directory }) | ||
|
|
@@ -117,7 +118,7 @@ export const Instance = { | |
| return State.create(() => Instance.directory, init, dispose) | ||
| }, | ||
| async reload(input: { directory: string; init?: () => Promise<any>; project?: Project.Info; worktree?: string }) { | ||
| const directory = Filesystem.resolve(input.directory) | ||
| const directory = Path.stored(input.directory) | ||
| Log.Default.info("reloading instance", { directory }) | ||
| await Promise.all([State.dispose(directory), disposeInstance(directory)]) | ||
| cache.delete(directory) | ||
|
|
@@ -129,7 +130,7 @@ export const Instance = { | |
| const directory = Instance.directory | ||
| Log.Default.info("disposing instance", { directory }) | ||
| await Promise.all([State.dispose(directory), disposeInstance(directory)]) | ||
| cache.delete(directory) | ||
| cache.delete(directory as StoredPath) | ||
| emit(directory) | ||
|
Comment on lines
131
to
134
|
||
| }, | ||
| async disposeAll() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { describe, expect, test } from "bun:test" | ||
| import fs from "fs/promises" | ||
| import path from "path" | ||
| import { Path } from "../../src/path/path" | ||
| import { tmpdir } from "../fixture/fixture" | ||
|
|
||
| describe("path", () => { | ||
| test("keeps sentinel storage paths unchanged", () => { | ||
| expect(String(Path.stored(""))).toBe("") | ||
| expect(String(Path.stored("/"))).toBe("/") | ||
| }) | ||
|
|
||
| test("resolves Windows alias roots for stored paths", async () => { | ||
| if (process.platform !== "win32") return | ||
| await using tmp = await tmpdir() | ||
|
|
||
| const real = path.join(tmp.path, "Target") | ||
| await fs.mkdir(real, { recursive: true }) | ||
| const alias = path.join(tmp.path, "Alias") | ||
| await fs.symlink(real, alias, "junction") | ||
|
|
||
| expect(Path.stored(alias)).toBe(Path.stored(real)) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ import { describe, expect, mock, test } from "bun:test" | |||||||||||||||||||||||||||
| import { Project } from "../../src/project/project" | ||||||||||||||||||||||||||||
| import { Log } from "../../src/util/log" | ||||||||||||||||||||||||||||
| import { $ } from "bun" | ||||||||||||||||||||||||||||
| import fs from "fs/promises" | ||||||||||||||||||||||||||||
| import path from "path" | ||||||||||||||||||||||||||||
| import { tmpdir } from "../fixture/fixture" | ||||||||||||||||||||||||||||
| import { Filesystem } from "../../src/util/filesystem" | ||||||||||||||||||||||||||||
|
|
@@ -100,6 +101,20 @@ describe("Project.fromDirectory", () => { | |||||||||||||||||||||||||||
| expect(fileExists).toBe(true) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| test("canonicalizes Windows alias roots before persisting", async () => { | ||||||||||||||||||||||||||||
| if (process.platform !== "win32") return | ||||||||||||||||||||||||||||
| const p = await loadProject() | ||||||||||||||||||||||||||||
| await using tmp = await tmpdir({ git: true }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const alias = path.join(path.dirname(tmp.path), path.basename(tmp.path) + "-alias") | ||||||||||||||||||||||||||||
| await fs.symlink(tmp.path, alias, "junction") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { project, sandbox } = await p.fromDirectory(alias) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| expect(String(project.worktree)).toBe(tmp.path) | ||||||||||||||||||||||||||||
| expect(String(sandbox)).toBe(tmp.path) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
| const { project, sandbox } = await p.fromDirectory(alias) | |
| expect(String(project.worktree)).toBe(tmp.path) | |
| expect(String(sandbox)).toBe(tmp.path) | |
| try { | |
| const { project, sandbox } = await p.fromDirectory(alias) | |
| expect(String(project.worktree)).toBe(tmp.path) | |
| expect(String(sandbox)).toBe(tmp.path) | |
| } finally { | |
| // Ensure the alias junction is removed even if the test fails. | |
| await fs.unlink(alias).catch(() => {}) | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
row.directory ? ... : nulltreats an empty-string directory asnull, which changes semantics compared to the previous code and also bypassesPath.stored("")'s sentinel behavior. Prefer a nullish check (e.g.,row.directory === null ? null : Path.stored(row.directory)) so""stays""whilenullstaysnull.