Conversation
Contributor
📝 WalkthroughWalkthroughReplaces legacy ACP builtins/customs with a registry-first system: adds registry manifest and icon caching, launch-spec resolution and installer, config migration and migration hook, process-launch refactor to use resolved specs, UI and type updates, tests, and prebuild script to fetch the registry. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Settings / Debug)
participant Config as ConfigPresenter
participant Registry as AcpRegistryService
participant LaunchSpec as AcpLaunchSpecService
participant Installer as Installer
participant ProcMgr as AcpProcessManager
participant OS as OS / Child Process
Client->>Config: ensureAcpAgentInstalled(agentId)
Config->>Registry: getAgent(agentId)
Registry-->>Config: AcpRegistryAgent
Config->>LaunchSpec: ensureRegistryAgentInstalled(agent)
LaunchSpec->>Installer: download archive or select runner
Installer->>Installer: validate & extract / install
Installer-->>LaunchSpec: installState (installed | error)
LaunchSpec-->>Config: installState
Config-->>Client: installState
Client->>Config: resolveAcpLaunchSpec(agentId, workdir)
Config->>LaunchSpec: resolveRegistryLaunchSpec(agent, installState)
LaunchSpec-->>Config: AcpResolvedLaunchSpec
Config->>ProcMgr: spawnAgentProcess(agentId, launchSpec, workdir)
ProcMgr->>ProcMgr: prepare env/PATH/cwd from launchSpec + state
ProcMgr->>OS: spawn(command, args, env, cwd)
OS-->>ProcMgr: process started / pid
ProcMgr-->>Config: process handle / status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5883c88849
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+462
to
+467
| const activeProfile = getActiveLegacyProfile(agent) | ||
| registryStates[mappedId] = { | ||
| agentId: mappedId, | ||
| enabled: Boolean(agent.enabled), | ||
| envOverride: normalizeEnv(activeProfile?.env), | ||
| updatedAt: Date.now() |
There was a problem hiding this comment.
When upgrading an existing ACP config, this migration only copies the active profile's env into registryStates and discards the profile's command/args. That means any user who had customized a builtin profile (for example, pointing kimi-cli at a locally installed wrapper or adding required launch flags) is silently switched to the registry launcher, because resolveAcpLaunchSpec() now only knows how to start registry/manual agents. Converting non-default builtin profiles into manual agents would avoid breaking those existing setups.
Useful? React with 👍 / 👎.
Comment on lines
+99
to
+103
| if (platformKey && agent.distribution.binary?.[platformKey]) { | ||
| return { | ||
| type: 'binary', | ||
| targetKey: platformKey, | ||
| binary: agent.distribution.binary[platformKey] |
There was a problem hiding this comment.
For agents that publish both a platform binary and an npx/uvx entry (for example Codex), this always selects the binary as soon as the current platform key exists. If that download/extraction path fails because GitHub/CDN is unavailable, tar is missing, or the archive is corrupted, we never try the package-runner entry even though it may still work, so the agent is left unusable with a permanent install error on an otherwise supported machine.
Useful? React with 👍 / 👎.
| "packageManager": "pnpm@10.13.1+sha512.37ebf1a5c7a30d5fabe0c5df44ee8da4c965ca0c5af3dbab28c3a1681b70a256218d05c81c9c0dcf767ef6b8551eb5b960042b9ed4300c59242336377e01cfad", | ||
| "scripts": { | ||
| "prebuild": "node scripts/fetch-provider-db.mjs", | ||
| "prebuild": "node scripts/fetch-provider-db.mjs && node scripts/fetch-acp-registry.mjs", |
There was a problem hiding this comment.
prebuild working without a live ACP registry fetch
This makes every build depend on fetching the registry from the ACP CDN, but the repo already checks in resources/acp-registry/registry.json and the runtime has a built-in-snapshot fallback in AcpRegistryService. In CI or local environments where that CDN is temporarily unreachable, pnpm build now fails even though a usable snapshot is already present in the tree. Treating the fetch as a best-effort refresh instead of a hard prerequisite would avoid that regression.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/presenter/newAgentPresenter/index.ts (1)
1101-1137:⚠️ Potential issue | 🟠 MajorCanonicalize ACP ids before they enter session state.
These helpers now resolve legacy aliases, but this presenter still stores and later classifies sessions by the raw
agentId. The raw ACP checks at Line 270, Line 786, and Line 908 will still miss ACP-only behavior for sessions created with ids likekimi-clionceproviderIdis absent. Persist the resolved id up front, or route every ACP membership check throughresolveAcpAgentAlias(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/newAgentPresenter/index.ts` around lines 1101 - 1137, The presenter currently mixes raw and canonical ACP agent IDs causing missed ACP detection for aliased IDs; update session creation/storage to use the resolved id returned by resolveAcpAgentAlias(...) so all persisted session state uses canonical ACP ids, and ensure every ACP membership check (including calls in resolveAgentImplementation, assertAcpAgent, and isAcpBackedSession) uses resolveAcpAgentAlias(agentId) before comparing or persisting; alternatively, route all existing ACP checks through a single helper that calls resolveAcpAgentAlias and then checks membership against configPresenter.getAcpAgents() so legacy aliases like "kimi-cli" are normalized consistently.src/renderer/src/i18n/zh-CN/settings.json (1)
1073-1123:⚠️ Potential issue | 🟡 MinorTranslate the new zh-CN ACP copy before release.
Keys like
builtinSectionTitle,debug.title,debug.entry, anddebug.healthCheckare still English, so the Simplified Chinese settings page will ship with mixed-language UI.Also applies to: 1196-1214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/zh-CN/settings.json` around lines 1073 - 1123, Translate the remaining English strings into Simplified Chinese by replacing the untranslated keys with appropriate zh-CN text: update builtinSectionTitle, debug.title, debug.entry, debug.healthCheck (and any other English strings around lines 1196-1214) so the settings page is fully localized; ensure translations follow existing style (concise, UI-friendly) and keep placeholders like {name} and {count} intact and untranslated tokens unchanged.
🟡 Minor comments (12)
src/renderer/src/i18n/zh-HK/settings.json-1220-1222 (1)
1220-1222:⚠️ Potential issue | 🟡 MinorUse zh-HK wording consistently in ACP debug labels.
zh-HKcurrently mixes English/Simplified Chinese here (Health Check,检查中...,健康检查失败). Please convert these to Traditional Chinese (HK-style) for consistency with the rest of the locale file.Suggested wording
- "healthCheck": "Health Check", - "healthChecking": "检查中...", - "healthCheckFailed": "健康检查失败" + "healthCheck": "健康檢查", + "healthChecking": "檢查中...", + "healthCheckFailed": "健康檢查失敗"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/zh-HK/settings.json` around lines 1220 - 1222, The three i18n entries healthCheck, healthChecking, and healthCheckFailed in settings.json currently use English/Simplified Chinese; update their values to Traditional Chinese (Hong Kong style) to match the locale—e.g., replace "Health Check" with a HK-TC phrase such as "系統檢查" (or "健康檢查" if preferred), "检查中..." with "檢查中…", and "健康检查失败" with "健康檢查失敗"; edit the values for the keys healthCheck, healthChecking, and healthCheckFailed accordingly so the file is consistently zh-HK.src/renderer/src/i18n/zh-HK/settings.json-1227-1265 (1)
1227-1265:⚠️ Potential issue | 🟡 MinorACP registry strings are mixed Simplified/Traditional Chinese in
zh-HK.This block introduces multiple Simplified Chinese phrases (for example
个,当前,筛选,仅看,安装中), which is inconsistent for the Hong Kong locale and will look fragmented in UI.Examples to normalize
- "registryCount": "{count} 个 Agent", + "registryCount": "{count} 個 Agent", - "registryEmpty": "当前筛选条件下没有可用的 Registry Agent。", + "registryEmpty": "目前篩選條件下沒有可用的 Registry Agent。", - "registrySearchPlaceholder": "搜索 ACP Registry Agent...", + "registrySearchPlaceholder": "搜尋 ACP Registry Agent...", - "all": "全部状态", + "all": "全部狀態", - "enabled": "仅看已启用", + "enabled": "僅看已啟用", - "installed": "仅看已安装", + "installed": "僅看已安裝", - "attention": "仅看待处理" + "attention": "僅看待處理"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/zh-HK/settings.json` around lines 1227 - 1265, The zh-HK locale block contains mixed Simplified/Traditional Chinese (e.g., "registryCount", "registryEmpty", "registryRefresh", "registrySearchPlaceholder", "filters", "installState", "registryInstallEntry", "registryInstallEntryDescription", "sharedMcpDescription", "installedSectionDescription", "installedCount", "installedEmptyTitle", "installedEmptyDescription", "registryOverlayEmpty", "registryInstallAction", "registryInstallDescription", "installFilters") — convert all Simplified phrases to proper Traditional Chinese consistent with Hong Kong usage (replace characters like 个→個, 当前→當前, 筛选→篩選, 仅看→僅看, 安装中→安裝中, 安装→安裝, 搜尋 wording as needed, etc.), keeping keys unchanged and preserving punctuation and placeholders (e.g., "{count}", "KEY=VALUE") so UI string semantics remain identical.src/renderer/src/stores/ui/agent.ts-46-50 (1)
46-50:⚠️ Potential issue | 🟡 MinorMinor type inconsistency with
installStatenullish coalescing.Line 50 converts
undefinedtonullviaa.installState ?? null, but theUIAgentinterface declaresinstallState?: Agent['installState']which would allowundefined. Ifnullis intentional to distinguish "explicitly no install state" from "unknown", the interface should reflect this:installState?: Agent['installState'] | nullOtherwise, consider preserving
undefined:installState: a.installState🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/stores/ui/agent.ts` around lines 46 - 50, The mapping currently forces installState to null when undefined (a.installState ?? null) but the UIAgent type declares installState?: Agent['installState'] (allowing undefined); either keep the original undefined by returning a.installState from the mapper, or update the UIAgent type to reflect that null is a valid value by changing installState to accept null (e.g. installState?: Agent['installState'] | null). Update whichever you choose consistently: adjust the mapper in the function that builds UIAgent (refer to the object with enabled/icon/description/source/installState) or adjust the UIAgent interface declaration to include | null.src/renderer/src/i18n/ko-KR/settings.json-1220-1265 (1)
1220-1265:⚠️ Potential issue | 🟡 MinorLocalize the new ACP strings before shipping the Korean locale.
This block mixes English and Simplified Chinese copy (
healthCheck*,registryCount,filters.*,installFilters.*, etc.) intoko-KR, so Korean users will see untranslated ACP UI text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ko-KR/settings.json` around lines 1220 - 1265, Several ACP localization keys (e.g., "healthCheck", "healthChecking", "healthCheckFailed", "registryCount", the "filters" object, "installFilters", "registryInstallEntry", "registryInstallEntryDescription", "registryEmpty", etc.) contain English and Simplified Chinese strings in the ko-KR bundle; replace each of these values with the correct Korean translations so the Korean locale is fully localized (update the values for those specific keys only, preserving the JSON keys and structure). Ensure translations cover "healthCheck*", "registryCount", all entries under "filters" and "installFilters", and the registry/install-related keys listed above, then run a quick locale validation to confirm no untranslated strings remain.src/renderer/src/i18n/ru-RU/settings.json-1220-1265 (1)
1220-1265:⚠️ Potential issue | 🟡 MinorLocalize the new ACP strings before shipping the Russian locale.
This block mixes English and Simplified Chinese copy (
healthCheck*,registryCount,filters.*,installFilters.*, etc.) intoru-RU, so Russian users will see untranslated ACP UI text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ru-RU/settings.json` around lines 1220 - 1265, The ru-RU settings JSON contains untranslated strings for ACP features; replace the English and Simplified Chinese values for keys like "healthCheck", "healthChecking", "healthCheckFailed", "registryCount", "registryEmpty", the "filters" object (keys "all","enabled","installed","attention"), the "installFilters" object, and entries such as "registryInstallEntry", "registryInstallEntryDescription", "registrySearchPlaceholder", "registryInstallAction", "registryInstallTitle"/"registryInstallDescription", "registryLearnMore", "registryRepository", "installedSectionTitle"/"installedSectionDescription", "installedCount", "installedEmptyTitle"/"installedEmptyDescription", and "registryOverlayEmpty" with correct Russian translations; ensure pluralization/placeholders (e.g. "{count}") and contextual meaning are preserved and validate the JSON after updating.test/main/presenter/configPresenter/acpLaunchSpecService.test.ts-10-23 (1)
10-23:⚠️ Potential issue | 🟡 MinorClean up the temp directories this test allocates.
tempDirsis populated but never removed, so repeated runs leave junk underos.tmpdir().fs.mkdtempSyncplusfs.rmSynckeeps the test isolated.🧹 Suggested fix
afterEach(() => { + for (const dir of tempDirs) { + fs.rmSync(dir, { recursive: true, force: true }) + } tempDirs.length = 0 vi.restoreAllMocks() }) const createService = () => { - const dir = path.join( - os.tmpdir(), - `deepchat-acp-spec-${Math.random().toString(16).slice(2, 10)}` - ) - fs.mkdirSync(dir, { recursive: true }) + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'deepchat-acp-spec-')) tempDirs.push(dir) return new AcpLaunchSpecService(dir) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/configPresenter/acpLaunchSpecService.test.ts` around lines 10 - 23, The test leaves temporary directories behind; update the test setup/teardown so createService uses fs.mkdtempSync (instead of manual path+mkdirSync) to create a unique temp directory and push it onto tempDirs, and modify afterEach to iterate tempDirs and remove each directory with fs.rmSync(dir, { recursive: true, force: true }) before clearing the array and calling vi.restoreAllMocks(); reference the createService function and the tempDirs variable when making these changes.src/renderer/src/components/icons/ModelIcon.vue-260-285 (1)
260-285:⚠️ Potential issue | 🟡 MinorKeep the
<img>alt text aligned with dynamic agent icons.Once
resolvedIconSrcprefersdynamicAgentIcon, the image still exposesiconKeyto assistive tech, which can degrade todefaultinstead of the actual agent identity. Use the active agent/model id or name when the dynamic source is active.♿ Suggested fix
<img v-else :src="resolvedIconSrc" - :alt="iconKey" + :alt="dynamicAgentIcon && !iconLoadFailed ? props.modelId : iconKey" :class="[customClass, { invert }, invert ? 'opacity-50' : '']" `@error`="handleIconError" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/icons/ModelIcon.vue` around lines 260 - 285, The img alt currently always uses iconKey even when resolvedIconSrc selects dynamicAgentIcon; change to compute and bind an accessible alt that reflects the active source (e.g., create a computed like resolvedAlt that returns props.modelId or model name when dynamicAgentIcon is used, otherwise iconKey) and use that for the <img> :alt binding so assistive tech sees the actual agent/model identity; update references around resolvedIconSrc, dynamicAgentIcon, iconKey, and the <img> alt binding (no changes to handleIconError needed).src/main/presenter/agentPresenter/acp/acpProcessManager.ts-898-903 (1)
898-903:⚠️ Potential issue | 🟡 MinorTrim
launchSpec.cwdbefore using it.Line 898 only trims
launchSpec.cwdfor the truthiness check, then passes the untrimmed value toexistsSync()andspawn(). A value like' /tmp/project 'now falls back toHOME_DIReven though the directory exists. If this field can also contain~or env placeholders, it should be expanded here for the same reason ascommandandargs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentPresenter/acp/acpProcessManager.ts` around lines 898 - 903, The code uses launchSpec.cwd untrimmed for fs.existsSync and spawn which can mis-detect paths like " /tmp/project "; trim launchSpec.cwd into cwd before use and expand tilde/env placeholders the same way command/args are handled so the path is normalized; update the block that sets cwd (referencing launchSpec.cwd, workdir, HOME_DIR, fs.existsSync and the spawn invocation) to 1) compute cwd = trim(launchSpec.cwd) (or fall back to workdir), 2) run the same path expansion routine used for command/args (expand ~ and env vars), then 3) check fs.existsSync on the expanded cwd and only fall back to HOME_DIR if the expanded path truly doesn't exist, and pass the expanded cwd to spawn.src/renderer/src/i18n/da-DK/settings.json-1139-1185 (1)
1139-1185:⚠️ Potential issue | 🟡 MinorLocalize the new ACP registry/debug strings before shipping
da-DK.The added health-check and registry strings here are still Chinese/English, so Danish users will hit mixed-language ACP UI in the new registry flows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/da-DK/settings.json` around lines 1139 - 1185, The Danish locale contains untranslated Chinese/English strings for the ACP registry and health-check keys; update the values for keys such as "healthCheck", "healthChecking", "healthCheckFailed", "registryCount", "registryEmpty", "registryRefresh", "registryRepair", "registrySearchPlaceholder", "registryInstallEntry", "registryInstallEntryDescription", "registryInstallAction", "registryInstallTitle", "registryInstallDescription", "registryLearnMore", "registryRepository" and any other new registry/debug keys to proper Danish translations so the UI is fully localized; keep the JSON keys unchanged and replace only the string values with idiomatic Danish equivalents consistent with other translations in this file.src/renderer/src/i18n/pt-BR/settings.json-1219-1265 (1)
1219-1265:⚠️ Potential issue | 🟡 MinorLocalize the new ACP registry/debug strings before merging
pt-BR.The added copy here is a mix of Chinese and English, so Brazilian Portuguese users will hit untranslated text throughout the new ACP registry and health-check flows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/pt-BR/settings.json` around lines 1219 - 1265, Several strings in the pt-BR locale are still in Chinese/English (e.g., "healthChecking", "healthCheckFailed", "registryCount", "registryEmpty", "registryRefresh", "registryRepair", "registrySearchPlaceholder", "registryInstallEntry", "registryInstallEntryDescription", "registryInstallAction", "registryInstallTitle", "registryInstallDescription", "registryLearnMore", "registryRepository", and keys under "installFilters"); update each of these keys in src/renderer/src/i18n/pt-BR/settings.json to proper Brazilian Portuguese translations consistent with surrounding style, ensuring placeholders like "{count}" remain intact and that "registrySearchPlaceholder" and "envOverridePlaceholder" keep format hints, then run the app or i18n tooling to verify no untranslated strings remain in the ACP registry and health-check flows.src/renderer/settings/components/AcpSettings.vue-641-655 (1)
641-655:⚠️ Potential issue | 🟡 MinorPreview the resolved distribution, not the first binary entry.
buildPreviewCommand()always prefers the firstdistribution.binaryentry, but the main-process launcher picks the current platform's binary or falls back tonpx/uvx. For multi-platform agents, theCMDline here can disagree with what DeepChat will actually start.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/components/AcpSettings.vue` around lines 641 - 655, buildPreviewCommand currently always picks the first entry in agent.distribution.binary which can differ from what the main-process launcher actually chooses; update buildPreviewCommand to resolve the distribution for the current runtime platform first (e.g. pick agent.distribution.binary[platformKey] or the platform-specific selection helper used by the main process) and only fall back to the first binary if no platform-specific binary exists, then fall through to agent.distribution.npx and agent.distribution.uvx as before; reference buildPreviewCommand, agent.distribution.binary, agent.distribution.npx and agent.distribution.uvx when implementing the platform-aware selection so the preview matches the main-process launcher.src/shared/types/presenters/legacy.presenters.d.ts-640-640 (1)
640-640:⚠️ Potential issue | 🟡 MinorRemove
resolveAcpLaunchSpecfrom shared presenter ABI; keep launch-spec resolution internal to main.This method is defined in the shared
IConfigPresenterinterface, making it part of the presenter contract. However, all usages are confined tosrc/main/(configPresenter, acpLaunchSpecService, acpProcessManager). Since presenters are main-process-only (per architecture), the raw spawn details—command,args,env,cwd,installDir—should not be exposed through the shared ABI where they could inadvertently become part of the IPC surface. Move the method to a main-only internal interface or static service; export only a high-level agent-state endpoint via the shared presenter if preload/renderer needs agent metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types/presenters/legacy.presenters.d.ts` at line 640, Remove resolveAcpLaunchSpec from the shared IConfigPresenter ABI: delete its declaration from legacy.presenters.d.ts and move its implementation/usage into a main-process-only internal API (e.g., acpLaunchSpecService or a new internal interface/class used by configPresenter, acpLaunchSpecService, and acpProcessManager). Ensure the shared presenter instead exposes only a high-level agent-state method (e.g., getAgentMetadata or similar) that returns sanitized agent info for preload/renderer, and update callers in src/main/ to call the new internal service rather than the shared IConfigPresenter.resolveAcpLaunchSpec.
🧹 Nitpick comments (3)
src/main/presenter/configPresenter/acpRegistryConstants.ts (1)
10-15: Redundant self-mapping in alias record.Line 13 maps
'codex-acp'to itself, which is a no-op. TheresolveAcpAgentAliasfunction already returns the originalagentIdwhen no mapping exists (via?? agentId), so this entry is unnecessary.🧹 Proposed cleanup
export const ACP_LEGACY_AGENT_ID_ALIASES: Record<string, string> = { 'kimi-cli': 'kimi', 'claude-code-acp': 'claude-acp', - 'codex-acp': 'codex-acp', 'dimcode-acp': 'dimcode' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/acpRegistryConstants.ts` around lines 10 - 15, Remove the redundant self-mapping from the ACP_LEGACY_AGENT_ID_ALIASES constant: delete the "'codex-acp': 'codex-acp'" entry because resolveAcpAgentAlias already returns the original agentId when no mapping exists (via "?? agentId"), so keeping a self-map is unnecessary; update ACP_LEGACY_AGENT_ID_ALIASES accordingly and run tests/linters to ensure no references rely on that explicit entry.scripts/fetch-acp-registry.mjs (1)
7-11: Consider adding a fetch timeout for build reliability.The
fetchcall has no timeout, which could cause the prebuild step to hang indefinitely if the CDN is slow or unresponsive. Consider adding anAbortControllerwith a reasonable timeout.⏱️ Proposed fix to add timeout
const main = async () => { - const response = await fetch(REGISTRY_URL) + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), 30000) + const response = await fetch(REGISTRY_URL, { signal: controller.signal }) + clearTimeout(timeoutId) if (!response.ok) { throw new Error(`Failed to fetch ACP registry: ${response.status} ${response.statusText}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/fetch-acp-registry.mjs` around lines 7 - 11, The fetch in main that calls fetch(REGISTRY_URL) has no timeout; wrap the request with an AbortController: create an AbortController, pass controller.signal to fetch, set a setTimeout to call controller.abort() after a reasonable timeout (e.g., 5–15s), and clear the timeout upon successful response; ensure you catch the abort error and throw or log a clear message (e.g., "Timed out fetching ACP registry") so prebuild doesn't hang. Use the existing main function, REGISTRY_URL, and the fetch call as the insertion points.src/shared/types/presenters/legacy.presenters.d.ts (1)
622-642: Complete the ACP surface migration before finalizing the dual-surface contract.
IConfigPresenternow exposes the new registry/manual ACP surface (lines 622–642), but the legacy ACP methods (getAcpAgents,getAgentMcpSelections,setAgentMcpSelections,addMcpToAgent,removeMcpFromAgent) remain active and are still called throughout the codebase innewAgentPresenter,mcpPresenter,llmProviderPresenter, and renderer components. This dual surface creates technical debt: callers continue using the legacy API, making it harder to retire the old implementation later. Establish a concrete migration plan—either update all call sites to use the new registry API or ensure backward compatibility with a clear transition story.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/types/presenters/legacy.presenters.d.ts` around lines 622 - 642, The code adds the new ACP registry/manual API surface (listAcpRegistryAgents, refreshAcpRegistry, getAcpRegistryIconMarkup, getAcpAgentState, setAcpAgentEnabled, setAcpAgentEnvOverride, ensureAcpAgentInstalled, repairAcpAgent, getAcpAgentInstallStatus, listManualAcpAgents, addManualAcpAgent, updateManualAcpAgent, removeManualAcpAgent, resolveAcpLaunchSpec, getAcpSharedMcpSelections, setAcpSharedMcpSelections) but legacy methods (getAcpAgents, getAgentMcpSelections, setAgentMcpSelections, addMcpToAgent, removeMcpFromAgent) are still used across newAgentPresenter, mcpPresenter, llmProviderPresenter and renderer components; either update every call site to the new registry/manual API or implement a compatibility adapter that implements the legacy method names by delegating to the new functions (mapping results and parameters appropriately) so callers keep working while you finish migration—pick one approach, implement it consistently, and remove legacy exports once all references are updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/acp-registry/registry.json`:
- Around line 218-234: Update the uvx distribution entry to pin the package to
the manifest version: change the "distribution" -> "uvx" -> "package" value for
the crow-cli entry so it includes the version (e.g., "crow-cli==0.1.14" or
"crow-cli@0.1.14") to match the manifest "version": "0.1.14"; locate the
crow-cli object by its "id": "crow-cli" / "name": "crow-cli" and update the
"package" string under "distribution"."uvx"."package" accordingly.
In `@src/main/presenter/configPresenter/acpLaunchSpecService.ts`:
- Around line 342-345: validateTarEntries currently only checks names via
sanitizeRelativePath but does not reject non-regular entries, so a tar
containing symlink/hardlink entries can escape the targetDir when
execFileSync('tar', ['-xf', ...]) runs; update validateTarEntries (and callers)
to first list and parse archive entries (e.g., using 'tar -tvf' or a Node tar
library) and reject any entry whose type is a
symlink/hardlink/device/character/block FIFO (or any type other than regular
file and directory) or whose link target, when resolved, would escape targetDir;
only proceed to execFileSync('tar', ['-xf', archivePath, '-C', targetDir]) after
all entries and any link targets pass sanitizeRelativePath/absolute-resolve
checks so extraction cannot create links outside targetDir.
- Around line 275-276: getBinaryInstallDir currently trusts agent.id and
agent.version from the manifest and joins them directly into the filesystem
path, allowing path traversal (../ or separator chars) to escape installRoot;
fix by normalizing/validating those segments before use—e.g., replace agent.id
and agent.version with safe variants (use path.basename or strip path separators
and reject empty/.. values), then join with this.installRoot, and additionally
assert the resolved path starts with this.installRoot to guard against escapes
in getBinaryInstallDir.
In `@src/main/presenter/configPresenter/acpRegistryMigrationService.ts`:
- Around line 48-50: The current startup compensation loop in
acpRegistryMigrationService checks installState.status and treats both
'installed' and 'installing' as terminal, which prevents retry after a crash;
change the condition to only skip when status === 'installed' so that agents
with status 'installing' are retried, i.e., replace the check using
agent.installState?.status ?? 'not_installed' and the if (status === 'installed'
|| status === 'installing') continue with a single-terminal check if (status ===
'installed') continue so that 'installing' is no longer treated as terminal
during startup compensation.
In `@src/main/presenter/configPresenter/acpRegistryService.ts`:
- Around line 172-185: The manifest processing currently allows duplicate
agent.id values which would cause shared state and unstable Vue keys; in the
function that maps and validates agents (using normalizeAgent and producing the
agents array from record.agents), detect duplicate AcpRegistryAgent.id values
(e.g., build a Set of ids while iterating mapped agents) and if any duplicate id
is found return null (same failure path used for empty version/agents) so the
manifest is rejected; ensure you check after normalization (i.e., on the final
agents array) and reference AcpRegistryAgent.id in your duplicate detection.
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 1184-1206: ensureAcpAgentInstalled (and repairAcpAgent) currently
sets an "installing" state but if
acpLaunchSpecService.ensureRegistryAgentInstalled (or the repair call) throws
the function exits before persisting a terminal failed state; wrap the call to
ensureRegistryAgentInstalled/repair in a try/catch, and in the catch build and
persist a failed AcpAgentInstallState (status: 'failed', error: captured error
message/object, lastCheckedAt: Date.now(), keep installedAt/installDir from
currentState if present) via acpConfHelper.setInstallState(registryAgent.id,
failedState), call notifyAcpAgentsChanged(), then rethrow the error; do the same
symmetrical change in repairAcpAgent so the renderer receives the terminal
failure update.
- Around line 233-235: The acpRegistryService.initialize() call is currently
fire-and-forget and only handles the success path; change it to explicitly
handle failures by attaching a .catch handler (or await inside an async startup
function) to capture initialization errors from acpRegistryService.initialize(),
log the error and take deterministic recovery (e.g., set a failed-initialized
flag, disable ACP-dependent features, or rethrow/exit as appropriate), and only
call notifyAcpAgentsChanged() on success; reference
acpRegistryService.initialize() and notifyAcpAgentsChanged() when making the
change.
In `@src/main/presenter/sqlitePresenter/index.ts`:
- Around line 568-570: When renaming agent aliases inside the loop over entries
(the for (const [from, to] of entries) block) handle UNIQUE(conversation_id,
agent_id) collisions on acp_sessions before calling this.db.prepare('UPDATE
acp_sessions...'). Add logic to detect rows where a conversation already has an
entry with agent_id = to and also has one with agent_id = from, and delete or
merge the "from" row (or otherwise resolve the conflict) prior to performing the
UPDATE; then run the existing this.db.prepare('UPDATE acp_sessions SET agent_id
= ? WHERE agent_id = ?').run(to, from). Ensure you reference the acp_sessions
table and the entries loop when implementing the pre-check/cleanup so the UPDATE
no longer violates the unique constraint.
In `@src/renderer/settings/components/AcpDebugDialog.vue`:
- Around line 544-572: The code assigns debugSessionId from
newSessionResult.sessionId but then immediately issues a cancel using
debugSessionId.value, which either cancels a placeholder or leaves
debugSessionId pointing at a just-canceled session; fix by using a local
newSessionId variable to store newSessionResult.sessionId, pass that local id
into runAcpDebugAction for the cancel call (instead of debugSessionId.value),
and only assign debugSessionId.value = newSessionId if you did not cancel the
session (or if cancelResult.status !== 'success'); ensure appendEvents uses
cancelResult.events as before and avoid persisting a canceled session id in
debugSessionId.
In `@src/renderer/settings/components/AcpSettings.vue`:
- Around line 280-282: The delete button currently calls
deleteManualAgent(agent) directly, allowing accidental immediate deletion;
restore a confirmation step by checking the existing settings flag
settings.acp.customDeleteConfirm (and prompt via the same confirm dialog flow
used elsewhere) before invoking deleteManualAgent; modify the click handler on
the Button that renders the delete action (the instance calling
deleteManualAgent) to call a small wrapper method (e.g.,
confirmAndDeleteManualAgent) which shows the confirmation UI when
settings.acp.customDeleteConfirm is true and only calls deleteManualAgent(agent)
after user confirms, and apply the same change to the other delete button
instance referenced in the file (the block around the other call at lines
~919-926).
In `@src/renderer/src/i18n/en-US/settings.json`:
- Around line 1095-1096: The en-US locale contains duplicate ACP keys and
Chinese fallback text: remove the duplicated Chinese entries that redeclare
"registryRefresh" and "registryRepair" (keep the earlier English values) and
replace any remaining Chinese strings in the en-US JSON with their proper
English translations so the file contains a single English definition per key
(e.g., ensure "registryRefresh" and "registryRepair" only appear once with
English text and update any other Chinese fallback strings in that block to
English).
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Around line 1219-1265: The fa-IR locale file contains Chinese strings that
must be replaced with Persian (or English fallback) for keys including
"healthChecking" and "healthCheckFailed", "registryCount", "registryEmpty",
"registryRefresh", "registryRepair", "registrySearchPlaceholder", the entire
"filters" object ("all","enabled","installed","attention"), the "installState"
object ("installed","installing","error","notInstalled"), and registry/install
related keys
("registryInstallEntry","registryInstallEntryDescription","registryInstallAction","registryInstallTitle","registryInstallDescription","registryLearnMore","registryRepository","installFilters").
Update those values to correct Persian translations (or neutral English) while
preserving the JSON keys and placeholders like {count} and KEY=VALUE formatting.
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Around line 1220-1222: The three i18n entries "healthCheck", "healthChecking",
and "healthCheckFailed" contain non-French text (English and Chinese); update
their values in the fr-FR settings JSON to proper French translations (e.g.,
"healthCheck" -> "Vérification de l'état", "healthChecking" -> "Vérification en
cours...", "healthCheckFailed" -> "Échec de la vérification de l'état") so the
French locale file contains only French strings.
- Around line 1227-1265: Replace the incorrect Chinese and English strings in
the French locale JSON by providing proper French translations for the listed
keys: registryCount, registryEmpty, registryRefresh, registryRepair,
registrySearchPlaceholder, filters.all/enabled/installed/attention,
envOverrideTitle, envOverridePlaceholder,
installState.installed/installing/error/notInstalled, registryInstallEntry,
registryInstallEntryDescription, sharedMcpTitle, sharedMcpDescription,
installedSectionTitle, installedSectionDescription, installedCount,
installedEmptyTitle, installedEmptyDescription, registryOverlayEmpty,
registryInstallAction, registryInstallTitle, registryInstallDescription,
registryLearnMore, registryRepository, and
installFilters.all/installed/notInstalled; ensure you preserve template
placeholders (e.g., {count}, KEY=VALUE) and key names exactly, use idiomatic
French for UI text, and keep capitalization consistent with other locale
entries.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Around line 1193-1239: The he-IL locale contains Chinese/English strings for
keys such as "healthChecking", "healthCheckFailed", "registryCount",
"registryEmpty", "registryRefresh", "registryRepair",
"registrySearchPlaceholder", the "filters" object
("all","enabled","installed","attention"), "installFilters"
("all","installed","notInstalled"), "registryInstallEntry",
"registryInstallEntryDescription", "registryInstallAction" and related
registry/install strings—replace each of these with proper Hebrew translations
(right-to-left friendly phrasing and punctuation), preserving the exact key
names and placeholders like "{count}" and "KEY=VALUE", and ensure
consistency/clarity with surrounding Hebrew translations.
In `@src/renderer/src/i18n/ja-JP/settings.json`:
- Around line 1219-1265: Replace all Simplified Chinese and English strings in
the ja-JP locale entries with proper Japanese translations: update keys like
"healthChecking" to "チェック中...", "healthCheckFailed" to Japanese text,
"filters.all"/"enabled"/"installed"/"attention" to Japanese equivalents,
"installState.installed"/"installing"/"error"/"notInstalled" to Japanese (e.g.,
"インストール済み", "インストール中", "インストール失敗", "未インストール"), and translate other entries such
as "registryCount", "registryEmpty", "registryRefresh", "registryRepair",
"registrySearchPlaceholder", "registryInstallEntry",
"registryInstallEntryDescription", "sharedMcpTitle", "sharedMcpDescription",
"installedSectionTitle", "installedSectionDescription", "installedCount",
"installedEmptyTitle", "installedEmptyDescription", "registryOverlayEmpty",
"registryInstallAction", "registryInstallTitle", "registryInstallDescription",
"registryLearnMore", "registryRepository", and
"installFilters.all/installed/notInstalled" into natural Japanese so the ja-JP
file contains only proper Japanese translations for those keys.
In `@src/renderer/src/i18n/zh-TW/settings.json`:
- Around line 1220-1245: The new entries in the zh-TW locale use Simplified
Chinese; update the values for keys "healthChecking", "healthCheckFailed",
"registryCount", and "registryEmpty" (and any other newly added keys in the
shown block like "healthCheck") to use Traditional Chinese characters (e.g.,
"检查中..." → "檢查中...", "健康检查失败" → "健康檢查失敗", "{count} 个 Agent" → "{count} 個 Agent",
and convert "当前筛选条件下没有可用的 Registry Agent。" to its Traditional equivalent),
ensuring consistency with the surrounding Traditional Chinese translations.
---
Outside diff comments:
In `@src/main/presenter/newAgentPresenter/index.ts`:
- Around line 1101-1137: The presenter currently mixes raw and canonical ACP
agent IDs causing missed ACP detection for aliased IDs; update session
creation/storage to use the resolved id returned by resolveAcpAgentAlias(...) so
all persisted session state uses canonical ACP ids, and ensure every ACP
membership check (including calls in resolveAgentImplementation, assertAcpAgent,
and isAcpBackedSession) uses resolveAcpAgentAlias(agentId) before comparing or
persisting; alternatively, route all existing ACP checks through a single helper
that calls resolveAcpAgentAlias and then checks membership against
configPresenter.getAcpAgents() so legacy aliases like "kimi-cli" are normalized
consistently.
In `@src/renderer/src/i18n/zh-CN/settings.json`:
- Around line 1073-1123: Translate the remaining English strings into Simplified
Chinese by replacing the untranslated keys with appropriate zh-CN text: update
builtinSectionTitle, debug.title, debug.entry, debug.healthCheck (and any other
English strings around lines 1196-1214) so the settings page is fully localized;
ensure translations follow existing style (concise, UI-friendly) and keep
placeholders like {name} and {count} intact and untranslated tokens unchanged.
---
Minor comments:
In `@src/main/presenter/agentPresenter/acp/acpProcessManager.ts`:
- Around line 898-903: The code uses launchSpec.cwd untrimmed for fs.existsSync
and spawn which can mis-detect paths like " /tmp/project "; trim launchSpec.cwd
into cwd before use and expand tilde/env placeholders the same way command/args
are handled so the path is normalized; update the block that sets cwd
(referencing launchSpec.cwd, workdir, HOME_DIR, fs.existsSync and the spawn
invocation) to 1) compute cwd = trim(launchSpec.cwd) (or fall back to workdir),
2) run the same path expansion routine used for command/args (expand ~ and env
vars), then 3) check fs.existsSync on the expanded cwd and only fall back to
HOME_DIR if the expanded path truly doesn't exist, and pass the expanded cwd to
spawn.
In `@src/renderer/settings/components/AcpSettings.vue`:
- Around line 641-655: buildPreviewCommand currently always picks the first
entry in agent.distribution.binary which can differ from what the main-process
launcher actually chooses; update buildPreviewCommand to resolve the
distribution for the current runtime platform first (e.g. pick
agent.distribution.binary[platformKey] or the platform-specific selection helper
used by the main process) and only fall back to the first binary if no
platform-specific binary exists, then fall through to agent.distribution.npx and
agent.distribution.uvx as before; reference buildPreviewCommand,
agent.distribution.binary, agent.distribution.npx and agent.distribution.uvx
when implementing the platform-aware selection so the preview matches the
main-process launcher.
In `@src/renderer/src/components/icons/ModelIcon.vue`:
- Around line 260-285: The img alt currently always uses iconKey even when
resolvedIconSrc selects dynamicAgentIcon; change to compute and bind an
accessible alt that reflects the active source (e.g., create a computed like
resolvedAlt that returns props.modelId or model name when dynamicAgentIcon is
used, otherwise iconKey) and use that for the <img> :alt binding so assistive
tech sees the actual agent/model identity; update references around
resolvedIconSrc, dynamicAgentIcon, iconKey, and the <img> alt binding (no
changes to handleIconError needed).
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Around line 1139-1185: The Danish locale contains untranslated Chinese/English
strings for the ACP registry and health-check keys; update the values for keys
such as "healthCheck", "healthChecking", "healthCheckFailed", "registryCount",
"registryEmpty", "registryRefresh", "registryRepair",
"registrySearchPlaceholder", "registryInstallEntry",
"registryInstallEntryDescription", "registryInstallAction",
"registryInstallTitle", "registryInstallDescription", "registryLearnMore",
"registryRepository" and any other new registry/debug keys to proper Danish
translations so the UI is fully localized; keep the JSON keys unchanged and
replace only the string values with idiomatic Danish equivalents consistent with
other translations in this file.
In `@src/renderer/src/i18n/ko-KR/settings.json`:
- Around line 1220-1265: Several ACP localization keys (e.g., "healthCheck",
"healthChecking", "healthCheckFailed", "registryCount", the "filters" object,
"installFilters", "registryInstallEntry", "registryInstallEntryDescription",
"registryEmpty", etc.) contain English and Simplified Chinese strings in the
ko-KR bundle; replace each of these values with the correct Korean translations
so the Korean locale is fully localized (update the values for those specific
keys only, preserving the JSON keys and structure). Ensure translations cover
"healthCheck*", "registryCount", all entries under "filters" and
"installFilters", and the registry/install-related keys listed above, then run a
quick locale validation to confirm no untranslated strings remain.
In `@src/renderer/src/i18n/pt-BR/settings.json`:
- Around line 1219-1265: Several strings in the pt-BR locale are still in
Chinese/English (e.g., "healthChecking", "healthCheckFailed", "registryCount",
"registryEmpty", "registryRefresh", "registryRepair",
"registrySearchPlaceholder", "registryInstallEntry",
"registryInstallEntryDescription", "registryInstallAction",
"registryInstallTitle", "registryInstallDescription", "registryLearnMore",
"registryRepository", and keys under "installFilters"); update each of these
keys in src/renderer/src/i18n/pt-BR/settings.json to proper Brazilian Portuguese
translations consistent with surrounding style, ensuring placeholders like
"{count}" remain intact and that "registrySearchPlaceholder" and
"envOverridePlaceholder" keep format hints, then run the app or i18n tooling to
verify no untranslated strings remain in the ACP registry and health-check
flows.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Around line 1220-1265: The ru-RU settings JSON contains untranslated strings
for ACP features; replace the English and Simplified Chinese values for keys
like "healthCheck", "healthChecking", "healthCheckFailed", "registryCount",
"registryEmpty", the "filters" object (keys
"all","enabled","installed","attention"), the "installFilters" object, and
entries such as "registryInstallEntry", "registryInstallEntryDescription",
"registrySearchPlaceholder", "registryInstallAction",
"registryInstallTitle"/"registryInstallDescription", "registryLearnMore",
"registryRepository", "installedSectionTitle"/"installedSectionDescription",
"installedCount", "installedEmptyTitle"/"installedEmptyDescription", and
"registryOverlayEmpty" with correct Russian translations; ensure
pluralization/placeholders (e.g. "{count}") and contextual meaning are preserved
and validate the JSON after updating.
In `@src/renderer/src/i18n/zh-HK/settings.json`:
- Around line 1220-1222: The three i18n entries healthCheck, healthChecking, and
healthCheckFailed in settings.json currently use English/Simplified Chinese;
update their values to Traditional Chinese (Hong Kong style) to match the
locale—e.g., replace "Health Check" with a HK-TC phrase such as "系統檢查" (or
"健康檢查" if preferred), "检查中..." with "檢查中…", and "健康检查失败" with "健康檢查失敗"; edit the
values for the keys healthCheck, healthChecking, and healthCheckFailed
accordingly so the file is consistently zh-HK.
- Around line 1227-1265: The zh-HK locale block contains mixed
Simplified/Traditional Chinese (e.g., "registryCount", "registryEmpty",
"registryRefresh", "registrySearchPlaceholder", "filters", "installState",
"registryInstallEntry", "registryInstallEntryDescription",
"sharedMcpDescription", "installedSectionDescription", "installedCount",
"installedEmptyTitle", "installedEmptyDescription", "registryOverlayEmpty",
"registryInstallAction", "registryInstallDescription", "installFilters") —
convert all Simplified phrases to proper Traditional Chinese consistent with
Hong Kong usage (replace characters like 个→個, 当前→當前, 筛选→篩選, 仅看→僅看, 安装中→安裝中,
安装→安裝, 搜尋 wording as needed, etc.), keeping keys unchanged and preserving
punctuation and placeholders (e.g., "{count}", "KEY=VALUE") so UI string
semantics remain identical.
In `@src/renderer/src/stores/ui/agent.ts`:
- Around line 46-50: The mapping currently forces installState to null when
undefined (a.installState ?? null) but the UIAgent type declares installState?:
Agent['installState'] (allowing undefined); either keep the original undefined
by returning a.installState from the mapper, or update the UIAgent type to
reflect that null is a valid value by changing installState to accept null (e.g.
installState?: Agent['installState'] | null). Update whichever you choose
consistently: adjust the mapper in the function that builds UIAgent (refer to
the object with enabled/icon/description/source/installState) or adjust the
UIAgent interface declaration to include | null.
In `@src/shared/types/presenters/legacy.presenters.d.ts`:
- Line 640: Remove resolveAcpLaunchSpec from the shared IConfigPresenter ABI:
delete its declaration from legacy.presenters.d.ts and move its
implementation/usage into a main-process-only internal API (e.g.,
acpLaunchSpecService or a new internal interface/class used by configPresenter,
acpLaunchSpecService, and acpProcessManager). Ensure the shared presenter
instead exposes only a high-level agent-state method (e.g., getAgentMetadata or
similar) that returns sanitized agent info for preload/renderer, and update
callers in src/main/ to call the new internal service rather than the shared
IConfigPresenter.resolveAcpLaunchSpec.
In `@test/main/presenter/configPresenter/acpLaunchSpecService.test.ts`:
- Around line 10-23: The test leaves temporary directories behind; update the
test setup/teardown so createService uses fs.mkdtempSync (instead of manual
path+mkdirSync) to create a unique temp directory and push it onto tempDirs, and
modify afterEach to iterate tempDirs and remove each directory with
fs.rmSync(dir, { recursive: true, force: true }) before clearing the array and
calling vi.restoreAllMocks(); reference the createService function and the
tempDirs variable when making these changes.
---
Nitpick comments:
In `@scripts/fetch-acp-registry.mjs`:
- Around line 7-11: The fetch in main that calls fetch(REGISTRY_URL) has no
timeout; wrap the request with an AbortController: create an AbortController,
pass controller.signal to fetch, set a setTimeout to call controller.abort()
after a reasonable timeout (e.g., 5–15s), and clear the timeout upon successful
response; ensure you catch the abort error and throw or log a clear message
(e.g., "Timed out fetching ACP registry") so prebuild doesn't hang. Use the
existing main function, REGISTRY_URL, and the fetch call as the insertion
points.
In `@src/main/presenter/configPresenter/acpRegistryConstants.ts`:
- Around line 10-15: Remove the redundant self-mapping from the
ACP_LEGACY_AGENT_ID_ALIASES constant: delete the "'codex-acp': 'codex-acp'"
entry because resolveAcpAgentAlias already returns the original agentId when no
mapping exists (via "?? agentId"), so keeping a self-map is unnecessary; update
ACP_LEGACY_AGENT_ID_ALIASES accordingly and run tests/linters to ensure no
references rely on that explicit entry.
In `@src/shared/types/presenters/legacy.presenters.d.ts`:
- Around line 622-642: The code adds the new ACP registry/manual API surface
(listAcpRegistryAgents, refreshAcpRegistry, getAcpRegistryIconMarkup,
getAcpAgentState, setAcpAgentEnabled, setAcpAgentEnvOverride,
ensureAcpAgentInstalled, repairAcpAgent, getAcpAgentInstallStatus,
listManualAcpAgents, addManualAcpAgent, updateManualAcpAgent,
removeManualAcpAgent, resolveAcpLaunchSpec, getAcpSharedMcpSelections,
setAcpSharedMcpSelections) but legacy methods (getAcpAgents,
getAgentMcpSelections, setAgentMcpSelections, addMcpToAgent, removeMcpFromAgent)
are still used across newAgentPresenter, mcpPresenter, llmProviderPresenter and
renderer components; either update every call site to the new registry/manual
API or implement a compatibility adapter that implements the legacy method names
by delegating to the new functions (mapping results and parameters
appropriately) so callers keep working while you finish migration—pick one
approach, implement it consistently, and remove legacy exports once all
references are updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b16774a0-4d40-409f-8dfa-0cfbbb5558d1
📒 Files selected for processing (43)
package.jsonresources/acp-registry/registry.jsonscripts/fetch-acp-registry.mjssrc/main/presenter/agentPresenter/acp/acpProcessManager.tssrc/main/presenter/configPresenter/acpConfHelper.tssrc/main/presenter/configPresenter/acpLaunchSpecService.tssrc/main/presenter/configPresenter/acpRegistryConstants.tssrc/main/presenter/configPresenter/acpRegistryMigrationService.tssrc/main/presenter/configPresenter/acpRegistryService.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/lifecyclePresenter/hooks/after-start/acpRegistryMigrationHook.tssrc/main/presenter/lifecyclePresenter/hooks/index.tssrc/main/presenter/llmProviderPresenter/providers/acpProvider.tssrc/main/presenter/newAgentPresenter/index.tssrc/main/presenter/sqlitePresenter/index.tssrc/renderer/settings/components/AcpDebugDialog.vuesrc/renderer/settings/components/AcpSettings.vuesrc/renderer/src/components/icons/AcpAgentIcon.vuesrc/renderer/src/components/icons/ModelIcon.vuesrc/renderer/src/components/mcp-config/AgentMcpSelector.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/agentModelStore.tssrc/renderer/src/stores/ui/agent.tssrc/shadcn/components/ui/dialog/DialogContent.vuesrc/shared/types/agent-interface.d.tssrc/shared/types/presenters/legacy.presenters.d.tstest/main/presenter/agentPresenter/acp/acpProcessManager.test.tstest/main/presenter/configPresenter/acpConfHelper.test.tstest/main/presenter/configPresenter/acpLaunchSpecService.test.tstest/main/presenter/configPresenter/acpRegistryMigrationService.test.tstest/renderer/components/AcpAgentIcon.test.tstest/renderer/components/ModelIcon.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/main/presenter/configPresenter/acpRegistryConstants.ts (1)
1-29: Consider moving the registry URL helpers intosrc/shared.
ACP_REGISTRY_ICON_PREFIXandisAcpRegistryIconUrl()now have renderer-side duplicates inAcpAgentIcon.vueandModelIcon.vue, plus similar logic in the fetch script. A shared helper would keep main and renderer aligned the next time the registry URL shape changes.As per coding guidelines, "Organize shared TypeScript types and utilities in
src/shared/for use across main, preload, and renderer processes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/acpRegistryConstants.ts` around lines 1 - 29, Move the registry URL helpers out of acpRegistryConstants.ts into a new shared utility module and update references: extract ACP_REGISTRY_ICON_PREFIX and isAcpRegistryIconUrl into src/shared (export both), then update imports in AcpAgentIcon.vue, ModelIcon.vue, and the fetch script to import the shared ACP_REGISTRY_ICON_PREFIX and isAcpRegistryIconUrl instead of re-defining them; keep existing names (ACP_REGISTRY_ICON_PREFIX, isAcpRegistryIconUrl) and ensure their behavior stays identical so other symbols in this file (e.g., ACP_REGISTRY_URL, getAcpRegistryIconFileName, resolveAcpAgentAlias) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/fetch-acp-registry.mjs`:
- Around line 24-39: Remote agent.id values are used directly in path.join when
writing SVGs (see iconAgents, ICON_TMP_DIR and the fs.writeFile call), allowing
path traversal or absolute paths; validate/sanitize agent.id before using it —
implement a check that rejects empty values and any id containing path
separators or traversal sequences (e.g. '/', '\\', '..') or restrict to a safe
filename pattern like /^[A-Za-z0-9._-]+$/; alternatively compute a safeName via
path.basename(agent.id) and ensure it equals the original id (or else throw),
then use that safeName when writing to path.join(ICON_TMP_DIR,
`${safeName}.svg`) to prevent escaping ICON_TMP_DIR.
In `@src/renderer/settings/components/AcpSettings.vue`:
- Around line 635-639: The dialog is corrupting argv because openManualDialog()
flattens agent.args with join(' ') and saveManualAgent() reparses with
parseArgs(), which splits quoted items (e.g., "--prompt", "hello world" =>
"--prompt", "hello", "world"); instead preserve the argument array in the dialog
state (e.g., store args as string[] in the component's reactive state) or switch
to a lossless serializer/parser pair (e.g., JSON.stringify/JSON.parse or a
shell-quote library) so openManualDialog(), saveManualAgent(), and parseArgs()
operate on a stable representation; update the dialog binding and save logic to
read/write the array directly (or use the chosen serializer) rather than
join/split with spaces.
- Around line 145-156: Replace the hardcoded metadata labels ("ID", "VER",
"CMD", "ARGS", "MCP") in the AcpSettings.vue template with vue-i18n keys from
the app i18n namespace, so they translate with the rest of the settings screen;
update the label spans near the agent display (the elements showing agent.id,
agent.version and buildPreviewCommand(agent)) and the other occurrences
referenced in the comment (the other label blocks around the ARGS/MCP display
and the later repeated blocks) to use the appropriate $t(...)/t(...) keys from
src/renderer/src/i18n instead of plain text, keeping the surrounding markup and
bindings (e.g., agent.id, agent.version, buildPreviewCommand(agent)) unchanged.
- Around line 643-657: buildPreviewCommand currently always appends
formatArgs(...) which returns a localized "none" placeholder when args are
empty, producing previews like "npx -y pkg None"; update buildPreviewCommand to
only append the formatted args when the args array is non-empty (check e.g.
firstBinary.args.length, agent.distribution.npx.args.length, and
agent.distribution.uvx.args.length) so that for binary/npx/uvx branches you only
add ` ${formatArgs(...)} ` when there are actual args; keep using formatArgs for
non-empty cases and omit it entirely for empty lists to avoid the "none"
placeholder.
In `@src/renderer/src/components/icons/AcpAgentIcon.vue`:
- Around line 47-52: The computed shouldRenderImage currently becomes true for
registry URLs until svgMarkup is populated, causing the component to render the
raw CDN <img> and bypass the inline/fallback handling; update shouldRenderImage
to also check isThemeableRegistryIcon and return false for themeable registry
icons (e.g. change to: Boolean(props.icon.trim()) &&
!shouldRenderInlineSvg.value && !isThemeableRegistryIcon.value) so registry SVGs
remain on the inline/fallback path; apply the same change to the other identical
computed (lines ~125-132) that uses the same logic.
- Around line 7-8: The current iconMarkupCache (Map<string, Promise<string |
null>>) caches misses and is keyed only by agentId, which can pin stale or null
markup; change the caching strategy in the functions that fetch icon markup
(referencing iconMarkupCache and agentId) to: use a cache keyed by a composite
key including agentId plus the icon identifier (e.g., `${agentId}:${icon}`),
store only in-flight Promises (Promise<string>) while the network request is
pending, and on resolution store the string only if non-empty; on rejection or
empty result remove the cache entry so future requests can retry; ensure you
never permanently memoize null/empty results and remove the entry after the
Promise settles on error or empty response to implement safe in-flight dedupe
rather than a session-long miss cache.
---
Nitpick comments:
In `@src/main/presenter/configPresenter/acpRegistryConstants.ts`:
- Around line 1-29: Move the registry URL helpers out of acpRegistryConstants.ts
into a new shared utility module and update references: extract
ACP_REGISTRY_ICON_PREFIX and isAcpRegistryIconUrl into src/shared (export both),
then update imports in AcpAgentIcon.vue, ModelIcon.vue, and the fetch script to
import the shared ACP_REGISTRY_ICON_PREFIX and isAcpRegistryIconUrl instead of
re-defining them; keep existing names (ACP_REGISTRY_ICON_PREFIX,
isAcpRegistryIconUrl) and ensure their behavior stays identical so other symbols
in this file (e.g., ACP_REGISTRY_URL, getAcpRegistryIconFileName,
resolveAcpAgentAlias) remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67cb1f0e-e65a-4ee2-9ed1-27b8f3e3ef49
⛔ Files ignored due to path filters (28)
resources/acp-registry/icons/amp-acp.svgis excluded by!**/*.svgresources/acp-registry/icons/auggie.svgis excluded by!**/*.svgresources/acp-registry/icons/autohand.svgis excluded by!**/*.svgresources/acp-registry/icons/claude-acp.svgis excluded by!**/*.svgresources/acp-registry/icons/cline.svgis excluded by!**/*.svgresources/acp-registry/icons/codebuddy-code.svgis excluded by!**/*.svgresources/acp-registry/icons/codex-acp.svgis excluded by!**/*.svgresources/acp-registry/icons/corust-agent.svgis excluded by!**/*.svgresources/acp-registry/icons/crow-cli.svgis excluded by!**/*.svgresources/acp-registry/icons/cursor.svgis excluded by!**/*.svgresources/acp-registry/icons/deepagents.svgis excluded by!**/*.svgresources/acp-registry/icons/dimcode.svgis excluded by!**/*.svgresources/acp-registry/icons/factory-droid.svgis excluded by!**/*.svgresources/acp-registry/icons/fast-agent.svgis excluded by!**/*.svgresources/acp-registry/icons/gemini.svgis excluded by!**/*.svgresources/acp-registry/icons/github-copilot-cli.svgis excluded by!**/*.svgresources/acp-registry/icons/goose.svgis excluded by!**/*.svgresources/acp-registry/icons/junie.svgis excluded by!**/*.svgresources/acp-registry/icons/kilo.svgis excluded by!**/*.svgresources/acp-registry/icons/kimi.svgis excluded by!**/*.svgresources/acp-registry/icons/minion-code.svgis excluded by!**/*.svgresources/acp-registry/icons/mistral-vibe.svgis excluded by!**/*.svgresources/acp-registry/icons/nova.svgis excluded by!**/*.svgresources/acp-registry/icons/opencode.svgis excluded by!**/*.svgresources/acp-registry/icons/pi-acp.svgis excluded by!**/*.svgresources/acp-registry/icons/qoder.svgis excluded by!**/*.svgresources/acp-registry/icons/qwen-code.svgis excluded by!**/*.svgresources/acp-registry/icons/stakpak.svgis excluded by!**/*.svg
📒 Files selected for processing (10)
scripts/fetch-acp-registry.mjssrc/main/presenter/configPresenter/acpRegistryConstants.tssrc/main/presenter/configPresenter/acpRegistryService.tssrc/main/presenter/configPresenter/index.tssrc/renderer/settings/components/AcpSettings.vuesrc/renderer/src/components/icons/AcpAgentIcon.vuesrc/renderer/src/components/icons/ModelIcon.vuesrc/shared/types/presenters/legacy.presenters.d.tstest/main/presenter/configPresenter/acpRegistryService.test.tstest/renderer/components/AcpAgentIcon.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/renderer/components/AcpAgentIcon.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/presenter/configPresenter/acpRegistryService.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/main/presenter/configPresenter/index.ts (2)
1329-1356: Unused_workdirparameter may need future implementation.The
_workdirparameter is explicitly marked as unused but is passed by callers (as shown in the relevant code snippets). If workdir-scoped resolution is planned, consider adding a TODO comment to clarify intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/index.ts` around lines 1329 - 1356, The parameter _workdir in resolveAcpLaunchSpec(agentId: string, _workdir?: string) is unused but callers pass a workdir; to avoid confusion add a brief TODO comment inside resolveAcpLaunchSpec (near the signature or top of the function) stating that workdir-scoped resolution is not yet implemented and will be used for future scoped installs, or implement workdir handling now if intended; reference resolveAcpLaunchSpec and the _workdir parameter so reviewers can locate and either document the planned behavior or wire the workdir into getRegistryAgentOrThrow/resolveRegistryLaunchSpec as appropriate.
1167-1177: Consider consolidating preinstall error handling.The fire-and-forget
ensureAcpAgentInstalledat line 1173 logs a warning on failure but doesn't update any UI state to indicate the preinstall attempt failed. SincesetAcpAgentEnabledreturns successfully, the user may not realize installation failed until they try to use the agent.This is acceptable for a background preinstall, but consider whether the UI should reflect a "pending install" state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/index.ts` around lines 1167 - 1177, The current fire-and-forget call to ensureAcpAgentInstalled inside setAcpAgentEnabled logs failures but doesn't update UI state; change setAcpAgentEnabled to mark the agent as "install pending" (via acpConfHelper or the existing handleAcpAgentsMutated flow) before calling ensureAcpAgentInstalled, and in the catch handler update the agent state to "install failed" (and call handleAcpAgentsMutated with the resolvedId) so the presenter/UI can reflect pending/failed install states; keep the call asynchronous but ensure both success and error paths update state via handleAcpAgentsMutated (and/or a new acpConfHelper.setAgentInstallStatus method).src/main/presenter/configPresenter/acpRegistryService.ts (3)
510-528: Verify icon cache pruning handles edge cases correctly.The pruning logic extracts
agentIdby removing the last 4 characters (.svg). This assumes agent IDs don't contain periods and always use the.svgextension. Consider usingpath.parse()for more robust filename parsing.♻️ Safer filename parsing
for (const file of files) { if (!file.endsWith('.svg')) { continue } - const agentId = file.slice(0, -4) + const agentId = path.parse(file).name if (expectedAgentIds.has(agentId)) { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/acpRegistryService.ts` around lines 510 - 528, In pruneStaleIconCache, the current agentId extraction uses file.slice(0, -4) which breaks on filenames with extra dots; instead use path.parse(file) to robustly check the extension and get the base name: verify path.parse(file).ext === '.svg' then use path.parse(file).name as agentId before comparing against expectedAgentIds, and keep the same fs.rmSync(path.join(this.iconCacheDir, file), { force: true }) behavior; update the try/catch block accordingly in the pruneStaleIconCache method.
223-231: Simplify redundant initialization logic.The initialization logic is confusing: line 224 sets
this.manifestbut lines 226-228 immediately overwrite it if cache exists, making the built-in fallback on line 224 only effective when cache doesn't exist.♻️ Proposed simplification
async initialize(): Promise<void> { - this.manifest = this.loadFromBuiltIn() ?? this.loadFromCache() - const cached = this.loadFromCache() - if (cached) { - this.manifest = cached - } + // Prefer cached manifest over built-in; built-in serves as fallback + this.manifest = this.loadFromCache() ?? this.loadFromBuiltIn() await this.refreshIfNeeded(false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/acpRegistryService.ts` around lines 223 - 231, The initialize method sets this.manifest redundantly; simplify by trying cache first and falling back to built-in: call loadFromCache() once, assign its result to this.manifest if present, otherwise assign loadFromBuiltIn(); remove the initial unconditional assignment to loadFromBuiltIn() and the duplicate second call to loadFromCache(); keep the await this.refreshIfNeeded(false) call. Target the initialize method and the loadFromBuiltIn/loadFromCache/refreshIfNeeded calls when making this change.
351-353: Usenet.fetchinstead of globalfetchfor consistency with the rest of the file.Line 351 uses the global
fetchwhilesyncAgentIconat line 488 uses Electron'snet.fetch. In Electron's main process,net.fetchprovides better integration with the network stack, including proper proxy support and system proxy configuration.♻️ Proposed fix
try { - const response = await fetch(ACP_REGISTRY_URL, { + const response = await net.fetch(ACP_REGISTRY_URL, { signal: controller.signal })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/acpRegistryService.ts` around lines 351 - 353, Replace the global fetch call with Electron's net.fetch to match the rest of the file: use net.fetch(ACP_REGISTRY_URL, { signal: controller.signal }) instead of fetch(ACP_REGISTRY_URL, {...}), import or reference net from 'electron' where other code (e.g., syncAgentIcon) does, and adjust any response handling to use the Electron net response shape (statusCode, on('data')/on('end') or response.arrayBuffer()/text() if available) so behavior stays equivalent to the original fetch-based flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/configPresenter/acpLaunchSpecService.ts`:
- Around line 334-346: downloadArchive creates a tempDir and writes archivePath
but never removes tempDir on failure or after successful extraction; update
downloadArchive to always clean up the tempDir on error (e.g., wrap
download/write in try/catch and remove tempDir on failure) and update
ensureRegistryAgentInstalled (the caller that calls extractArchive and deletes
archivePath) to also remove the parent tempDir after successful
extraction/removal of archivePath so the temporary directory is not leaked;
reference tempDir and archivePath variables and the extractArchive and
ensureRegistryAgentInstalled call sites so cleanup happens both on exception and
on the normal path.
In `@test/main/presenter/configPresenter/acpLaunchSpecService.test.ts`:
- Around line 10-13: The test suite tracks temporary directories in the tempDirs
array (populated by createService) but afterEach only resets the array length
without deleting them; update the afterEach teardown to iterate over tempDirs
and remove each directory (use fs.rmSync or fs.rm with { recursive: true, force:
true } or equivalent) while guarding with fs.existsSync and swallowing/logging
errors, then clear tempDirs and call vi.restoreAllMocks(); reference the
tempDirs variable, the createService helper that pushes dirs, and the existing
afterEach block when making the change.
---
Nitpick comments:
In `@src/main/presenter/configPresenter/acpRegistryService.ts`:
- Around line 510-528: In pruneStaleIconCache, the current agentId extraction
uses file.slice(0, -4) which breaks on filenames with extra dots; instead use
path.parse(file) to robustly check the extension and get the base name: verify
path.parse(file).ext === '.svg' then use path.parse(file).name as agentId before
comparing against expectedAgentIds, and keep the same
fs.rmSync(path.join(this.iconCacheDir, file), { force: true }) behavior; update
the try/catch block accordingly in the pruneStaleIconCache method.
- Around line 223-231: The initialize method sets this.manifest redundantly;
simplify by trying cache first and falling back to built-in: call
loadFromCache() once, assign its result to this.manifest if present, otherwise
assign loadFromBuiltIn(); remove the initial unconditional assignment to
loadFromBuiltIn() and the duplicate second call to loadFromCache(); keep the
await this.refreshIfNeeded(false) call. Target the initialize method and the
loadFromBuiltIn/loadFromCache/refreshIfNeeded calls when making this change.
- Around line 351-353: Replace the global fetch call with Electron's net.fetch
to match the rest of the file: use net.fetch(ACP_REGISTRY_URL, { signal:
controller.signal }) instead of fetch(ACP_REGISTRY_URL, {...}), import or
reference net from 'electron' where other code (e.g., syncAgentIcon) does, and
adjust any response handling to use the Electron net response shape (statusCode,
on('data')/on('end') or response.arrayBuffer()/text() if available) so behavior
stays equivalent to the original fetch-based flow.
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 1329-1356: The parameter _workdir in resolveAcpLaunchSpec(agentId:
string, _workdir?: string) is unused but callers pass a workdir; to avoid
confusion add a brief TODO comment inside resolveAcpLaunchSpec (near the
signature or top of the function) stating that workdir-scoped resolution is not
yet implemented and will be used for future scoped installs, or implement
workdir handling now if intended; reference resolveAcpLaunchSpec and the
_workdir parameter so reviewers can locate and either document the planned
behavior or wire the workdir into
getRegistryAgentOrThrow/resolveRegistryLaunchSpec as appropriate.
- Around line 1167-1177: The current fire-and-forget call to
ensureAcpAgentInstalled inside setAcpAgentEnabled logs failures but doesn't
update UI state; change setAcpAgentEnabled to mark the agent as "install
pending" (via acpConfHelper or the existing handleAcpAgentsMutated flow) before
calling ensureAcpAgentInstalled, and in the catch handler update the agent state
to "install failed" (and call handleAcpAgentsMutated with the resolvedId) so the
presenter/UI can reflect pending/failed install states; keep the call
asynchronous but ensure both success and error paths update state via
handleAcpAgentsMutated (and/or a new acpConfHelper.setAgentInstallStatus
method).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eae7bd8d-8d21-4c10-b60e-54510423ac6e
📒 Files selected for processing (24)
resources/acp-registry/registry.jsonscripts/fetch-acp-registry.mjssrc/main/presenter/configPresenter/acpLaunchSpecService.tssrc/main/presenter/configPresenter/acpRegistryConstants.tssrc/main/presenter/configPresenter/acpRegistryMigrationService.tssrc/main/presenter/configPresenter/acpRegistryService.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/newAgentPresenter/legacyImportService.tssrc/main/presenter/sqlitePresenter/index.tssrc/renderer/settings/components/AcpDebugDialog.vuesrc/renderer/settings/components/AcpSettings.vuesrc/renderer/src/components/icons/AcpAgentIcon.vuesrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsontest/main/presenter/configPresenter/acpLaunchSpecService.test.tstest/main/presenter/configPresenter/acpRegistryMigrationService.test.tstest/main/presenter/configPresenter/acpRegistryService.test.tstest/main/presenter/newAgentPresenter/legacyImportService.test.tstest/main/presenter/sqlitePresenter.test.tstest/renderer/components/AcpAgentIcon.test.ts
✅ Files skipped from review due to trivial changes (6)
- test/main/presenter/sqlitePresenter.test.ts
- src/renderer/src/i18n/fa-IR/settings.json
- src/renderer/src/i18n/he-IL/settings.json
- resources/acp-registry/registry.json
- test/main/presenter/configPresenter/acpRegistryService.test.ts
- src/renderer/src/i18n/zh-TW/settings.json
🚧 Files skipped from review as they are similar to previous changes (8)
- scripts/fetch-acp-registry.mjs
- src/renderer/src/i18n/fr-FR/settings.json
- test/main/presenter/configPresenter/acpRegistryMigrationService.test.ts
- src/renderer/src/i18n/ja-JP/settings.json
- src/main/presenter/configPresenter/acpRegistryConstants.ts
- src/main/presenter/configPresenter/acpRegistryMigrationService.ts
- src/renderer/src/i18n/en-US/settings.json
- src/renderer/settings/components/AcpDebugDialog.vue
Comment on lines
+334
to
+346
| private async downloadArchive(url: string, agent: AcpRegistryAgent): Promise<string> { | ||
| const safeAgentId = sanitizeInstallSegment(agent.id, 'agent id') | ||
| const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), `deepchat-acp-${safeAgentId}-`)) | ||
| const archivePath = path.join(tempDir, path.basename(new URL(url).pathname)) | ||
| const response = await fetch(url) | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to download archive: ${response.status} ${response.statusText}`) | ||
| } | ||
|
|
||
| const arrayBuffer = await response.arrayBuffer() | ||
| fs.writeFileSync(archivePath, Buffer.from(arrayBuffer)) | ||
| return archivePath | ||
| } |
Contributor
There was a problem hiding this comment.
Temp directory not cleaned up on download/extraction failure.
If downloadArchive succeeds but extractArchive fails (line 187), the temp directory created at line 336 is leaked. The archivePath is deleted on success (line 188) but the parent temp directory remains.
🧹 Proposed fix to clean up temp directory
private async downloadArchive(url: string, agent: AcpRegistryAgent): Promise<string> {
const safeAgentId = sanitizeInstallSegment(agent.id, 'agent id')
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), `deepchat-acp-${safeAgentId}-`))
const archivePath = path.join(tempDir, path.basename(new URL(url).pathname))
- const response = await fetch(url)
- if (!response.ok) {
- throw new Error(`Failed to download archive: ${response.status} ${response.statusText}`)
+ try {
+ const response = await fetch(url)
+ if (!response.ok) {
+ throw new Error(`Failed to download archive: ${response.status} ${response.statusText}`)
+ }
+
+ const arrayBuffer = await response.arrayBuffer()
+ fs.writeFileSync(archivePath, Buffer.from(arrayBuffer))
+ return archivePath
+ } catch (error) {
+ fs.rmSync(tempDir, { recursive: true, force: true })
+ throw error
}
-
- const arrayBuffer = await response.arrayBuffer()
- fs.writeFileSync(archivePath, Buffer.from(arrayBuffer))
- return archivePath
}Then in ensureRegistryAgentInstalled, also clean the temp dir's parent after extraction:
const archivePath = await this.downloadArchive(binaryConfig.archive, agent)
- await this.extractArchive(archivePath, installDir)
- fs.rmSync(archivePath, { force: true })
+ const archiveTempDir = path.dirname(archivePath)
+ try {
+ await this.extractArchive(archivePath, installDir)
+ } finally {
+ fs.rmSync(archiveTempDir, { recursive: true, force: true })
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/configPresenter/acpLaunchSpecService.ts` around lines 334
- 346, downloadArchive creates a tempDir and writes archivePath but never
removes tempDir on failure or after successful extraction; update
downloadArchive to always clean up the tempDir on error (e.g., wrap
download/write in try/catch and remove tempDir on failure) and update
ensureRegistryAgentInstalled (the caller that calls extractArchive and deletes
archivePath) to also remove the parent tempDir after successful
extraction/removal of archivePath so the temporary directory is not leaked;
reference tempDir and archivePath variables and the extractArchive and
ensureRegistryAgentInstalled call sites so cleanup happens both on exception and
on the normal path.
Comment on lines
+10
to
+13
| afterEach(() => { | ||
| tempDirs.length = 0 | ||
| vi.restoreAllMocks() | ||
| }) |
Contributor
There was a problem hiding this comment.
Temp directories are tracked but never cleaned up.
The tempDirs array is populated in createService but afterEach only resets the array length without actually removing the directories. This leaves test artifacts in the system temp directory.
🧹 Proposed fix to clean up temp directories
afterEach(() => {
+ for (const dir of tempDirs) {
+ try {
+ fs.rmSync(dir, { recursive: true, force: true })
+ } catch {
+ // ignore cleanup errors
+ }
+ }
tempDirs.length = 0
vi.restoreAllMocks()
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/main/presenter/configPresenter/acpLaunchSpecService.test.ts` around
lines 10 - 13, The test suite tracks temporary directories in the tempDirs array
(populated by createService) but afterEach only resets the array length without
deleting them; update the afterEach teardown to iterate over tempDirs and remove
each directory (use fs.rmSync or fs.rm with { recursive: true, force: true } or
equivalent) while guarding with fs.existsSync and swallowing/logging errors,
then clear tempDirs and call vi.restoreAllMocks(); reference the tempDirs
variable, the createService helper that pushes dirs, and the existing afterEach
block when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests