mirror of
https://github.com/NanmiCoder/claude-code-haha.git
synced 2026-10-10 11:53:10 +08:00
fix(connectors): stop stale enabledPlugins tombstones from failing skill installs
A first-install failure wrote enabled=false into settings for a plugin that was never published to a marketplace. Every later plugin load then reported plugin-not-found for that inert entry, and the session reload at the end of any connector prepare treated those foreign errors as a failure, so all skill connectors died in the installing-plugin phase. Align with upstream semantics (verified against codex-rs and the ChatGPT desktop app): never write a disabled tombstone for an unpublished plugin, treat an explicitly disabled missing entry as inert instead of a load error, and stop letting unrelated plugin errors veto a connector reload.
This commit is contained in:
@@ -179,6 +179,31 @@ describe('Plugins API', () => {
|
||||
expect(typeof body.summary.errors).toBe('number')
|
||||
})
|
||||
|
||||
it('a disabled settings entry for a missing plugin is inert, not a load error', async () => {
|
||||
const marketplaceRoot = path.join(tmpDir, 'marketplace-root')
|
||||
const pluginsDir = path.join(tmpDir, 'plugins')
|
||||
const marketplaceFile = path.join(marketplaceRoot, '.claude-plugin', 'marketplace.json')
|
||||
await fs.mkdir(path.dirname(marketplaceFile), { recursive: true })
|
||||
await fs.mkdir(pluginsDir, { recursive: true })
|
||||
await fs.writeFile(marketplaceFile, JSON.stringify({ name: 'test-market', owner: { name: 'Test' }, plugins: [] }), 'utf-8')
|
||||
await fs.writeFile(
|
||||
path.join(pluginsDir, 'known_marketplaces.json'),
|
||||
JSON.stringify({ 'test-market': { source: { source: 'directory', path: marketplaceRoot }, installLocation: marketplaceRoot, lastUpdated: new Date(0).toISOString() } }),
|
||||
'utf-8',
|
||||
)
|
||||
// Tombstone left behind by a failed install or uninstall: explicitly
|
||||
// disabled, but the plugin no longer exists in the marketplace.
|
||||
await fs.writeFile(
|
||||
path.join(tmpDir, 'settings.json'),
|
||||
JSON.stringify({ enabledPlugins: { 'gone@test-market': false, 'kept@test-market': true } }),
|
||||
'utf-8',
|
||||
)
|
||||
|
||||
const result = await loadAllPluginsCacheOnly()
|
||||
expect(result.errors.some(error => error.source === 'gone@test-market')).toBe(false)
|
||||
expect(result.errors.some(error => error.source === 'kept@test-market')).toBe(true)
|
||||
})
|
||||
|
||||
it('POST /api/plugins/reload hot-reloads an active CLI session and updates slash commands', async () => {
|
||||
const controlRequests: Array<{ sessionId: string; request: Record<string, unknown> }> = []
|
||||
conversationService.hasSession = ((sessionId: string) => sessionId === 'session-plugins') as typeof conversationService.hasSession
|
||||
|
||||
@@ -124,6 +124,25 @@ test('cancelling a first install leaves no installed or failed connector behind'
|
||||
expect(f.service.get('feishu').failedPhase).toBeUndefined()
|
||||
expect(f.calls).not.toContain('install')
|
||||
expect(f.calls).not.toContain('enabled:true')
|
||||
// No enabled=false tombstone either: the plugin was never published to a
|
||||
// marketplace, so a settings entry would surface as plugin-not-found in
|
||||
// every later plugin load and fail unrelated reloads.
|
||||
expect(f.calls).not.toContain('enabled:false')
|
||||
} finally { f.cleanup() }
|
||||
})
|
||||
|
||||
test('a failed first install writes no disabled plugin tombstone into settings', async () => {
|
||||
const f = fixture()
|
||||
try {
|
||||
const baseInstall = f.deps.bridge.installConnectorPlugin
|
||||
f.deps.bridge.installConnectorPlugin = async (def, installation) => { await baseInstall(def, installation); throw new Error('simulated bridge failure') }
|
||||
f.service.action('feishu', 'prepare')
|
||||
await settled(f.service)
|
||||
expect(f.service.get('feishu')).toMatchObject({ installed: false, status: 'error' })
|
||||
expect(f.calls).toContain('install')
|
||||
// The failure path must not disable an unpublished plugin: that settings
|
||||
// key refers to a plugin no marketplace knows and never gets cleaned up.
|
||||
expect(f.calls).not.toContain('enabled:false')
|
||||
} finally { f.cleanup() }
|
||||
})
|
||||
|
||||
|
||||
@@ -236,7 +236,15 @@ export class ConnectorService {
|
||||
}
|
||||
let cleanupFailed = false
|
||||
if (!restored) {
|
||||
try { await bridge.setConnectorPluginEnabled(def, false); await bridge.reloadConnectorSessions(options.sessionId) } catch { cleanupFailed = true }
|
||||
// A first install that never published has no settings entry to clear:
|
||||
// writing enabled=false here would leave a permanent plugin-not-found
|
||||
// tombstone in enabledPlugins (the plugin is not in any marketplace),
|
||||
// poisoning every later reload's error_count.
|
||||
const publishedToSettings = published || !!previous.installation
|
||||
try {
|
||||
if (publishedToSettings) await bridge.setConnectorPluginEnabled(def, false)
|
||||
await bridge.reloadConnectorSessions(options.sessionId)
|
||||
} catch { cleanupFailed = true }
|
||||
}
|
||||
if (cancelled && !restored) { try { await adapter?.deactivate() } catch { cleanupFailed = true } }
|
||||
if (cancelled && !persistenceFailed && !cleanupFailed) {
|
||||
|
||||
@@ -48,3 +48,14 @@ test('one chat failing to obtain tools prevents connector readiness; stopped cha
|
||||
reload.mockResolvedValue({ applied: false, reason: 'not_running', commands: 0, agents: 0, plugins: 0, mcpServers: 0, errors: 0 })
|
||||
await expect(reloadConnectorSessions(undefined, remote)).resolves.toBeUndefined()
|
||||
})
|
||||
|
||||
test('an unrelated plugin load error in a chat does not veto the connector reload', async () => {
|
||||
spyOn(conversationService, 'getActiveSessions').mockReturnValue(['chat-a'])
|
||||
// error_count > 0 from a foreign plugin (e.g. stale enabledPlugins tombstone)
|
||||
// must not fail this connector's refresh; the connector itself is proven by
|
||||
// requiredPlugin/requiredMcpServer inside reloadSessionComponents.
|
||||
spyOn(sessionReload, 'reloadSessionComponents').mockResolvedValue({
|
||||
applied: true, commands: 1, agents: 0, plugins: 1, mcpServers: 0, errors: 1,
|
||||
})
|
||||
await expect(reloadConnectorSessions(undefined, remote)).resolves.toBeUndefined()
|
||||
})
|
||||
|
||||
@@ -264,7 +264,12 @@ export async function reloadConnectorSessions(sessionId?: string, requiredConnec
|
||||
const sessions = new Set(conversationService.getActiveSessions())
|
||||
if (sessionId && conversationService.hasSession(sessionId)) sessions.add(sessionId)
|
||||
const results = await Promise.all([...sessions].map(id => reloadSessionComponents(id, requiredMcpServer, requiredPlugin)))
|
||||
if (results.some(result => result.reason === 'failed' || result.errors > 0)) {
|
||||
// error_count covers ALL plugins in the session, including unrelated ones
|
||||
// with stale settings entries. The connector itself is verified by
|
||||
// requiredPlugin/requiredMcpServer inside reloadSessionComponents and by
|
||||
// isConnectorPluginReady after this call; a foreign plugin's load error
|
||||
// must not veto this connector's install.
|
||||
if (results.some(result => result.reason === 'failed')) {
|
||||
throw new Error('Connector changed on disk, but an active task could not refresh its tools and skills. Retry the connection check before use.')
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2073,6 +2073,12 @@ async function loadPluginsFromMarketplaces({
|
||||
}
|
||||
|
||||
if (!result) {
|
||||
// An explicitly disabled entry for a plugin that is not in its
|
||||
// marketplace (uninstalled, delisted, or a failed-install leftover)
|
||||
// is an inert state, not a load error. Reporting it as
|
||||
// plugin-not-found makes every reload's error_count non-zero, which
|
||||
// unrelated flows (e.g. connector session refresh) treat as failure.
|
||||
if (!isEnabledPluginSettingValue(enabledValue)) return null
|
||||
errors.push({
|
||||
type: 'plugin-not-found',
|
||||
source: pluginId,
|
||||
|
||||
Reference in New Issue
Block a user