fix: avoid unnecessary project plugin dependencies - #13300
Conversation
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 4ff1cbb)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4ff1cbb)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Reviewed by grok-4.6 · Input: 116.3K · Output: 19.1K · Cached: 948.9K Review guidance: REVIEW.md from base branch |
| // kilocode_change - Auto-discovered plugins under config directories are already local files, so ConfigPlugin.load | ||
| // returns normalized Specs and we only need to attach origin metadata here. | ||
| const list = yield* Effect.promise(() => ConfigPlugin.load(dir)) | ||
| plugins.push(...list) // kilocode_change |
There was a problem hiding this comment.
The comment is not necessarily to the line i commented but more towards the entire file.
Would be great if we can extract parts of this code in upstream path into a kilo owned path (to minimize upstream conflicts on merge). Could you check if this can be ported into a helper at packages/opencode/src/kilocode/config/plugin-deps.ts for example, then changes in this file reduce and you can have a minimal call to the helper. It's good that tests are already in kilo path. Can you address that?
zipRight, Effect.fork and Fiber.poll are gone in effect 4.0.0-beta.83, so typecheck failed on this file. Uses andThen, forkChild and a bounded join.
|
Pushed 10ad635 to fix
Moved to the v4 equivalents:
|
|
thanks for addressing the comment @mvanhorn. Can you take a look at the other comment I posted too? |
Per review, extract the local plugin dependency install out of the upstream config.ts and into packages/opencode/src/kilocode/config/plugin-deps.ts so the upstream file keeps a minimal call and future merges have less to conflict on. needsLocalPluginDependency owns the file:// specifier test and installLocalPluginDependency owns the npm install effect, taking the npm service, dir, version and local flag as parameters. Behavior is unchanged: the plugins accumulator still collects both the file-loaded specs and the ConfigPlugin.load results before the check, and the resulting fiber is still pushed onto deps.
|
Done in 738577d. Added The upstream file drops from 23 lines to 5: // kilocode_change start
if (needsLocalPluginDependency(plugins)) {
deps.push(yield* installLocalPluginDependency(npmSvc, dir, InstallationVersion, InstallationLocal))
}
// kilocode_change endThat also let the Behaviour is unchanged: the
The |
|
Thanks for addressing the comments! merged. |
Context
Adjust the config-directory loading loop in
packages/opencode/src/config/config.tsso dependency installation is scheduled only after the directory's config files and auto-discovered plugins have been inspected, and only when that directory contributes a file-based plugin that needs the local@kilocode/pluginruntime/types. Preserve the existing detached install fiber, warning behavior, andwaitForDependencieswiring for directories that do require the dependency; npm package plugins continue to resolve through the shared package cache, while built-in plugins must not trigger a project-local install. Add focused Kilo regression coverage inpackages/opencode/test/kilocode/config/config.test.tsthat observes the real config-loading decision for plugin-free and plugin-bearing directories without introducing a production helper used only by tests.Fixes #12778
Implementation
Not applicable to this change.
Screenshots
How to Test
Not applicable to this change.
Get in Touch
Not applicable to this change.
AI was used for assistance.