src/renderer/src/components/message/MessageBlockToolCall.vue (1)

4-6: Verify layout behavior with visual testing.

The changes to inline-flex, removal of flex-wrap, and addition of overflow-hidden alter the container's layout behavior. The switch to inline-flex means the container will only be as wide as its content (up to max-width) rather than filling available space, and removing flex-wrap prevents content from wrapping to multiple lines.

Please verify:

• The tool call block displays correctly with various content lengths
• Long server names and function names truncate properly without breaking layout
• The component flows correctly with surrounding elements in different screen sizes

src/renderer/src/components/message/MessageItemAssistant.vue (1)

7-19: Consider adding alt for ACP ModelIcon for consistency/accessibility

The ACP-specific branch correctly switches to currentMessage.model_id, but it drops the alt prop that the non-ACP branch provides. To keep accessibility and behavior consistent, you can also pass :alt="currentMessage.role" (or a more specific label) on the ACP branch.

-      <ModelIcon
-        v-if="currentMessage.model_provider === 'acp'"
-        :model-id="currentMessage.model_id"
-        :is-dark="themeStore.isDark"
-        custom-class="w-[18px] h-[18px]"
-      />
+      <ModelIcon
+        v-if="currentMessage.model_provider === 'acp'"
+        :model-id="currentMessage.model_id"
+        :is-dark="themeStore.isDark"
+        custom-class="w-[18px] h-[18px]"
+        :alt="currentMessage.role"
+      />

src/main/presenter/llmProviderPresenter/managers/modelManager.ts (1)

14-21: Model/providerId normalization and logging are reasonable, but somewhat redundant

The added logging and providerId fix-up in getModelList help harden against malformed model metadata and provide good diagnostics. Note that BaseLLMProvider.fetchModels() now also normalizes providerId, so this layer is mostly a safety net rather than strictly necessary. Keeping both is fine; if logs become noisy in production, you could later centralize the normalization in one place.

Also applies to: 23-34, 56-66

src/main/presenter/llmProviderPresenter/agent/acpMessageFormatter.ts (2)

34-41: Consider improving tool call text formatting for clarity.

The tool call text concatenation on line 38 directly concatenates the function name and arguments without a delimiter:

text: `${prefix} TOOL CALL ${toolCall.id || ''}: ${toolCall.function?.name || 'unknown'} ${toolCall.function?.arguments || ''}`

This could make it difficult to parse or read when arguments are present. Consider adding a clearer delimiter or structure, such as:

text: `${prefix} TOOL CALL ${toolCall.id || ''}: ${toolCall.function?.name || 'unknown'} ARGS: ${toolCall.function?.arguments || ''}`

---

60-92: Content normalization is functional but lacks defensive error handling.

The normalizeContent method handles various content formats (string, array with text/image parts) but doesn't validate or handle malformed input gracefully. If content has an unexpected structure, the method silently returns an empty array or skips invalid parts.

For production resilience, consider:

• Logging warnings when content doesn't match expected structure
• Validating that content is the expected type before processing

This is optional as silent failure may be acceptable for a formatter, but explicit logging would aid debugging.

src/renderer/src/stores/agentModelStore.ts (1)

29-75: Document hardcoded ACP model defaults and consider validation.

The refreshAgentModels method uses hardcoded default values for all ACP agent models:

• contextLength: 8192
• maxTokens: 4096
• vision: false, functionCall: true, reasoning: false, enableSearch: false

Consider:

1. Documentation: Add a comment explaining why these specific defaults are appropriate for all ACP agents
2. Uniqueness validation: No check that agent.id values are unique. Duplicate IDs could cause conflicts in model selection
3. Error handling: No try-catch for getAcpAgents() - failures will bubble up unhandled

Example validation:

const agents = await configPresenter.getAcpAgents()
const seenIds = new Set<string>()
for (const agent of agents) {
  if (seenIds.has(agent.id)) {
    console.warn(`Duplicate ACP agent ID detected: ${agent.id}`)
  }
  seenIds.add(agent.id)
}

src/main/presenter/llmProviderPresenter/agent/types.ts (1)

15-41: Clarify session keying and process descriptor vs agentId symmetry

The abstractions look clean, but there are two design points worth double‑checking:

1. AgentSessionManager.getSession(conversationId: string) only keys by conversationId, while your SQLite table (AcpSessionsTable) uses (conversation_id, agent_id) as the uniqueness constraint. If a conversation can have multiple agents, this signature may be ambiguous. Consider either:

   • Making getSession take the agent descriptor as well, or
   • Clearly documenting that an AgentSessionManager is scoped to a single agent so conversationId is sufficient.

2. In AgentProcessManager, getConnection(agent: TDescriptor) accepts a generic descriptor, but getProcess/release take agentId: string. If descriptors can be richer than a string, you may want these methods to also accept TDescriptor for symmetry, or alternatively rename TDescriptor to TAgentId and keep everything string‑based.

Both are design/API questions, but locking this down now will make later providers/sessions easier to reason about.

src/renderer/src/i18n/fr-FR/routes.json (1)

15-16: French ACP label is fine; consider plural if the page lists multiple agents

"settings-acp": "Agent ACP" is valid and clear. If this settings page manages multiple agents (as the English “ACP Agents” suggests), you might optionally prefer "Agents ACP" for number agreement, but it’s not strictly required.

src/renderer/src/i18n/zh-TW/routes.json (1)

15-16: zh-TW ACP label is clear; optional localization tweak

The new entry "settings-acp": "ACP Agent" is valid and understandable, and the JSON remains well‑formed.

If you want to align more closely with "MCP市場", you could optionally localize “Agent” (e.g., "ACP 代理"), but this is stylistic.

src/renderer/src/components/ModelChooser.vue (1)

33-44: Consider consolidating the conditional ModelIcon rendering.

The current implementation duplicates the ModelIcon component with only the :model-id prop varying. You could simplify this using a computed property or ternary expression:

Apply this diff to reduce duplication:

                <div
                  class="flex h-7 w-7 items-center justify-center rounded-md border border-border bg-muted/40 text-[11px] font-semibold uppercase text-muted-foreground transition group-data-[selected=true]:border-primary group-data-[selected=true]:bg-primary/20 group-data-[selected=true]:text-primary"
                >
-                  <ModelIcon
-                    v-if="provider.id === 'acp'"
-                    class="h-4 w-4 shrink-0 opacity-80 transition group-hover:opacity-100 group-data-[selected=true]:opacity-100"
-                    :model-id="model.id"
-                    :is-dark="themeStore.isDark"
-                  />
-                  <ModelIcon
-                    v-else
+                  <ModelIcon
                    class="h-4 w-4 shrink-0 opacity-80 transition group-hover:opacity-100 group-data-[selected=true]:opacity-100"
-                    :model-id="provider.id"
+                    :model-id="provider.id === 'acp' ? model.id : provider.id"
                    :is-dark="themeStore.isDark"
                  />
                </div>

src/renderer/src/i18n/ko-KR/chat.json (1)

19-24: ACP workdir & toolCall keys look structurally correct; consider localizing new labels

The new ACP workdir strings and toolCall block mirror the en-US structure and placeholders, so they should integrate cleanly. If full localization is a goal for ko-KR, you may want to translate the ACP workdir labels instead of leaving them in English for consistency with the rest of this file.

Also applies to: 72-82

src/renderer/settings/components/ModelProviderSettings.vue (1)

217-223: Avoid navigation side-effects inside activeProvider computed; consider using visibleProviders for default selection

Filtering visibleProviders to exclude the acp provider and deriving enabled/disabled lists from it makes sense. The acp special-case in activeProvider, however, performs a route redirect from inside a computed, which tightly couples this computed to router behavior and can be brittle if route params change.

Two follow-ups you might consider:

• Move the acp redirect into setActiveProvider (or a watch on route.params.providerId) so computed logic stays side-effect free.
• In onMounted, pick the initial provider from visibleProviders instead of providerStore.sortedProviders to avoid briefly navigating to settings-provider with acp and then immediately redirecting to settings-acp.

Also applies to: 298-305

src/main/presenter/configPresenter/providerModelHelper.ts (1)

44-61: Provider model store isolation and providerId normalization look good; consider sanitizing storeName

The per-provider store isolation (name: models_${providerId}, cwd: userDataPath/provider_models) and providerId normalization in both getProviderModels and setProviderModels should make the model cache much more robust against mismatched or missing providerId values.

Two minor follow-ups you might consider:

• If there is any chance that providerId can contain path- or filename-unfriendly characters (/, \, :, etc.), normalize or hash it when building storeName to avoid filesystem issues.
• The post-validation incorrectProviderIds checks will essentially always be empty because you correct model.providerId earlier in the same map; you can keep them for extra safety or drop them later as dead diagnostics.

Functionally, the changes are solid as-is.

Also applies to: 68-116, 119-153

test/main/presenter/llmProviderPresenter/acpContentMapper.test.ts (1)

1-114: Good coverage of ACP tool call mapping; core behaviors are well tested

These tests exercise the critical AcpContentMapper behaviors: emitting start/chunk/end events with correctly assembled arguments and maintaining independent tool-call state per session even when ids collide. That gives solid confidence in the streaming tool-call path.

If you expand tests later, you might add cases for failed status or non-JSON argument buffers, but the current suite is already very useful.

src/main/presenter/configPresenter/providerHelper.ts (2)

36-103: Provider store self-heal works, but listChanged may miss some repair cases

The getProviders flow does a nice job of:

• Skipping MODEL_META-like entries (with providerId but no apiType).
• Merging partial provider entries with defaults when ids match.
• Restoring full defaults if no valid providers are found.
• Ensuring all default providers are present in the final list.

One nuance: listChanged only checks for a length change or remaining entries without apiType. If you repair entries by merging them with templates (i.e., fill in missing fields but preserve length and add apiType), listChanged stays false and the healed data is never written back, so the underlying store remains in its pre-repair shape. Callers still get repairedProviders, so behavior is correct, but you lose the “self-healing” persistence.

If you want the store to be truly normalized after any repair, consider broadening listChanged (e.g., deep-compare ids/order, or track whether you merged/skipped anything during the loop) and persisting when repairs actually occurred.

---

115-134: Centralize provider validation to avoid future inconsistencies

setProviders now filters out invalid entries (missing id or apiType) before persisting, which is good. However, several code paths still write provider lists directly via setSetting:

• updateProvidersBatch uses batchUpdate.providers as-is.
• updateProviderAtomic, addProviderAtomic, and reorderProvidersAtomic also call setSetting directly.

As long as those callers only ever pass well-formed LLM_PROVIDERs this is fine, but over time it’s easy for a new caller to bypass the validation unintentionally. To keep the invariants enforced uniformly, consider routing all writes of the full providers array through a single validation path (e.g., reuse setProviders from these methods, or extract the validation/filtering into a shared helper).

Also applies to: 175-178, 180-183, 207-208

src/main/presenter/sqlitePresenter/index.ts (1)

256-259: Consider wrapping cascade deletion in a transaction.

The deletion of a conversation and its associated ACP sessions is performed sequentially without transaction protection. If deleteByConversation fails after the conversation is deleted, the ACP sessions will remain orphaned.

Apply this diff to ensure atomic deletion:

 public async deleteConversation(conversationId: string): Promise<void> {
-  await this.conversationsTable.delete(conversationId)
-  await this.acpSessionsTable.deleteByConversation(conversationId)
+  await this.runTransaction(() => {
+    this.conversationsTable.delete(conversationId)
+    this.acpSessionsTable.deleteByConversation(conversationId)
+  })
 }

Note: Verify that the table deletion methods are synchronous (not returning promises) before applying this change, as runTransaction expects synchronous operations.

test/main/presenter/threadPresenter/permissionHandler.test.ts (1)

112-126: Consider adding edge case coverage for ACP permission handling.

The tests correctly verify the happy path for granted/denied permissions. Consider adding test cases for error scenarios:

it('handles missing permissionRequestId gracefully', async () => {
  const blockWithoutRequestId = {
    ...block,
    extra: { ...block.extra, permissionRequestId: undefined }
  }
  const { handler, messageId, toolCallId } = createHandler({ 
    block: blockWithoutRequestId 
  })
  
  // Should not crash and possibly log error
  await expect(
    handler.handlePermissionResponse(messageId, toolCallId, true, 'write', false)
  ).resolves.not.toThrow()
})

it('handles resolveAgentPermission rejection', async () => {
  const mockReject = vi.fn().mockRejectedValue(new Error('Network error'))
  const { handler, messageId, toolCallId } = createHandler({ 
    resolveAgentPermission: mockReject 
  })
  
  await expect(
    handler.handlePermissionResponse(messageId, toolCallId, true, 'write', false)
  ).rejects.toThrow('Network error')
})

src/shared/types/presenters/agent-provider.d.ts (1)

1-32: Agent provider/session types look good; avoid duplicate definitions

The new AgentSessionState / AgentProcessHandle shapes are sensible and cover expected metadata. However, the same symbols also exist in src/main/presenter/llmProviderPresenter/agent/types.ts, which risks the two diverging over time.

Consider centralizing these type definitions (e.g., only in @shared/presenter and importing them where needed) rather than redefining similar interfaces in multiple places.

src/renderer/src/i18n/ru-RU/chat.json (1)

19-24: Consider localizing new ACP workdir strings

The new acpWorkdir* strings ("ACP Workdir", "Set ACP working directory", etc.) are still in English while the rest of this file is Russian. Functionally it’s fine, but for consistency and UX, it would be better to translate these into Russian (similar to how toolCall.* was localized).

Also applies to: 73-82

test/main/presenter/llmProviderPresenter.test.ts (1)

4-9: SQLite presenter mock correctly matches new constructor requirements

The tests have been updated to:

• Import ISQLitePresenter and build a mockSqlitePresenter that includes the new ACP session/workdir methods.
• Pass this mock into LLMProviderPresenter wherever it’s instantiated, matching the new (configPresenter, sqlitePresenter) signature.

For these integration-style tests (which target non-ACP flows), this level of mocking is sufficient and keeps the existing behavior intact. If ISQLitePresenter keeps evolving, you might eventually want a shared factory/helper for this mock to avoid manual drift across test files, but that can be deferred.

Also applies to: 79-110, 177-178, 582-583

src/renderer/src/components/chat-input/ChatInput.vue (2)

102-150: ACP workdir integration and state wiring look solid

The ACP workdir button/tooltip wiring matches the composable API: isAcpModel and hasWorkdir correctly gate visibility/state, and conversationId plus activeModelSource give the composable enough context for both chat and new-thread flows. The pending-workdir path should behave correctly with modelInfo-driven new-thread creation and the chat store’s acpWorkdirMap.

Minor nit: in the template you can drop the .value access on acpWorkdir.* since Vue automatically unwraps refs in templates; this would slightly simplify the markup.

Also applies to: 369-397, 533-545

---

205-216: Guard ACP ModelIcon usage against missing id

The ACP branch renders:

<ModelIcon
  v-if="config.activeModel.value.providerId === 'acp'"
  :model-id="config.activeModel.value.id"
  ...
/>

If there’s any transient state where providerId === 'acp' but id is unset, model-id will be undefined. ModelIcon will likely degrade gracefully, but you may want to either:

• Also check for config.activeModel.value.id in the v-if, or
• Provide a sensible fallback (e.g. a generic ACP icon) when id is missing.

This would make the UI a bit more robust to incomplete activeModel state.

src/renderer/src/stores/providerStore.ts (1)

227-248: optimizeProviderOrder currently ignores the enable flag

In optimizeProviderOrder, the computed newOrder is the same regardless of the enable boolean:

const newOrder = enable
  ? [...enabledOrder, providerId, ...disabledOrder]
  : [...enabledOrder, providerId, ...disabledOrder]

So the enable argument is effectively unused. Behavior is still reasonable (the provider is placed after all currently enabled providers and before all disabled ones), but this is either:

• Dead parameter that can be removed, or
• A missed opportunity to differentiate where disabled providers should land.

If the intent is just “move provider to the enabled/disabled boundary”, consider dropping enable from the signature; otherwise, adjust the false branch to place the provider in the disabled section explicitly.

src/renderer/src/components/chat-input/composables/useAcpWorkdir.ts (1)

