Fix Settings plugin installs registering unloadable directory paths#1428
Conversation
Plugins installed from Settings → Built-in Plugins registered the manifest directory as the plugin path, but since FN-4128 the loader requires a loadable entry FILE (Node ESM cannot import directories), so enabling failed with "Plugin entry must be a file, got directory". Only the CLI startup path had been migrated to entry-file resolution, which is why CLI-auto-installed plugins worked and Settings installs never did. - Add resolvePluginEntryPath (bundled.js → dist/index.js → src/index.ts) to @fusion/core; the CLI keeps its local copy (its test fs mocks don't reach externalized core) with sync comments both ways. - Register the resolved entry file in both dashboard install routes; 400 with a clear message when a package has no loadable entry. - Heal legacy directory-path registrations on enable, mirroring the CLI's startup heal, so existing broken rows recover from the UI without a restart. - Route tests: assert installs register entry files, cover the enable-route heal, and update existing install tests to the entry-file contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… main) into plugin install fixes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPlugin installation and enablement now resolve and register the loadable entry file (bundled.js, dist/index.js, or src/index.ts) instead of the package directory. A new ChangesPlugin entry file resolution and dashboard integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes a regression introduced in FN-4128 where both dashboard install routes registered the package directory rather than a concrete loadable entry file, causing every Settings-initiated plugin enable to fail with "Plugin entry must be a file, got directory." The fix resolves and persists the real entry file (
Confidence Score: 5/5Safe to merge. The core bug fix — registering an entry file instead of a directory at install time and healing legacy directory rows on enable — is correctly implemented and tested on both surfaces. The implementation is straightforward and consistent: both install routes now call resolvePluginEntryPath before persisting the path, both enable routes heal legacy directory rows in place, and the drift-guard test ensures the CLI and core copies of the helper stay in sync. No incorrect data is persisted, no existing contract is broken, and the heal is fail-soft. The only gap is missing install-handler entry-file tests for createPluginRouter specifically, which does not affect correctness since the same function and code structure is used. packages/dashboard/src/tests/plugin-routes.test.ts — the createPluginRouter install handler's dist/index.js, src/index.ts, and no-entry-file branches are untested Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Settings UI
participant RT as routes.ts / createPluginRouter
participant RPM as resolvePluginManifest
participant REP as resolvePluginEntryPath
participant PS as PluginStore
participant PL as PluginLoader
UI->>RT: "POST /api/plugins {mode:install, path}"
RT->>RPM: resolvePluginManifest(path)
RPM-->>RT: "{manifestDir, manifest}"
RT->>REP: resolvePluginEntryPath(manifestDir)
alt bundled.js / dist/index.js / src/index.ts found
REP-->>RT: entryFilePath
RT->>PS: "registerPlugin({path: entryFilePath})"
PS-->>RT: plugin
RT-->>UI: "201 {plugin}"
else no loadable entry
REP-->>RT: null
RT-->>UI: 400 no loadable entry file
end
UI->>RT: POST /api/plugins/:id/enable
RT->>PS: enablePlugin(id)
PS-->>RT: plugin (path may be legacy dir)
RT->>RT: stat(plugin.path).isDirectory()?
alt path is directory (legacy registration)
RT->>REP: resolvePluginEntryPath(plugin.path)
REP-->>RT: entryFilePath
RT->>PS: "updatePlugin(id, {path: entryFilePath})"
PS-->>RT: healed plugin
end
RT->>PL: loadPlugin(id)
PL-->>RT: ok
RT-->>UI: "200 {plugin}"
Reviews (5): Last reviewed commit: "Fix quality-backfill suites broken by fa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dashboard/src/__tests__/plugin-routes.test.ts`:
- Around line 283-285: The tests currently only simulate a single existing entry
("bundled.js") via mockExistsSync and assert only that the stored path contains
the plugin id; update the suite to fully exercise the entry-resolution contract
by (1) tightening existing bundled-plugin assertions to require a filename
suffix (e.g., ensure the resolved path endsWith a file like ".js" or ".ts" not
just a directory and still contains the plugin id), (2) extend mockExistsSync
behavior in the relevant cases to return true for the other supported candidates
("dist/index.js" and "src/index.ts") and add separate assertions verifying those
exact file endings are chosen, and (3) add a test case for the "manifest exists
but no loadable entry file" scenario where mockExistsSync returns false for all
candidates and assert the 400/no-entry failure path is triggered; focus your
edits around the mockExistsSync setup and the affected tests in
plugin-routes.test.ts so each surface (bundled.js, dist/index.js, src/index.ts,
and no-entry) is explicitly asserted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a0dd587d-fe09-48c3-93c6-62c86f233e99
📒 Files selected for processing (8)
.changeset/workflow-graph-editor-and-bundled-plugins.mddocs/solutions/integration-issues/bundled-plugin-registration-drift.mdpackages/cli/src/plugins/bundled-plugin-install.tspackages/core/src/index.tspackages/core/src/plugin-loader.tspackages/dashboard/src/__tests__/plugin-routes.test.tspackages/dashboard/src/plugin-routes.tspackages/dashboard/src/routes.ts
- Add heal block to createPluginRouter's enable handler so it matches routes.ts (directory-path registrations re-pointed at entry files) - Test the 400 "no loadable entry file" install branch - Add a real-fs drift-guard test asserting the CLI and @fusion/core copies of resolvePluginEntryPath resolve identically Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both addressed in 7dde43a: createPluginRouter's |
…1428) - Bundled fallback assertions for dependency-graph/reports now require the bundled.js entry-file suffix instead of just containing the id - Add route-level fallback cases for dist/index.js and src/index.ts Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/dashboard/src/__tests__/plugin-routes.test.ts (1)
520-524:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGood progress on entry-resolution coverage, but gaps remain.
The new test at lines 770-788 addresses the no-entry-file case—nice! However, the remaining gaps from the earlier review still need attention:
Lines 520-524 and 552-556: These bundled-plugin assertions use
stringContaining(plugin-id)rather than a file-suffix pattern. A directory-path regression would still pass. Tighten tostringMatching(/plugin-id[\\/]bundled\.js$/)or similar.Missing fallback coverage: The entry-resolution contract checks
bundled.js,dist/index.js, andsrc/index.tsin order, but no tests exercise the latter two. Add narrow cases wheremockExistsSyncreturns false forbundled.jsbut true fordist/index.js(and similarly forsrc/index.ts), then assert the registered path ends with the chosen candidate.As per coding guidelines, "Regression tests must assert the general invariant across ALL known surfaces, not only the single reported reproduction (FN-5893)".
Also applies to: 552-556, 770-788
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/src/__tests__/plugin-routes.test.ts` around lines 520 - 524, Tighten the bundled-plugin path assertions and add fallback tests: replace the existing expect(pluginStore.registerPlugin).toHaveBeenCalledWith(expect.objectContaining({ path: expect.stringContaining("fusion-plugin-dependency-graph") })) checks with a regex-based assertion such as expect.stringMatching(/fusion-plugin-dependency-graph[\\/](?:bundled\.js|dist[\\/]index\.js|src[\\/]index\.(ts|js))$/) to ensure the candidate file suffix is asserted; add two new narrow tests that stub/mock mockExistsSync to return false for bundled.js but true for dist/index.js (and a second test false for both bundled.js and dist/index.js but true for src/index.ts) and assert pluginStore.registerPlugin was called with a manifest path matching the expected chosen candidate using the same stringMatching pattern; keep references to pluginStore.registerPlugin and mockExistsSync so the changes are localized in the existing plugin-routes.test.ts tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/dashboard/src/__tests__/plugin-routes.test.ts`:
- Around line 520-524: Tighten the bundled-plugin path assertions and add
fallback tests: replace the existing
expect(pluginStore.registerPlugin).toHaveBeenCalledWith(expect.objectContaining({
path: expect.stringContaining("fusion-plugin-dependency-graph") })) checks with
a regex-based assertion such as
expect.stringMatching(/fusion-plugin-dependency-graph[\\/](?:bundled\.js|dist[\\/]index\.js|src[\\/]index\.(ts|js))$/)
to ensure the candidate file suffix is asserted; add two new narrow tests that
stub/mock mockExistsSync to return false for bundled.js but true for
dist/index.js (and a second test false for both bundled.js and dist/index.js but
true for src/index.ts) and assert pluginStore.registerPlugin was called with a
manifest path matching the expected chosen candidate using the same
stringMatching pattern; keep references to pluginStore.registerPlugin and
mockExistsSync so the changes are localized in the existing
plugin-routes.test.ts tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aed725ee-13b9-407d-a611-a16c0dd8cd39
📒 Files selected for processing (3)
packages/cli/src/plugins/__tests__/resolve-plugin-entry-path-sync.test.tspackages/dashboard/src/__tests__/plugin-routes.test.tspackages/dashboard/src/plugin-routes.ts
…tails - Expand the directory-path follow-up section: PR #1428, 400 no-entry branch, heal-on-enable in both routers - Document the vitest fs-mock vs externalized workspace-dep trap and the real-fs drift-guard test pattern - Add Plugin Entry to CONCEPTS.md Plugins cluster Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…erge race Main went red when the fast-tests quality-backfill projects (PR #1385) landed alongside the workflow-columns stream (PR #1424) — the new test projects were written against pre-stream code: - TaskFieldsSection.css toggle knob used background: #fff, violating the theme-token assertion in AgentListModal's styling-parity test; use var(--card) per the SkillsView toggle convention - ListView.test.tsx api mock lacked fetchBoardWorkflows (TaskDetailModal now calls it on mount) - chat.test.ts and routes-agent-import.test.ts @fusion/core mocks lacked registerTraitHookImpl (engine merge-trait registers hooks at import) - auto-merge-toggle-blank.mobile and board-mobile-initial-render used vi.runAllTimers(), which never terminates now that sse-bus starts a keepalive setInterval; use vi.runOnlyPendingTimers() Both quality-backfill projects now pass fully: 7151/7151 across 414 files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Follow-up to #1423. Enabling a plugin installed from Settings → Built-in Plugins failed with "Plugin entry must be a file, got directory: …"; those plugins now load. The install routes registered the manifest directory as the plugin path, but since FN-4128 the loader requires a loadable entry file (Node ESM cannot import directories) — only the CLI startup path had been migrated to entry-file resolution, which is why CLI-auto-installed plugins worked while every Settings install broke on enable.
The dashboard install routes now resolve and register the entry file (
bundled.js→dist/index.js→src/index.ts, helper added to@fusion/core), return a clear 400 when a package ships no loadable entry, and the enable route heals legacy directory-path registrations in place — mirroring the CLI's startup heal — so already-broken rows recover from the UI without a restart or reinstall.Route tests now assert installs register entry files and cover the enable-route heal; existing install tests were updated to the entry-file contract (their old mocks let the directory contract pass silently).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests