fix(plugins): keep Windows marketplace cache on copy ENOENT (#2220)

* fix(plugins): keep marketplace cache when Windows copy hits ENOENT (#2183)

ChromeDevTools marketplace add cloned successfully then failed finalize
because recursive fs.cp throws on unreadable nested files. Keep the
validated clone and join tilde plugin-cache paths so Windows does not
glue the home directory onto .openclaude.

* fix(plugins): keep marketplace cache if dest cleanup throws (#2183)

Copy-fallback recovery still failed add when removing a partial dest
threw, and getMarketplace refetch did not persist a keep-temp cachePath.

* fix(plugins): use persisted installLocation after marketplace refetch (#2183)

Keep-temp recovery can change cachePath on disk while same-call
getPluginById and install-by-name still returned the pre-refetch path.

* fix(plugins): persist keep-temp installLocation only for remote sources

getMarketplace refetch was rewriting file/directory installLocation to the
marketplace root, which can make later remove delete a user directory.
This commit is contained in:
JATMN
2026-09-12 08:22:43 +08:00
committed by GitHub
parent 055e5d551a
commit e2b021d8bb
6 changed files with 450 additions and 14 deletions
@@ -0,0 +1,75 @@
import { afterAll, afterEach, describe, expect, mock, test } from 'bun:test'
const staleInstallLocation = '/stale/marketplaces/mymarketplace'
const liveInstallLocation = '/live/marketplaces/temp_keep'
const realMarketplaceManager = await import(
`../../utils/plugins/marketplaceManager.js?spread=${Date.now()}`
)
const realInstallationHelpers = await import(
`../../utils/plugins/pluginInstallationHelpers.js?spread=${Date.now()}`
)
let configLoadCount = 0
let capturedInstallLocation: string | undefined
mock.module('../../utils/plugins/marketplaceManager.js', () => ({
...realMarketplaceManager,
getMarketplace: async () => ({
name: 'MyMarketplace',
owner: { name: 'test' },
plugins: [{ name: 'demo-plugin', source: './' }],
}),
getPluginById: async () => null,
loadKnownMarketplacesConfig: async () => {
configLoadCount++
const installLocation =
configLoadCount === 1 ? staleInstallLocation : liveInstallLocation
return {
MyMarketplace: {
source: { source: 'url', url: 'https://example.com/marketplace.json' },
installLocation,
lastUpdated: '2020-01-01T00:00:00.000Z',
},
}
},
}))
mock.module('../../utils/plugins/pluginInstallationHelpers.js', () => ({
...realInstallationHelpers,
installResolvedPlugin: async (opts: {
marketplaceInstallLocation?: string
}) => {
capturedInstallLocation = opts.marketplaceInstallLocation
return { ok: true, closure: [] as string[], depNote: '' }
},
}))
afterAll(() => {
mock.module(
'../../utils/plugins/marketplaceManager.js',
() => realMarketplaceManager,
)
mock.module(
'../../utils/plugins/pluginInstallationHelpers.js',
() => realInstallationHelpers,
)
})
const { installPluginOp } = await import(
`./pluginOperations.ts?bust=install-location-${Date.now()}`
)
describe('installPluginOp keep-temp installLocation (#2183)', () => {
afterEach(() => {
configLoadCount = 0
capturedInstallLocation = undefined
})
test('install-by-name uses persisted installLocation after getMarketplace refetch', async () => {
const result = await installPluginOp('demo-plugin', 'user')
expect(result.success).toBe(true)
expect(capturedInstallLocation).toBe(liveInstallLocation)
expect(configLoadCount).toBeGreaterThanOrEqual(2)
})
})
+3 -1
View File
@@ -348,7 +348,9 @@ export async function installPluginOp(
if (pluginEntry) {
foundPlugin = pluginEntry
foundMarketplace = mktName
marketplaceInstallLocation = mktConfig.installLocation
const refreshed = await loadKnownMarketplacesConfig()
marketplaceInstallLocation =
refreshed[mktName]?.installLocation ?? mktConfig.installLocation
break
}
} catch (error) {
@@ -0,0 +1,43 @@
import { afterEach, describe, expect, test } from 'bun:test'
import { homedir } from 'os'
import { join } from 'path'
import { expandTilde } from './pathValidation.js'
describe('expandTilde', () => {
const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform')
afterEach(() => {
if (originalPlatform) {
Object.defineProperty(process, 'platform', originalPlatform)
}
})
test('expands ~/ using path.join so home and .openclaude stay separate', () => {
const expanded = expandTilde('~/.openclaude/plugins')
expect(expanded).toBe(join(homedir(), '.openclaude', 'plugins'))
expect(expanded).not.toMatch(/[^/\\]\.openclaude/)
})
test('expands a bare tilde to the home directory', () => {
expect(expandTilde('~')).toBe(homedir())
})
test('leaves non-tilde paths unchanged', () => {
expect(expandTilde('C:\\Users\\GarryLai\\.openclaude\\plugins')).toBe(
'C:\\Users\\GarryLai\\.openclaude\\plugins',
)
expect(expandTilde('/tmp/plugins')).toBe('/tmp/plugins')
})
test('expands ~\\ on win32 rather than leaving the tilde literal', () => {
Object.defineProperty(process, 'platform', {
value: 'win32',
configurable: true,
})
const expanded = expandTilde('~\\.openclaude\\plugins')
expect(expanded.startsWith(homedir())).toBe(true)
expect(expanded).not.toBe('~\\.openclaude\\plugins')
expect(expanded.includes('.openclaude')).toBe(true)
expect(expanded).not.toMatch(/[^/\\]\.openclaude/)
})
})
+14 -3
View File
@@ -1,6 +1,6 @@
import memoize from 'lodash-es/memoize.js'
import { homedir } from 'os'
import { dirname, isAbsolute, resolve } from 'path'
import { dirname, isAbsolute, join, resolve } from 'path'
import type { ToolPermissionContext } from '../../Tool.js'
import { getPlatform } from '../../utils/platform.js'
import {
@@ -76,14 +76,25 @@ export function getGlobBaseDirectory(path: string): string {
/**
* Expands tilde (~) at the start of a path to the user's home directory.
* Note: ~username expansion is not supported for security reasons.
*
* Uses path.join rather than string concatenation. Concatenating
* `homedir() + path.slice(1)` keeps the leading `/` from `~/...`, which
* happens to work on POSIX (`/home/user` + `/.openclaude`) but on Windows
* produces mixed separators (`C:\Users\Name/.openclaude`) and, for any
* helper that strips `~/` including the slash, glues the names together
* (`C:\Users\Name.openclaude`). Plugin cache overrides such as
* CLAUDE_CODE_PLUGIN_CACHE_DIR=`~/.openclaude/plugins` go through this
* helper (issue #2183).
*/
export function expandTilde(path: string): string {
if (path === '~') {
return homedir()
}
if (
path === '~' ||
path.startsWith('~/') ||
(process.platform === 'win32' && path.startsWith('~\\'))
) {
return homedir() + path.slice(1)
return join(homedir(), path.slice(2))
}
return path
}
+265 -2
View File
@@ -10,9 +10,10 @@ import {
test,
} from 'bun:test'
import * as realAxios from 'axios'
import { existsSync, mkdirSync, mkdtempSync, readdirSync, rmSync } from 'fs'
import { existsSync, mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'fs'
import { tmpdir } from 'os'
import { join } from 'path'
import * as realExecFileNoThrow from '../execFileNoThrow.js'
import {
getFsImplementation,
NodeFsOperations,
@@ -249,6 +250,11 @@ describe('loadAndCacheMarketplace — Windows cache finalization (#1500)', () =>
*/
describe('loadAndCacheMarketplace — rename failure fallback (EXDEV)', () => {
let loadAndCacheWithMockedAxios: typeof loadAndCacheMarketplace
let getMarketplaceWithMockedAxios: typeof import('./marketplaceManager.js').getMarketplace
let getPluginByIdWithMockedAxios: typeof import('./marketplaceManager.js').getPluginById
let saveKnownMarketplacesConfigWithMockedAxios: typeof import('./marketplaceManager.js').saveKnownMarketplacesConfig
let loadKnownMarketplacesConfigWithMockedAxios: typeof import('./marketplaceManager.js').loadKnownMarketplacesConfig
let clearMarketplacesCacheWithMockedAxios: typeof import('./marketplaceManager.js').clearMarketplacesCache
let tempDir: string
let originalFs: FsOperations
let originalCacheDir: string | undefined
@@ -262,7 +268,7 @@ describe('loadAndCacheMarketplace — rename failure fallback (EXDEV)', () => {
const fakeMarketplaceJson = {
name: 'MyMarketplace',
owner: { name: 'test' },
plugins: [],
plugins: [{ name: 'demo-plugin', source: './' }],
}
const axiosGetSpy = mock(async () => ({
data: fakeMarketplaceJson,
@@ -277,6 +283,31 @@ describe('loadAndCacheMarketplace — rename failure fallback (EXDEV)', () => {
},
isAxiosError: () => false,
}))
mock.module('../execFileNoThrow.js', () => ({
...realExecFileNoThrow,
execFileNoThrow: async () => ({
code: 255,
stdout: '',
stderr: 'Permission denied',
}),
execFileNoThrowWithCwd: async (
_file: string,
args: string[] = [],
) => {
if (args.includes('clone')) {
const targetPath = args.at(-1)
if (targetPath) {
mkdirSync(join(targetPath, '.claude-plugin'), { recursive: true })
writeFileSync(
join(targetPath, '.claude-plugin', 'marketplace.json'),
JSON.stringify(fakeMarketplaceJson),
)
}
return { code: 0, stdout: '', stderr: '' }
}
return { code: 1, stdout: '', stderr: 'not a git repository' }
},
}))
// Re-import with mocked axios so the module under test picks up the mock.
// The `?bust=` suffix is the same trick the dynamic import at the top of
@@ -294,10 +325,16 @@ describe('loadAndCacheMarketplace — rename failure fallback (EXDEV)', () => {
`./marketplaceManager.ts?bust=exdev-test-reimport-${Date.now()}`
)) as typeof import('./marketplaceManager.js')
loadAndCacheWithMockedAxios = mod._test.loadAndCacheMarketplace
getMarketplaceWithMockedAxios = mod.getMarketplace
getPluginByIdWithMockedAxios = mod.getPluginById
saveKnownMarketplacesConfigWithMockedAxios = mod.saveKnownMarketplacesConfig
loadKnownMarketplacesConfigWithMockedAxios = mod.loadKnownMarketplacesConfig
clearMarketplacesCacheWithMockedAxios = mod.clearMarketplacesCache
})
afterAll(() => {
mock.module('axios', () => realAxios)
mock.module('../execFileNoThrow.js', () => realExecFileNoThrow)
})
beforeEach(() => {
@@ -333,6 +370,7 @@ describe('loadAndCacheMarketplace — rename failure fallback (EXDEV)', () => {
})
afterEach(() => {
clearMarketplacesCacheWithMockedAxios?.()
setFsImplementation(originalFs)
if (originalCacheDir === undefined) {
delete process.env.CLAUDE_CODE_PLUGIN_CACHE_DIR
@@ -416,6 +454,231 @@ describe('loadAndCacheMarketplace — rename failure fallback (EXDEV)', () => {
const newEntries = afterEntries.filter(e => !beforeEntries.has(e))
expect(newEntries).toEqual(['mymarketplace'])
})
test('keeps the temp cache when copy fallback throws ENOENT (issue #2183)', async () => {
renameSpy.mockImplementation(() => {
throw new Error('EXDEV: cross-device link not permitted, rename')
})
cpSpy.mockImplementation(async () => {
throw Object.assign(
new Error(
"ENOENT: no such file or directory, copyfile 'chromedevtools-chrome-devtools-mcp/third_party/devtools-frontend/build/android/gyp/binary_baseline_profile.pydeps' -> 'chrome-devtools-plugins/third_party/devtools-frontend/build/android/gyp/binary_baseline_profile.pydeps'",
),
{ code: 'ENOENT' },
)
})
const source: MarketplaceSource = {
source: 'url',
url: 'https://example.com/marketplace.json',
}
const cacheDir = join(tempDir, 'marketplaces')
mkdirSync(cacheDir, { recursive: true })
const result = await loadAndCacheWithMockedAxios!(source)
expect(result.marketplace.name).toBe('MyMarketplace')
expect(result.cachePath.startsWith(join(cacheDir, 'temp_'))).toBe(true)
expect(existsSync(result.cachePath)).toBe(true)
expect(existsSync(join(cacheDir, 'mymarketplace'))).toBe(false)
expect(cpSpy).toHaveBeenCalled()
})
test('keeps the temp cache when dest cleanup after copy ENOENT throws', async () => {
renameSpy.mockImplementation(() => {
throw new Error('EXDEV: cross-device link not permitted, rename')
})
let copyAttempted = false
cpSpy.mockImplementation(async () => {
copyAttempted = true
throw Object.assign(
new Error(
"ENOENT: no such file or directory, copyfile 'chromedevtools-chrome-devtools-mcp/third_party/devtools-frontend/build/android/gyp/binary_baseline_profile.pydeps' -> 'chrome-devtools-plugins/third_party/devtools-frontend/build/android/gyp/binary_baseline_profile.pydeps'",
),
{ code: 'ENOENT' },
)
})
const cacheDir = join(tempDir, 'marketplaces')
mkdirSync(cacheDir, { recursive: true })
const finalCachePath = join(cacheDir, 'mymarketplace')
setFsImplementation({
...NodeFsOperations,
rm: async (path, options?: { recursive?: boolean; force?: boolean }) => {
rmCallCount++
if (copyAttempted && path === finalCachePath) {
throw Object.assign(new Error('EPERM: operation not permitted'), {
code: 'EPERM',
})
}
rmSync(path, options ?? { recursive: true, force: true })
},
rename: renameSpy,
cp: cpSpy,
})
const source: MarketplaceSource = {
source: 'url',
url: 'https://example.com/marketplace.json',
}
const result = await loadAndCacheWithMockedAxios!(source)
expect(result.marketplace.name).toBe('MyMarketplace')
expect(result.cachePath.startsWith(join(cacheDir, 'temp_'))).toBe(true)
expect(existsSync(result.cachePath)).toBe(true)
expect(cpSpy).toHaveBeenCalled()
})
test('getMarketplace refetch persists keep-temp cachePath as installLocation', async () => {
renameSpy.mockImplementation(() => {
throw new Error('EXDEV: cross-device link not permitted, rename')
})
cpSpy.mockImplementation(async () => {
throw Object.assign(
new Error(
"ENOENT: no such file or directory, copyfile 'chromedevtools-chrome-devtools-mcp/third_party/devtools-frontend/build/android/gyp/binary_baseline_profile.pydeps' -> 'chrome-devtools-plugins/third_party/devtools-frontend/build/android/gyp/binary_baseline_profile.pydeps'",
),
{ code: 'ENOENT' },
)
})
const cacheDir = join(tempDir, 'marketplaces')
mkdirSync(cacheDir, { recursive: true })
const staleCanonicalPath = join(cacheDir, 'mymarketplace')
await saveKnownMarketplacesConfigWithMockedAxios!({
MyMarketplace: {
source: {
source: 'url',
url: 'https://example.com/marketplace.json',
},
installLocation: staleCanonicalPath,
lastUpdated: '2020-01-01T00:00:00.000Z',
},
})
clearMarketplacesCacheWithMockedAxios!()
const marketplace = await getMarketplaceWithMockedAxios!('MyMarketplace')
expect(marketplace.name).toBe('MyMarketplace')
const config = await loadKnownMarketplacesConfigWithMockedAxios!()
const persisted = config.MyMarketplace?.installLocation
expect(persisted?.startsWith(join(cacheDir, 'temp_'))).toBe(true)
expect(existsSync(persisted!)).toBe(true)
expect(persisted).not.toBe(staleCanonicalPath)
})
test('getPluginById returns persisted keep-temp installLocation after refetch', async () => {
renameSpy.mockImplementation(() => {
throw new Error('EXDEV: cross-device link not permitted, rename')
})
cpSpy.mockImplementation(async () => {
throw Object.assign(
new Error(
"ENOENT: no such file or directory, copyfile 'chromedevtools-chrome-devtools-mcp/third_party/devtools-frontend/build/android/gyp/binary_baseline_profile.pydeps' -> 'chrome-devtools-plugins/third_party/devtools-frontend/build/android/gyp/binary_baseline_profile.pydeps'",
),
{ code: 'ENOENT' },
)
})
const cacheDir = join(tempDir, 'marketplaces')
mkdirSync(cacheDir, { recursive: true })
const staleCanonicalPath = join(cacheDir, 'mymarketplace')
await saveKnownMarketplacesConfigWithMockedAxios!({
MyMarketplace: {
source: {
source: 'url',
url: 'https://example.com/marketplace.json',
},
installLocation: staleCanonicalPath,
lastUpdated: '2020-01-01T00:00:00.000Z',
},
})
clearMarketplacesCacheWithMockedAxios!()
const pluginInfo = await getPluginByIdWithMockedAxios!(
'demo-plugin@MyMarketplace',
)
expect(pluginInfo).not.toBeNull()
expect(pluginInfo!.entry.name).toBe('demo-plugin')
expect(
pluginInfo!.marketplaceInstallLocation.startsWith(
join(cacheDir, 'temp_'),
),
).toBe(true)
expect(pluginInfo!.marketplaceInstallLocation).not.toBe(staleCanonicalPath)
})
test('keeps the GitHub clone directory when copy fallback throws ENOENT', async () => {
renameSpy.mockImplementation(() => {
throw new Error('EXDEV: cross-device link not permitted, rename')
})
cpSpy.mockImplementation(async () => {
throw Object.assign(
new Error(
"ENOENT: no such file or directory, copyfile 'chromedevtools-chrome-devtools-mcp/third_party/devtools-frontend/build/android/gyp/binary_baseline_profile.pydeps' -> 'chrome-devtools-plugins/third_party/devtools-frontend/build/android/gyp/binary_baseline_profile.pydeps'",
),
{ code: 'ENOENT' },
)
})
const source: MarketplaceSource = {
source: 'github',
repo: 'ChromeDevTools/chrome-devtools-mcp',
}
const cacheDir = join(tempDir, 'marketplaces')
mkdirSync(cacheDir, { recursive: true })
const result = await loadAndCacheWithMockedAxios!(source)
const cloneDir = join(cacheDir, 'chromedevtools-chrome-devtools-mcp')
expect(result.marketplace.name).toBe('MyMarketplace')
expect(result.cachePath).toBe(cloneDir)
expect(
existsSync(join(cloneDir, '.claude-plugin', 'marketplace.json')),
).toBe(true)
expect(existsSync(join(cacheDir, 'mymarketplace'))).toBe(false)
expect(cpSpy).toHaveBeenCalled()
})
test('getMarketplace refetch does not rewrite local file installLocation', async () => {
const localRoot = join(tempDir, 'local-marketplace')
mkdirSync(join(localRoot, '.claude-plugin'), { recursive: true })
writeFileSync(
join(localRoot, '.claude-plugin', 'marketplace.json'),
JSON.stringify(fakeMarketplaceJson),
)
const storedManifestPath = join(
localRoot,
'.claude-plugin',
'marketplace.json',
)
const missingCachePath = join(tempDir, 'missing-installLocation.json')
await saveKnownMarketplacesConfigWithMockedAxios!({
MyMarketplace: {
source: {
source: 'file',
path: storedManifestPath,
},
installLocation: missingCachePath,
lastUpdated: '2020-01-01T00:00:00.000Z',
},
})
clearMarketplacesCacheWithMockedAxios!()
const marketplace = await getMarketplaceWithMockedAxios!('MyMarketplace')
expect(marketplace.name).toBe('MyMarketplace')
const config = await loadKnownMarketplacesConfigWithMockedAxios!()
expect(config.MyMarketplace?.installLocation).toBe(missingCachePath)
expect(config.MyMarketplace?.installLocation).not.toBe(localRoot)
})
})
/**
+50 -8
View File
@@ -1831,14 +1831,37 @@ async function loadAndCacheMarketplace(
// Rename temp cache to final name
try {
await fs.rename(temporaryCachePath, finalCachePath)
temporaryCachePath = finalCachePath
} catch (renameError) {
// Rename may fail for cross-device moves (EXDEV). Fall back to
// copy + delete.
await fs.cp(temporaryCachePath, finalCachePath, { recursive: true })
await fs.rm(temporaryCachePath, { recursive: true, force: true })
// copy + delete. Recursive copy can still throw ENOENT on Windows
// for dangling symlinks or unreadable nested files in large
// clones (ChromeDevTools/chrome-devtools-mcp third_party trees,
// issue #2183). The clone already succeeded and marketplace.json
// was parsed from temporaryCachePath — keep that directory rather
// than failing the add.
try {
await fs.cp(temporaryCachePath, finalCachePath, {
recursive: true,
})
await fs.rm(temporaryCachePath, { recursive: true, force: true })
temporaryCachePath = finalCachePath
} catch (copyError) {
try {
await fs.rm(finalCachePath, { recursive: true, force: true })
} catch (cleanupError) {
logForDebugging(
`Failed to remove partial marketplace cache at ${finalCachePath}: ${errorMessage(cleanupError)}`,
{ level: 'warn' },
)
}
logForDebugging(
`Marketplace cache rename and copy failed; keeping ${temporaryCachePath}. rename=${errorMessage(renameError)} copy=${errorMessage(copyError)}`,
{ level: 'warn' },
)
}
}
temporaryCachePath = finalCachePath
cleanupNeeded = false // Successfully renamed, no cleanup needed
cleanupNeeded = false // Clone is the live cache (renamed, copied, or kept)
} catch (error) {
const errorMsg = errorMessage(error)
throw new Error(
@@ -2271,16 +2294,28 @@ export const getMarketplace = memoize(
// Cache doesn't exist or is invalid, fetch from source
let marketplace: PluginMarketplace
let cachePath: string
try {
;({ marketplace } = await loadAndCacheMarketplace(entry.source))
;({ marketplace, cachePath } = await loadAndCacheMarketplace(
entry.source,
))
} catch (error) {
throw new Error(
`Failed to load marketplace "${name}" from source (${entry.source.source}): ${errorMessage(error)}`,
)
}
// Update lastUpdated only when we actually fetch
// Persist cachePath for remote sources: keep-temp recovery (#2183) may
// leave the live clone at temporaryCachePath instead of the canonical
// marketplace-name directory already stored in installLocation.
// Local file/directory sources do not go through rename/copy; cachePath is
// the user's marketplace root. Overwriting a stored manifest path with that
// root would make removeMarketplaceSource recursively delete the user's
// directory.
config[name]!.lastUpdated = new Date().toISOString()
if (!isLocalMarketplaceSource(entry.source)) {
config[name]!.installLocation = cachePath
}
await saveKnownMarketplacesConfig(config)
return marketplace
@@ -2376,9 +2411,16 @@ export async function getPluginById(pluginId: string): Promise<{
return null
}
// getMarketplace refetch may persist a keep-temp cachePath (#2183) that
// differs from the snapshot loaded above.
const refreshedConfig = await loadKnownMarketplacesConfig()
const installLocation =
refreshedConfig[marketplaceName]?.installLocation ??
marketplaceConfig.installLocation
return {
entry: plugin,
marketplaceInstallLocation: marketplaceConfig.installLocation,
marketplaceInstallLocation: installLocation,
}
} catch (error) {
logForDebugging(