1-176: ACP workdir composable design matches chat store and thread presenter usage

The composable cleanly encapsulates ACP workdir behavior:

• isAcpModel gating ensures all logic is scoped to ACP agents with a valid id.
• loadWorkdir only calls threadPresenter.getAcpWorkdir when both conversationId and agentId are present, and synchronizes the resulting path back into chatStore.setAcpWorkdirPreference, which lines up with how createThread and loadChatConfig use acpWorkdirMap (per the provided chat store snippets).
• The pendingWorkdir path for “chosen before conversation exists” is handled by selectWorkdir + syncPendingWhenReady, so the workdir is applied once a conversation id appears, and the tooltip/button state stays in sync.
• clearWorkdir correctly differentiates between conversation‑bound and pre‑conversation state, resetting both backend and preference.

If you want a bit more robustness, you could wrap devicePresenter.selectDirectory() in a try/catch to avoid bubbling any unexpected IPC errors out of selectWorkdir, but functionally this composable looks solid.

src/renderer/src/stores/chat.ts (1)

167-184: ACP workdir injection on thread creation looks correct but is intentionally conservative

Cloning settings into normalizedSettings and only attaching acpWorkdirMap when:

• no map is present/empty,
• providerId === 'acp',
• and modelId is a non-empty string,

is a good way to avoid mutating callers and only apply pending workdirs to ACP threads. You also correctly scope the injected map to the active modelId.

If in the future you want to support multiple ACP agents per conversation, you may consider copying over the entire acpWorkdirMap instead of just the selected modelId entry, but the current behavior is coherent with a single-agent-per-thread model.

src/renderer/settings/components/AcpSettings.vue (3)

467-505: Profile save handler covers all four modes; minor suggestion on explicit fallthrough handling

The switch (profileDialogState.mode) correctly handles builtin-add, builtin-edit, custom-add, and custom-edit, followed by a single refreshAfterMutation and success toast, with savingProfile guarded via finally.

You might optionally add a default branch that logs an error if an unknown mode appears, to make the code more future-proof against refactors:

default:
  console.error('[ACP] Unknown profile dialog mode:', profileDialogState.mode)
  return

---

532-559: Manager delete flow is safe and user-friendly

handleManagerDelete:

• prompts via window.confirm,
• respects a boolean removed result from the presenter and shows a specific error toast if the profile cannot be deleted,
• refreshes and shows a success toast on success.

If you later add a shared confirmation dialog component, this would be a good candidate to migrate away from window.confirm for a more consistent look, but functionally this is fine.

---

94-105: Consider localizing CMD, ARGS, and ENV labels

Minor i18n nit: the labels "CMD:", "ARGS:", and "ENV:" in both the builtin and custom sections are currently hard-coded. Everything else in this component goes through t(...), so you might consider adding translation keys for these to keep the UI fully localized.

Also applies to: 170-180

src/renderer/settings/components/AcpProfileManagerDialog.vue (1)

88-123: Typing and env formatting helper look good; optional small i18n enhancement

The props and emits typing around AcpBuiltinAgent, AcpAgentProfile, and AcpBuiltinAgentId are precise and make the component easy to consume from TypeScript.

formatEnv safely handles empty env maps and falls back to settings.acp.none, returning a compact KEY=VALUE list.

Tiny optional suggestion: similar to AcpSettings, if you want full i18n consistency, you could move the literal "ENV:" label in the template into a translation key instead of keeping it hard-coded.

src/renderer/src/stores/modelStore.ts (2)

115-116: Model type normalization is now consistent across code paths

Normalizing model.type with a ModelType.Chat fallback in normalizeRendererModel, applyUserDefinedModelConfig, normalizeStoredModel, and the getModelList mapping makes type handling more robust when upstream metadata omits type. If you find this pattern spreading further, consider centralizing it in a small helper (e.g. resolveModelType(model, fallback?)) to avoid future drift, but what’s here is already safe and clear.

Also applies to: 175-191, 322-323, 376-377

---

218-235: Enabled state update logic is sound; consider taming log noise

updateEnabledState correctly maintains enabledModels per provider and reassigns the ref to trigger reactivity. The added console.log is useful during development but may be noisy in production if refreshProviderModels is called frequently. If logs become too verbose in real usage, consider gating them behind a debug flag or environment check.

src/main/presenter/llmProviderPresenter/agent/acpSessionPersistence.ts (1)

39-87: Session/workdir handling looks solid; consider tightening status typing and fallback path.

The lazy creation in updateWorkdir and the clearSession semantics (resetting sessionId/status but preserving workdir) are coherent and should behave well with the ACP provider.

Two small optional refinements:

• The literal 'idle' is used as an AgentSessionLifecycleStatus value in both updateWorkdir and clearSession. If the status union ever changes, this will drift silently. Consider centralizing it (e.g., a typed constant or enum member) to keep compiler help.
• In getDefaultWorkdir, falling back to process.env.HOME || process.cwd() is fine for tests and most environments, but process.cwd() can be an unexpected location (e.g., install dir) if HOME is missing. You might prefer a more explicit OS‑aware fallback, or at least a comment documenting why cwd is acceptable here.

src/main/presenter/llmProviderPresenter/agent/acpContentMapper.ts (3)

53-99: Content mapping is sensible; consider future‑proofing non‑text content handling.

pushContent/emitAsText give a clear mapping for text vs reasoning and image events/blocks, while safely stringifying audio/resources. That should work fine with current consumers.

If you later surface richer audio/resource UIs, it might be worth:

• Emitting dedicated stream events for audio/resources (similar to image_data), and/or
• Truncating JSON.stringify output for large resource payloads to avoid oversized reasoning/content blocks.

For now this is an acceptable trade‑off.

---

111-159: Tool call lifecycle handling is well‑structured; watch for never‑completed calls.

handleToolCallUpdate plus getOrCreateToolCallState/emitToolCallStartIfNeeded/emitToolCallChunk/emitToolCallEnd provide a clear lifecycle:

• Start event once per (sessionId, toolCallId).
• Reasoning updates on initial call or status change.
• Buffered args via argumentsBuffer + chunk events and blocks.
• Cleanup in emitToolCallEnd by deleting from toolCallStates.

One possible edge case: if a tool call never emits a completed/failed status (e.g., session aborted mid‑call), its entry will remain in toolCallStates until process exit. If ACP sessions can be long‑lived or heavily multiplexed, you might eventually want an explicit cleanup hook (e.g., on session close/timeout) to clear stale tool call state by sessionId.

---

215-236: JSON argument parsing strikes a good balance; optionally distinguish “invalid” vs “incomplete”.

tryParseJsonArguments:

• Trims and early‑returns undefined for empty buffers.
• Only attempts JSON.parse when the buffer looks JSON‑like.
• Logs a warning but still returns the raw string on parse failure, so you don’t lose the payload.

That’s a reasonable compromise. If you want slightly clearer downstream semantics, you could consider returning a tuple like { raw: string; validJson: boolean } or tagging the warning case, so UI or tooling can render “possibly incomplete JSON” differently from plain arguments. Not required, just an option.

src/main/presenter/configPresenter/acpConfHelper.ts (1)

301-352: Legacy agent replacement API is robust; consider sharing more logic with migrateLegacyAgents.

replaceWithLegacyAgents:

• Sanitizes incoming legacy agents via normalizeLegacyAgent.
• Rebuilds builtins/customs from scratch, using BUILTIN_ORDER and default templates.
• Persists normalized arrays and bumps the store version, returning sanitized configs.

This mirrors what migrateLegacyAgents does for the old agents key. If you find yourself evolving the legacy format again, you might want a shared internal helper for the “sanitize + partition into builtins/customs” pattern to avoid drift between these two flows. Behavior is fine as‑is.

src/main/presenter/configPresenter/index.ts (1)

1066-1093: Agent mutation refresh path is pragmatic; consider guarding against noisy refreshes if needed.

refreshAcpProviderAgents:

• Locates the existing acp provider instance through the global presenter.
• Casts to AcpProvider and calls refreshAgents(agentIds) when available.
• Wraps the whole operation in a try/catch with a warn‑level log on failure.

This is a practical way to keep long‑running ACP processes in sync with config changes. If you ever see excessive refresh churn from rapid UI edits, you might later introduce simple debounce or coalescing around agentIds, but that’s an optimization rather than a correctness issue.

src/main/presenter/llmProviderPresenter/providers/acpProvider.ts (3)

109-134: Redundant providerId validation.

Line 114 explicitly sets providerId: this.provider.id, then lines 125-131 validate and re-fix the same field. Since no mutation occurs between these lines, the validation will never trigger. Consider removing the redundant check unless there's evidence of past issues.

Apply this diff to remove the redundant validation:

       const model: MODEL_META = {
         id: agent.id,
         name: agent.name,
         group: 'ACP',
         providerId: this.provider.id, // Ensure providerId is explicitly set
         isCustom: true,
         contextLength: 8192,
         maxTokens: 4096,
         description: agent.command,
         functionCall: true,
         reasoning: false,
         enableSearch: false,
         type: ModelType.Chat
       }
-
-      // Validate that providerId is correctly set
-      if (model.providerId !== this.provider.id) {
-        console.error(
-          `[ACP] fetchProviderModels: Model ${model.id} has incorrect providerId: expected "${this.provider.id}", got "${model.providerId}"`
-        )
-        model.providerId = this.provider.id // Fix it
-      }
-
       return model

---

523-540: Clarify fallback when grant has no matching option.

Lines 534-536: if granted=true but no matching permission option exists, the code defaults to cancelled with a warning. This could confuse callers expecting the grant to succeed. Consider throwing an error instead, or document this edge case clearly.

     const option = this.pickPermissionOption(state.params.options, granted ? 'allow' : 'deny')
     if (option) {
       state.resolve({ outcome: { outcome: 'selected', optionId: option.optionId } })
     } else if (granted) {
-      console.warn('[ACP] No matching permission option for grant, defaulting to cancel')
-      state.resolve({ outcome: { outcome: 'cancelled' } })
+      const error = new Error(`[ACP] No matching allow option for grant request ${requestId}`)
+      console.error(error.message)
+      state.reject(error)
     } else {
       state.resolve({ outcome: { outcome: 'cancelled' } })
     }

---

551-582: Verbose logging may impact performance.

Line 574 logs every chunk from the stream, which could generate excessive logs in production. Consider removing this line or gating it behind a debug/trace flag.

-    for await (const chunk of this.coreStream(
+    for await (const chunk of this.coreStream(
       messages,
       modelId,
       mergedConfig,
       temperature,
       maxTokens,
       []
     )) {
-      console.log('[ACP] collectFromStream: chunk:', chunk)
       if (chunk.type === 'text' && chunk.content) {

src/shared/types/presenters/legacy.presenters.d.ts (1)

675-740: Consider consolidating overlapping types.

AcpAgentConfig (lines 709-715) and AcpCustomAgent (lines 693-700) have nearly identical structure, with AcpCustomAgent adding only an enabled field. This duplication may cause confusion. Consider making AcpAgentConfig a base type or merging them with optional enabled field.

Example consolidation:

export interface AcpAgentConfig {
  id: string
  name: string
  command: string
  args?: string[]
  env?: Record<string, string>
  enabled?: boolean  // Optional, present for custom agents
}

export type AcpCustomAgent = Required<Pick<AcpAgentConfig, 'enabled'>> & AcpAgentConfig

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: zerob13
Repo: ThinkInAIXYZ/deepchat PR: 804
File: src/main/presenter/llmProviderPresenter/providers/tokenfluxProvider.ts:153-156
Timestamp: 2025-08-28T05:55:31.482Z
Learning: TokenFlux models generally support function calling by default, so it's reasonable to assume hasFunctionCalling = true for TokenFlux provider implementations in src/main/presenter/llmProviderPresenter/providers/tokenfluxProvider.ts

Learnt from: neoragex2002
Repo: ThinkInAIXYZ/deepchat PR: 550
File: src/renderer/src/stores/chat.ts:1011-1035
Timestamp: 2025-06-21T15:49:17.044Z
Learning: In src/renderer/src/stores/chat.ts, the user prefers to keep both `text` and `content` properties in the `handleMeetingInstruction` function's `sendMessage` call, even though they are redundant, rather than removing the `content` property.