Conversation
Add plugin host runtime and migrate codex composer
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
github-actions
bot
added
size:XXL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| } | ||
| }; | ||
|
|
||
| const reloadAllPlugins = async (preferredIds?: string[]) => { |
There was a problem hiding this comment.
🟡 Medium plugins/manager.ts:329
Concurrent calls to reloadAllPlugins race and corrupt state: overlapping executions can double-unload plugins, clear pluginStates while another call is iterating, and install duplicate watchers. This happens because scheduleFullReload and per-plugin reload timers can fire simultaneously with different keys, and there is no mechanism to serialize reload operations. Consider adding a guard (e.g., a promise or flag) to ensure only one reload executes at a time, with subsequent calls either awaiting the in-progress reload or queuing.
Also found in 1 other location(s)
apps/web/src/plugins/host.tsx:222
Concurrent invocations of
loadPlugins(triggered by multipleonRegistryUpdatedevents or rapid effect re-runs) share the samecancelledflag but run independently. Both concurrent calls will proceed past theif (cancelled)checks and load the same plugins, causing duplicate entries incomposerProvidersRef.currentandloadedPluginsRef.current. The first invocation unregisters all plugins, then both start loading, leading to each plugin being registered twice.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/plugins/manager.ts around line 329:
Concurrent calls to `reloadAllPlugins` race and corrupt state: overlapping executions can double-unload plugins, clear `pluginStates` while another call is iterating, and install duplicate watchers. This happens because `scheduleFullReload` and per-plugin reload timers can fire simultaneously with different keys, and there is no mechanism to serialize reload operations. Consider adding a guard (e.g., a promise or flag) to ensure only one reload executes at a time, with subsequent calls either awaiting the in-progress reload or queuing.
Evidence trail:
apps/server/src/plugins/manager.ts lines 253-263 (scheduleFullReload with rootWatchTimers), lines 310-319 (per-plugin reload with reloadTimers), lines 329-351 (reloadAllPlugins async function with no lock/mutex), lines 143-151 (clearWatchers and watchers array), lines 149-157 (unloadPlugin with cleanup tasks). git_grep for locking mechanisms (reloadLock, reloadMutex, reloadInProgress, isReloading) returned no results.
Also found in 1 other location(s):
- apps/web/src/plugins/host.tsx:222 -- Concurrent invocations of `loadPlugins` (triggered by multiple `onRegistryUpdated` events or rapid effect re-runs) share the same `cancelled` flag but run independently. Both concurrent calls will proceed past the `if (cancelled)` checks and load the same plugins, causing duplicate entries in `composerProvidersRef.current` and `loadedPluginsRef.current`. The first invocation unregisters all plugins, then both start loading, leading to each plugin being registered twice.
| function extractHeading(markdown: string): string | undefined { | ||
| const heading = /^#\s+(.+)$/m.exec(markdown)?.[1]?.trim(); | ||
| return heading && heading.length > 0 ? heading : undefined; | ||
| } |
There was a problem hiding this comment.
🟢 Low src/prompts.ts:39
extractHeading searches the entire markdown string including frontmatter, so a YAML comment like # some note in the frontmatter block is incorrectly returned as the document heading instead of the actual markdown heading that appears after ---. Consider anchoring the regex to exclude frontmatter, e.g. by stripping the frontmatter block before matching or ensuring the heading appears after the frontmatter delimiter.
+function extractHeading(markdown: string): string | undefined {
+ const withoutFrontmatter = markdown.replace(/^---\n[\s\S]*?\n---\n?/, '');
+ const heading = /^#\s+(.+)$/m.exec(withoutFrontmatter)?.[1]?.trim();
+ return heading && heading.length > 0 ? heading : undefined;
+}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/prompts.ts around lines 39-42:
`extractHeading` searches the entire markdown string including frontmatter, so a YAML comment like `# some note` in the frontmatter block is incorrectly returned as the document heading instead of the actual markdown heading that appears after `---`. Consider anchoring the regex to exclude frontmatter, e.g. by stripping the frontmatter block before matching or ensuring the heading appears after the frontmatter delimiter.
Evidence trail:
apps/server/src/prompts.ts lines 28-29 (extractFrontmatter), lines 39-41 (extractHeading using /^#\s+(.+)$/m on full markdown), lines 44-46 (parsePromptMarkdown passes full input.markdown to extractHeading without stripping frontmatter). Commit: REVIEWED_COMMIT.
| function normalizePluginRoots(cwd: string): string[] { | ||
| const configuredRoots = (process.env[PLUGINS_ENV_VAR] ?? "") | ||
| .split(path.delimiter) | ||
| .map((value) => value.trim()) | ||
| .filter((value) => value.length > 0) | ||
| .map((value) => path.resolve(value)); | ||
| const localRoot = path.resolve(cwd, DEFAULT_LOCAL_PLUGINS_DIR); | ||
| return Array.from(new Set([localRoot, ...configuredRoots])); |
There was a problem hiding this comment.
🟡 Medium plugins/discovery.ts:45
Line 50 resolves relative paths from the environment variable against process.cwd() rather than the cwd parameter. When a caller passes a cwd different from process.cwd(), relative paths in the environment variable resolve inconsistently with localRoot which correctly uses path.resolve(cwd, DEFAULT_LOCAL_PLUGINS_DIR). Consider changing path.resolve(value) to path.resolve(cwd, value) so all paths resolve relative to the same base.
- .map((value) => path.resolve(value));
+ .map((value) => path.resolve(cwd, value));🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/plugins/discovery.ts around lines 45-52:
Line 50 resolves relative paths from the environment variable against `process.cwd()` rather than the `cwd` parameter. When a caller passes a `cwd` different from `process.cwd()`, relative paths in the environment variable resolve inconsistently with `localRoot` which correctly uses `path.resolve(cwd, DEFAULT_LOCAL_PLUGINS_DIR)`. Consider changing `path.resolve(value)` to `path.resolve(cwd, value)` so all paths resolve relative to the same base.
Evidence trail:
apps/server/src/plugins/discovery.ts lines 44-52 (REVIEWED_COMMIT): `normalizePluginRoots(cwd: string)` function shows line 50 `.map((value) => path.resolve(value))` resolving env var paths against process.cwd(), while line 51 `path.resolve(cwd, DEFAULT_LOCAL_PLUGINS_DIR)` resolves localRoot against the cwd parameter. apps/server/src/plugins/manager.ts line 337: `discoverPluginRoots(input.cwd)` shows the function is called with a cwd parameter that may differ from process.cwd().
1 participant
Summary
skills.listfallback for$and/skillscomposer suggestionsVerification
Note
Fix skill autocomplete fallback by introducing a plugin host and composer bridge for skills, prompts, and slash commands
T3CODE_PLUGIN_DIRSand a localplugins/dir, activates them, validates procedure I/O via Effect schemas, serves web bundles at/__plugins/:id/web.js, and hot-reloads on file changes.PluginHostProvider(host.tsx) that dynamically imports enabled plugin web modules, registers composer providers and slot renderers, and exposes them via React context withusePluginComposerItemsandPluginSlot.composerBridge(composerBridge.ts) withbuildComposerMenuItemsthat merges plugin-provided items with built-in skills, prompts, paths, and models, applying scoring and deduplication per trigger kind.detectComposerTrigger(composer-logic.ts) to recognize/workspace,/skills, and$<token>(skill-mention) trigger kinds, and makes the generic/<query>path returnslash-commandwithout a fixed allowlist.skills.listandprompts.listWS methods and aplugins.registryUpdatedpush channel to the protocol (ws.ts), backed bylistAvailableSkillsandlistAvailablePromptson the server.codex-composerplugin (plugins/codex-composer/) that provides workspace and skill picker slash commands.ProjectSearchEntriesInputschema now accepts emptyquerystrings; callers that relied on non-empty validation will silently pass.📊 Macroscope summarized bd06126. 34 files reviewed, 9 issues evaluated, 1 issue filtered, 3 comments posted
🗂️ Filtered Issues
apps/web/src/plugins/host.tsx — 0 comments posted, 3 evaluated, 1 filtered
loadPlugins(triggered by multipleonRegistryUpdatedevents or rapid effect re-runs) share the samecancelledflag but run independently. Both concurrent calls will proceed past theif (cancelled)checks and load the same plugins, causing duplicate entries incomposerProvidersRef.currentandloadedPluginsRef.current. The first invocation unregisters all plugins, then both start loading, leading to each plugin being registered twice. [ Cross-file consolidated ]