From 9fd4dedcae816fb980032213e5df97f1e9bb761f Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 25 Jun 2026 23:17:02 -0700 Subject: [PATCH] fix(review): resolve PR feedback on resolveGlobalDir guard - Avoid the legacy-migration side effect on the hot path: run the cheap, read-only checks (basename === ".fusion" && parent has .git) FIRST, and only call resolveGlobalDirForHome() (which can perform a one-time rename) when a dir actually looks project-local. - Normalize paths before the home-dir comparison (realpathSync when present, else resolve) so a trailing slash, doubled separator, or symlinked home doesn't make the legitimate home global dir trip the guard. - Harden the browse-directory regression test: the mock returns a global dir DISTINCT from getFusionDir() and asserts CentralCore is constructed with the global dir, so a revert to getFusionDir() now fails the test. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/src/global-settings.ts | 40 ++++++++++++------- .../__tests__/browse-directory-routes.test.ts | 7 +++- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/packages/core/src/global-settings.ts b/packages/core/src/global-settings.ts index e4187e09e1..3054849753 100644 --- a/packages/core/src/global-settings.ts +++ b/packages/core/src/global-settings.ts @@ -14,9 +14,9 @@ */ import { homedir } from "node:os"; -import { basename, dirname, join } from "node:path"; +import { basename, dirname, join, resolve } from "node:path"; import { mkdir, readFile, writeFile, rename, chmod } from "node:fs/promises"; -import { existsSync, mkdirSync, renameSync } from "node:fs"; +import { existsSync, mkdirSync, realpathSync, renameSync } from "node:fs"; import type { GlobalSettings } from "./types.js"; import { DEFAULT_GLOBAL_SETTINGS } from "./types.js"; import { sanitizeCliAgentsSettings } from "./settings-schema.js"; @@ -98,20 +98,32 @@ export function resolveGlobalDir(dir?: string): string { FNXC:GlobalDirGuard 2026-06-25-22:55: The heuristic is intentionally conservative but can't perfectly distinguish a project `.fusion` from a legitimately version-controlled custom global dir (e.g. a dotfiles repo with `~/dotfiles/.fusion` + `.git`). To avoid hard-crashing that rare setup, honor an explicit opt-out env var `FUSION_ALLOW_PROJECT_LOCAL_GLOBAL_DIR=true`. This is not reachable via normal production call sites (they resolve to ~/.fusion); it only matters for operators who deliberately configure a custom global dir inside a repo. + + FNXC:GlobalDirGuard 2026-06-26-06:25: + Order matters and the home-dir comparison must be normalized: + - Run the CHEAP, read-only checks first (basename is `.fusion` AND its parent contains a `.git`). Only if both hold do we call `resolveGlobalDirForHome()` — which can perform a one-time legacy-dir rename — so we never trigger that filesystem side effect on the hot path (every explicit-dir call, e.g. getGlobalSettingsDir() in dashboard routes, previously hit it). + - Compare against the home global dir using normalized real paths (realpathSync when the path exists, else resolve()), so a trailing slash, doubled separator, or symlinked home dir doesn't make the legitimate home global dir look like a foreign project dir and trip the guard. */ if (process.env.VITEST !== "true" && process.env.FUSION_ALLOW_PROJECT_LOCAL_GLOBAL_DIR !== "true") { - const homeGlobalDir = resolveGlobalDirForHome(getHomeDir()); - const looksLikeProjectFusionDir = - dir !== homeGlobalDir && - basename(dir) === ".fusion" && - existsSync(join(dirname(dir), ".git")); - if (looksLikeProjectFusionDir) { - throw new Error( - `resolveGlobalDir(): refusing project-local '.fusion' directory '${dir}' for the central/global store. ` + - "This would create a stray per-project central database seeded with default global settings and silently reset them. " + - "Pass the resolved global dir (or omit the argument so it defaults to ~/.fusion); see TaskStore.getGlobalSettingsDir(). " + - "If this really is your intended global dir (e.g. a version-controlled dotfiles repo), set FUSION_ALLOW_PROJECT_LOCAL_GLOBAL_DIR=true to override.", - ); + const isFusionDirInsideRepo = + basename(dir) === ".fusion" && existsSync(join(dirname(dir), ".git")); + if (isFusionDirInsideRepo) { + const homeGlobalDir = resolveGlobalDirForHome(getHomeDir()); + const normalize = (p: string): string => { + try { + return realpathSync.native(p); + } catch { + return resolve(p); + } + }; + if (normalize(dir) !== normalize(homeGlobalDir)) { + throw new Error( + `resolveGlobalDir(): refusing project-local '.fusion' directory '${dir}' for the central/global store. ` + + "This would create a stray per-project central database seeded with default global settings and silently reset them. " + + "Pass the resolved global dir (or omit the argument so it defaults to ~/.fusion); see TaskStore.getGlobalSettingsDir(). " + + "If this really is your intended global dir (e.g. a version-controlled dotfiles repo), set FUSION_ALLOW_PROJECT_LOCAL_GLOBAL_DIR=true to override.", + ); + } } } return dir; diff --git a/packages/dashboard/src/__tests__/browse-directory-routes.test.ts b/packages/dashboard/src/__tests__/browse-directory-routes.test.ts index bc05399869..1cccd30a46 100644 --- a/packages/dashboard/src/__tests__/browse-directory-routes.test.ts +++ b/packages/dashboard/src/__tests__/browse-directory-routes.test.ts @@ -40,6 +40,7 @@ vi.mock("@fusion/core", async () => { // Import after mocking import { browseDirectory } from "../../app/api.js"; +import { CentralCore } from "@fusion/core"; function mockFetchResponse( ok: boolean, @@ -71,9 +72,9 @@ class MockStoreForRoutes extends EventEmitter { return "/tmp/fn-944/.fusion"; } - // FNXC:GlobalDirGuard 2026-06-25-23:10: Routes resolve the global central dir via getGlobalSettingsDir(); mock mirrors getFusionDir() (CentralCore is mocked) so route behavior matches pre-change. + // FNXC:GlobalDirGuard 2026-06-26-06:25: Return a global dir DISTINCT from getFusionDir() so the regression test can assert the route constructs CentralCore with the global dir — a future revert to getFusionDir() then fails the CentralCore-constructor assertion below instead of silently passing. getGlobalSettingsDir(): string { - return this.getFusionDir(); + return "/tmp/fn-944/.fusion-global"; } getDatabase() { @@ -154,6 +155,8 @@ describe("GET /api/browse-directory route handler", () => { expect(mockListNodes).toHaveBeenCalled(); expect(mockClose).toHaveBeenCalled(); expect(globalThis.fetch).not.toHaveBeenCalled(); + // FNXC:GlobalDirGuard 2026-06-26-06:25: The node-aware route must build CentralCore from the GLOBAL dir, never the project `.fusion/`. Asserting the distinct global path makes a revert to getFusionDir() ("/tmp/fn-944/.fusion") fail here. + expect(vi.mocked(CentralCore)).toHaveBeenCalledWith("/tmp/fn-944/.fusion-global"); }); });