From b82dd78209d91c35a57bafa11317a7c9b4270cf9 Mon Sep 17 00:00:00 2001 From: zerob13 Date: Tue, 20 Jan 2026 19:05:56 +0800 Subject: [PATCH 1/2] docs: add spec for agent provider --- .../agent-provider-simplification/plan.md | 68 +++++++++++++++++++ .../agent-provider-simplification/spec.md | 43 ++++++++++++ .../agent-provider-simplification/tasks.md | 26 +++++++ 3 files changed, 137 insertions(+) create mode 100644 docs/specs/agent-provider-simplification/plan.md create mode 100644 docs/specs/agent-provider-simplification/spec.md create mode 100644 docs/specs/agent-provider-simplification/tasks.md diff --git a/docs/specs/agent-provider-simplification/plan.md b/docs/specs/agent-provider-simplification/plan.md new file mode 100644 index 000000000..1820f65a5 --- /dev/null +++ b/docs/specs/agent-provider-simplification/plan.md @@ -0,0 +1,68 @@ +# Plan: Agent Provider Simplification (ACP-only) + +## Summary + +Replace the “agent provider” abstraction and detection logic with a single explicit rule: **ACP is the only agent provider and is identified by `providerId === 'acp'`.** + +## Current Call Flow (relevant parts) + +- Main: + - `ProviderInstanceManager.createProviderInstance()` already special-cases `provider.id === 'acp'`. + - `ProviderInstanceManager.isAgentProvider()` uses `instanceof BaseAgentProvider` and (if instance not created) a constructor prototype check (`isAgentConstructor`). + - `LLMProviderPresenter.isAgentProvider()` exposes this to the renderer via `ILlmProviderPresenter`. +- Renderer: + - `src/renderer/src/stores/modelStore.ts` calls `llmproviderPresenter.isAgentProvider(providerId)` over IPC to choose between: + - `agentModelStore.refreshAgentModels(providerId)` (ACP path) + - `refreshStandardModels + refreshCustomModels` (standard path) + - Other renderer logic already treats ACP as special via `provider.id === 'acp'`. + +## Proposed Changes + +### 1) Remove agent-provider classification API + +- Remove `isAgentProvider(providerId: string)` from: + - `src/shared/types/presenters/llmprovider.presenter.d.ts` + - `src/shared/types/presenters/legacy.presenters.d.ts` + - `src/main/presenter/llmProviderPresenter/index.ts` + - `src/main/presenter/llmProviderPresenter/managers/providerInstanceManager.ts` + +Rationale: It is only used by the renderer for ACP gating, and ACP can be identified locally by ID. + +### 2) Replace renderer gating with an explicit ACP check + +- In `src/renderer/src/stores/modelStore.ts`: + - Remove the async IPC call `llmP.isAgentProvider(providerId)`. + - Replace with a local predicate: `providerId === 'acp'`. + - Keep the existing ACP refresh path using `agentModelStore.refreshAgentModels('acp')` (no behavioral change). + +### 3) Remove `BaseAgentProvider` (optional but preferred) + +Because `BaseAgentProvider` is only used by `AcpProvider`, delete the base class and: + +- Make `AcpProvider` extend `BaseLLMProvider` directly. +- Move `cleanup()` logic into `AcpProvider` (or delegate to `AcpSessionManager` / `AcpProcessManager`). +- Ensure `cleanup()` is safe to call multiple times and during shutdown. + +Notes: +- `acpCleanupHook` currently awaits `cleanup()` even though `BaseAgentProvider.cleanup()` is `void`. Consider standardizing ACP cleanup to `Promise` to match usage. + +## Compatibility / Migration + +- No user data migration. +- Provider ID `acp` remains unchanged and is treated as a stable internal contract. +- Any internal IPC typing generation must be updated to reflect removal of `isAgentProvider`. + +## Test Strategy + +Add minimal tests focusing on the only behavioral dependency (renderer model refresh selection): + +- Renderer unit test for `modelStore.refreshProviderModels()`: + - When `providerId === 'acp'`, it uses `agentModelStore.refreshAgentModels`. + - When `providerId !== 'acp'`, it uses standard refresh path. + +Main-process unit tests are optional; the change is mostly removal and ACP-id checks. + +## Rollout + +Single PR is acceptable if changes stay localized (types + modelStore + ACP provider base class cleanup). + diff --git a/docs/specs/agent-provider-simplification/spec.md b/docs/specs/agent-provider-simplification/spec.md new file mode 100644 index 000000000..2c6aa6579 --- /dev/null +++ b/docs/specs/agent-provider-simplification/spec.md @@ -0,0 +1,43 @@ +# Agent Provider Simplification (ACP-only) + +## Background + +DeepChat currently distinguishes between: + +- **LLM providers**: network-backed providers that implement `BaseLLMProvider` (OpenAI/Anthropic/etc). +- **Agent providers**: providers that manage local agent sessions/processes (currently only `acp` via `AcpProvider`). + +The codebase implements this distinction via a dedicated base class (`BaseAgentProvider`) and a runtime/type-detection API (`isAgentProvider`), which is then consumed from the renderer via IPC. + +## Problem + +- `BaseAgentProvider` is only used by `AcpProvider`, so the abstraction adds indirection without real reuse. +- Provider type detection is over-engineered (`isAgentConstructor` + prototype checks) and duplicates existing ACP-specific branching. +- The renderer calls `llmproviderPresenter.isAgentProvider(providerId)` over IPC, but the only “agent provider” is `providerId === 'acp'`. This creates unnecessary main↔renderer coupling and call complexity. + +## Goals + +- Treat **ACP as the only agent provider** and identify it **only by `providerId === 'acp'`**. +- Remove the generic “agent provider type detection” path and the renderer IPC dependency for this decision. +- Keep user-visible behavior unchanged: + - ACP agents still appear as selectable models when ACP is enabled. + - Non-ACP providers keep the standard model/custom-model refresh behavior. + - Shutdown and provider disable still clean up ACP resources. + +## Non-goals + +- Supporting multiple agent providers beyond ACP. +- Redesigning ACP model derivation (agents-as-models) or session/workspace semantics. +- Changing persisted provider IDs or stored settings schemas. + +## Acceptance Criteria + +- Renderer no longer calls `llmproviderPresenter.isAgentProvider(...)`; ACP decision is local (`providerId === 'acp'`). +- Main process no longer needs `isAgentConstructor` / prototype-based provider classification. +- No remaining runtime dependency on `BaseAgentProvider` for correctness (ACP cleanup remains correct). +- `pnpm run typecheck`, `pnpm test`, `pnpm run lint` pass. + +## Open Questions + +- None. + diff --git a/docs/specs/agent-provider-simplification/tasks.md b/docs/specs/agent-provider-simplification/tasks.md new file mode 100644 index 000000000..9b6918842 --- /dev/null +++ b/docs/specs/agent-provider-simplification/tasks.md @@ -0,0 +1,26 @@ +# Tasks: Agent Provider Simplification (ACP-only) + +1. Update renderer to stop using IPC for agent-provider detection + - Remove `llmproviderPresenter.isAgentProvider` usage from `src/renderer/src/stores/modelStore.ts`. + - Gate ACP behavior by `providerId === 'acp'`. + +2. Remove `isAgentProvider` from the presenter contract + - Remove from `src/shared/types/presenters/llmprovider.presenter.d.ts`. + - Remove from `src/shared/types/presenters/legacy.presenters.d.ts`. + - Remove implementation from `src/main/presenter/llmProviderPresenter/index.ts`. + +3. Remove main-side agent-provider classification implementation + - Delete `ProviderInstanceManager.isAgentProvider()` and `isAgentConstructor()` in `src/main/presenter/llmProviderPresenter/managers/providerInstanceManager.ts`. + - Ensure no other code path depends on `BaseAgentProvider` type checks. + +4. Remove `BaseAgentProvider` abstraction (preferred) + - Delete `src/main/presenter/llmProviderPresenter/baseAgentProvider.ts`. + - Update `src/main/presenter/llmProviderPresenter/providers/acpProvider.ts` to extend `BaseLLMProvider` directly. + - Keep/adjust ACP cleanup semantics (safe shutdown, provider disable, app quit). + +5. Add/adjust tests + - Add a Vitest suite under `test/renderer/**` validating model refresh selection for ACP vs non-ACP. + +6. Quality gates + - Run `pnpm run format`, `pnpm run lint`, `pnpm run typecheck`, and `pnpm test`. + From 1449f47c3952ef0f9749b392bd776a37d0d21e88 Mon Sep 17 00:00:00 2001 From: zerob13 Date: Wed, 21 Jan 2026 10:04:17 +0800 Subject: [PATCH 2/2] refactor(agent): remove BaseAgentProvider layer and simplify provider hierarchy --- .../llmProviderPresenter/baseAgentProvider.ts | 47 --------- .../presenter/llmProviderPresenter/index.ts | 4 - .../managers/providerInstanceManager.ts | 24 ----- .../providers/acpProvider.ts | 25 +---- src/renderer/src/stores/modelStore.ts | 10 +- .../types/presenters/legacy.presenters.d.ts | 1 - .../presenters/llmprovider.presenter.d.ts | 1 - test/renderer/stores/modelStore.test.ts | 99 +++++++++++++++++++ 8 files changed, 102 insertions(+), 109 deletions(-) delete mode 100644 src/main/presenter/llmProviderPresenter/baseAgentProvider.ts create mode 100644 test/renderer/stores/modelStore.test.ts diff --git a/src/main/presenter/llmProviderPresenter/baseAgentProvider.ts b/src/main/presenter/llmProviderPresenter/baseAgentProvider.ts deleted file mode 100644 index 0d48eab11..000000000 --- a/src/main/presenter/llmProviderPresenter/baseAgentProvider.ts +++ /dev/null @@ -1,47 +0,0 @@ -import { BaseLLMProvider } from './baseProvider' -import type { - AgentPermissionRequest, - AgentPermissionResult, - AgentProcessManager, - AgentSessionManager -} from '../agentPresenter/acp' - -/** - * Base class for Agent-specific providers. - * Ensures that session/process lifecycle management is centralized - * while allowing subclasses to supply concrete managers. - */ -export abstract class BaseAgentProvider< - TSessionManager extends AgentSessionManager = AgentSessionManager, - TProcessManager extends AgentProcessManager = AgentProcessManager, - TPermissionRequest = AgentPermissionRequest, - TPermissionResult = AgentPermissionResult -> extends BaseLLMProvider { - protected abstract getSessionManager(): TSessionManager - protected abstract getProcessManager(): TProcessManager - protected abstract requestPermission(params: TPermissionRequest): Promise - - /** - * Default cleanup hook invoked when provider instances are torn down. - * Clears in-memory sessions and tears down any running agent processes. - */ - public cleanup(): void { - void this.getSessionManager() - .clearAllSessions() - .catch((error) => { - console.warn( - `[AgentProvider] Failed to clear sessions for provider "${this.provider.id}":`, - error - ) - }) - - void this.getProcessManager() - .shutdown() - .catch((error) => { - console.warn( - `[AgentProvider] Failed to shutdown process manager for provider "${this.provider.id}":`, - error - ) - }) - } -} diff --git a/src/main/presenter/llmProviderPresenter/index.ts b/src/main/presenter/llmProviderPresenter/index.ts index 61c9b86cc..3b8046593 100644 --- a/src/main/presenter/llmProviderPresenter/index.ts +++ b/src/main/presenter/llmProviderPresenter/index.ts @@ -118,10 +118,6 @@ export class LLMProviderPresenter implements ILlmProviderPresenter { return this.providerInstanceManager.getProviderById(id) } - isAgentProvider(providerId: string): boolean { - return this.providerInstanceManager.isAgentProvider(providerId) - } - async setCurrentProvider(providerId: string): Promise { // 如果有正在生成的流，先停止它们 await this.stopAllStreams() diff --git a/src/main/presenter/llmProviderPresenter/managers/providerInstanceManager.ts b/src/main/presenter/llmProviderPresenter/managers/providerInstanceManager.ts index f55260155..f0b4f3bbc 100644 --- a/src/main/presenter/llmProviderPresenter/managers/providerInstanceManager.ts +++ b/src/main/presenter/llmProviderPresenter/managers/providerInstanceManager.ts @@ -1,7 +1,6 @@ import { ProviderBatchUpdate, ProviderChange } from '@shared/provider-operations' import { IConfigPresenter, LLM_PROVIDER } from '@shared/presenter' import { BaseLLMProvider } from '../baseProvider' -import { BaseAgentProvider } from '../baseAgentProvider' import { OpenAIProvider } from '../providers/openAIProvider' import { DeepseekProvider } from '../providers/deepseekProvider' import { SiliconcloudProvider } from '../providers/siliconcloudProvider' @@ -134,11 +133,6 @@ export class ProviderInstanceManager { ]) } - private static isAgentConstructor(ctor?: ProviderConstructor): boolean { - if (!ctor) return false - return BaseAgentProvider.prototype.isPrototypeOf(ctor.prototype) - } - init(): void { const providers = this.options.configPresenter.getProviders() for (const provider of providers) { @@ -250,24 +244,6 @@ export class ProviderInstanceManager { return instance } - isAgentProvider(providerId: string): boolean { - const instance = this.providerInstances.get(providerId) - if (instance) { - return instance instanceof BaseAgentProvider - } - - const provider = this.providers.get(providerId) - if (!provider) { - return false - } - - const ProviderClass = - ProviderInstanceManager.PROVIDER_ID_MAP.get(provider.id) ?? - ProviderInstanceManager.PROVIDER_TYPE_MAP.get(provider.apiType) - - return ProviderInstanceManager.isAgentConstructor(ProviderClass) - } - private handleProviderAdd(change: ProviderChange): void { if (!change.provider) return diff --git a/src/main/presenter/llmProviderPresenter/providers/acpProvider.ts b/src/main/presenter/llmProviderPresenter/providers/acpProvider.ts index 8f5d9e153..84629f61a 100644 --- a/src/main/presenter/llmProviderPresenter/providers/acpProvider.ts +++ b/src/main/presenter/llmProviderPresenter/providers/acpProvider.ts @@ -1,6 +1,5 @@ import type * as schema from '@agentclientprotocol/sdk/dist/schema.js' -import { SUMMARY_TITLES_PROMPT } from '../baseProvider' -import { BaseAgentProvider } from '../baseAgentProvider' +import { BaseLLMProvider, SUMMARY_TITLES_PROMPT } from '../baseProvider' import type { ChatMessage, LLMResponse, @@ -58,12 +57,7 @@ type PendingPermissionState = { reject: (error: Error) => void } -export class AcpProvider extends BaseAgentProvider< - AcpSessionManager, - AcpProcessManager, - schema.RequestPermissionRequest, - schema.RequestPermissionResponse -> { +export class AcpProvider extends BaseLLMProvider { private readonly processManager: AcpProcessManager private readonly sessionManager: AcpSessionManager private readonly sessionPersistence: AcpSessionPersistence @@ -102,21 +96,6 @@ export class AcpProvider extends BaseAgentProvider< void this.initWhenEnabled() } - protected getSessionManager(): AcpSessionManager { - return this.sessionManager - } - - protected getProcessManager(): AcpProcessManager { - return this.processManager - } - - protected async requestPermission( - params: schema.RequestPermissionRequest - ): Promise { - void params - return { outcome: { outcome: 'cancelled' } } - } - protected async fetchProviderModels(): Promise { try { const acpEnabled = await this.configPresenter.getAcpEnabled() diff --git a/src/renderer/src/stores/modelStore.ts b/src/renderer/src/stores/modelStore.ts index 79eb9de46..a33ee2f51 100644 --- a/src/renderer/src/stores/modelStore.ts +++ b/src/renderer/src/stores/modelStore.ts @@ -38,14 +38,6 @@ export const useModelStore = defineStore('model', () => { const customModelQueries = new Map>() const enabledModelQueries = new Map>() const queryCache = useQueryCache() - const isAgentProvider = async (providerId: string): Promise => { - try { - return Boolean(await llmP.isAgentProvider(providerId)) - } catch (error) { - console.warn(`[ModelStore] Failed to determine provider type: ${providerId}`, error) - return false - } - } const matchesProviderModelsEntry = ( entry: { key: readonly unknown[] }, @@ -436,7 +428,7 @@ export const useModelStore = defineStore('model', () => { } const refreshProviderModels = async (providerId: string) => { - if (await isAgentProvider(providerId)) { + if (providerId === 'acp') { try { const { rendererModels, modelMetas } = await agentModelStore.refreshAgentModels(providerId) updateProviderModelsCache(providerId, modelMetas) diff --git a/src/shared/types/presenters/legacy.presenters.d.ts b/src/shared/types/presenters/legacy.presenters.d.ts index 50aaf4ad9..06379493b 100644 --- a/src/shared/types/presenters/legacy.presenters.d.ts +++ b/src/shared/types/presenters/legacy.presenters.d.ts @@ -946,7 +946,6 @@ export interface ILlmProviderPresenter { setProviders(provider: LLM_PROVIDER[]): void getProviders(): LLM_PROVIDER[] getProviderById(id: string): LLM_PROVIDER - isAgentProvider(providerId: string): boolean getExistingProviderInstance?(providerId: string): unknown getModelList(providerId: string): Promise updateModelStatus(providerId: string, modelId: string, enabled: boolean): Promise diff --git a/src/shared/types/presenters/llmprovider.presenter.d.ts b/src/shared/types/presenters/llmprovider.presenter.d.ts index 159128c90..13d185efd 100644 --- a/src/shared/types/presenters/llmprovider.presenter.d.ts +++ b/src/shared/types/presenters/llmprovider.presenter.d.ts @@ -175,7 +175,6 @@ export interface ILlmProviderPresenter { setProviders(provider: LLM_PROVIDER[]): void getProviders(): LLM_PROVIDER[] getProviderById(id: string): LLM_PROVIDER - isAgentProvider(providerId: string): boolean getProviderInstance(providerId: string): unknown getExistingProviderInstance(providerId: string): unknown getModelList(providerId: string): Promise diff --git a/test/renderer/stores/modelStore.test.ts b/test/renderer/stores/modelStore.test.ts new file mode 100644 index 000000000..ff717ea05 --- /dev/null +++ b/test/renderer/stores/modelStore.test.ts @@ -0,0 +1,99 @@ +import { describe, it, expect, vi } from 'vitest' +import { ref } from 'vue' + +const createQueryCache = () => { + return { + ensure: vi.fn((options: any) => ({ + key: options.key, + state: ref({ data: undefined }) + })), + invalidateQueries: vi.fn(async () => undefined), + refresh: vi.fn(async (entry: any) => entry.state.value), + fetch: vi.fn(async (entry: any) => entry.state.value), + setQueriesData: vi.fn() + } +} + +const setupStore = async () => { + vi.resetModules() + + const queryCache = createQueryCache() + const agentModelStore = { + refreshAgentModels: vi.fn() + } + const modelConfigStore = { + getModelConfig: vi.fn(async () => null) + } + const configPresenter = { + getDbProviderModels: vi.fn(async () => []), + getProviderModels: vi.fn(async () => []), + getCustomModels: vi.fn(async () => []), + getBatchModelStatus: vi.fn(async () => ({})) + } + const llmPresenter = { + getModelList: vi.fn(async () => []) + } + + vi.doMock('pinia', () => ({ + defineStore: (_id: string, setup: any) => setup + })) + + vi.doMock('@pinia/colada', () => ({ + useQueryCache: () => queryCache + })) + + vi.doMock('@/stores/agentModelStore', () => ({ + useAgentModelStore: () => agentModelStore + })) + + vi.doMock('@/stores/modelConfigStore', () => ({ + useModelConfigStore: () => modelConfigStore + })) + + vi.doMock('@/stores/providerStore', () => ({ + useProviderStore: () => ({ providers: [] }) + })) + + vi.doMock('@/composables/usePresenter', () => ({ + usePresenter: (name: string) => (name === 'configPresenter' ? configPresenter : llmPresenter) + })) + + vi.doMock('@/composables/useIpcMutation', () => ({ + useIpcMutation: () => ({ mutateAsync: vi.fn() }) + })) + + const { useModelStore } = await import('@/stores/modelStore') + const store = useModelStore() + + return { + store, + agentModelStore, + configPresenter, + llmPresenter + } +} + +describe('modelStore.refreshProviderModels', () => { + it('uses ACP refresh path for acp provider', async () => { + const { store, agentModelStore, configPresenter } = await setupStore() + agentModelStore.refreshAgentModels.mockResolvedValue({ + rendererModels: [], + modelMetas: [] + }) + + await store.refreshProviderModels('acp') + + expect(agentModelStore.refreshAgentModels).toHaveBeenCalledWith('acp') + expect(configPresenter.getDbProviderModels).not.toHaveBeenCalled() + }) + + it('uses standard refresh path for non-acp provider', async () => { + const { store, agentModelStore, configPresenter } = await setupStore() + + await store.refreshProviderModels('openai') + + expect(agentModelStore.refreshAgentModels).not.toHaveBeenCalled() + expect(configPresenter.getDbProviderModels).toHaveBeenCalledWith('openai') + expect(configPresenter.getProviderModels).toHaveBeenCalledWith('openai') + }) +})