Skip to content

Conversation

@Myriad-Dreamin
Copy link
Contributor

This PR doesn't really run a QQ plugin, but we have been initialize one which can access ema agent API in Server. See docs/plugin.zh-CN.md for detail.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a plugin infrastructure for the EverMindAgent system, enabling extensibility through pluggable modules. The initial implementation includes a QQ plugin (currently a placeholder), a plugin loading system, and supporting infrastructure. The architecture allows plugins to be dynamically loaded based on environment configuration and provides them with access to the core server APIs.

Key Changes

  • Added plugin interface definitions and infrastructure in the core ema package
  • Implemented a plugin loader in ema-ui that discovers and initializes plugins based on environment variables
  • Created a starter QQ plugin package (ema-plugin-qq) as a proof of concept

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
packages/ema/src/plugin.ts Defines the plugin interface types (EmaPlugin, EmaPluginProvider, EmaPluginModule) for implementing plugins with lifecycle hooks and optional HTTP handlers
packages/ema/src/server.ts Adds a public plugins property to the Server class for storing loaded plugin instances
packages/ema/src/index.ts Exports the new plugin types from the core package
packages/ema-ui/src/plugin.ts Implements plugin discovery from package.json dependencies and dynamic loading based on EMA_PLUGINS environment variable
packages/ema-ui/instrumentation.ts Initializes plugin loading at application startup using Next.js instrumentation hook
packages/ema-ui/package.json Adds ema-plugin-qq as a peer dependency to make it discoverable by the plugin loader
packages/ema-plugin-qq/src/index.ts Implements a minimal QQ plugin that logs startup confirmation
packages/ema-plugin-qq/package.json Defines the QQ plugin package metadata and dependencies
packages/ema-plugin-qq/tsconfig.json TypeScript configuration for the QQ plugin package
docs/plugin.zh-CN.md Chinese documentation explaining the plugin system, configuration, and development process
deployment/local.yml Adds commented-out napcat service configuration for future QQ integration
pnpm-lock.yaml Updates workspace dependencies to include the new plugin package
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 7 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 11 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

Comment on lines 4 to 8
getServer()
.then(loadPlugins)
.catch((error) => {
console.error("Failed to load plugins:", error);
});
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next.js instrumentation requires exporting a register function. The current code executes immediately at the top level, which may not work correctly with Next.js's instrumentation hook. According to Next.js documentation, you should export a register async function that will be called once when the server starts.

Suggested change
getServer()
.then(loadPlugins)
.catch((error) => {
console.error("Failed to load plugins:", error);
});
/**
* Next.js instrumentation hook that initializes the server and loads plugins.
* This function is called once when the Next.js server starts.
*/
export async function register(): Promise<void> {
try {
const server = await getServer();
await loadPlugins(server);
} catch (error) {
console.error("Failed to load plugins:", error);
}
}

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +55
export async function loadPlugins(server: Server): Promise<void> {
/**
* Comma-separated list of plugins to load
*
* @example
* EMA_PLUGINS=qq,discord
*/
const enabledPlugins = new Set(
(process.env.EMA_PLUGINS ?? "")
.split(",")
.map((name) => name.trim())
.filter((name) => name.length > 0),
);

await Promise.all(
getPluginModules()
.filter((name) => enabledPlugins.has(name))
.map(async (name: string) => {
try {
const m: EmaPluginModule = await import(`ema-plugin-${name}`);
if (m.Plugin.name in server.plugins) {
throw new Error(`Plugin ${m.Plugin.name} already loaded`);
}

const plugin = new m.Plugin(server);

server.plugins[m.Plugin.name] = plugin;
} catch (error) {
console.error(`Failed to load plugin package "${name}":`, error);
return;
}
}),
);

const plugins = Object.entries(server.plugins);
await Promise.all(
plugins.map(async ([name, plugin]) => {
if (!plugin) {
return;
}
try {
return await plugin.start();
} catch (error) {
console.error(`Failed to start plugin "${name}":`, error);
}
}),
);
}
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new plugin loading functionality in loadPlugins and plugin infrastructure lacks test coverage. According to the custom guideline in the project (see CONTRIBUTING.md and existing tests in packages/ema/src/server.spec.ts), new features should include test cases. Consider adding tests for: plugin discovery from package.json, plugin loading with EMA_PLUGINS env var, error handling for missing/invalid plugins, and plugin lifecycle (start method).

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +35 to +38
} catch (error) {
console.error(`Failed to load plugin package "${name}":`, error);
return;
}
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error handling silently returns without re-throwing or properly handling the error. When a plugin fails to load (line 36), the error is logged but execution continues. However, when getPluginModules() includes a plugin name that doesn't exist, the dynamic import at line 27 will fail with a potentially cryptic error message. Consider providing more context about whether the plugin package doesn't exist or if there was an import error.

Copilot uses AI. Check for mistakes.
# image: mlikiowa/napcat-docker:latest
# container_name: napcat
# restart: always
# network_mode: bridge
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commented napcat service configuration contains a potentially problematic network configuration. Line 20 sets network_mode: bridge, but this is the default in Docker Compose and is typically omitted. More importantly, if this service needs to communicate with other services in the stack (like the app service), it should use the custom network ema-network defined in the compose file instead of the default bridge network.

Copilot uses AI. Check for mistakes.
Comment on lines +65 to +74
/**
* The web API endpoints of the plugin
* @param request - The request object
*/
GET?(request: Request): Promise<Response>;
/**
* The web API endpoints of the plugin
* @param request - The request object
*/
POST?(request: Request): Promise<Response>;
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing JSDoc documentation for the GET and POST methods. According to the codebase conventions (packages/ema/src/server.ts and other files show comprehensive JSDoc), optional methods should also include @returns tags and detailed descriptions explaining their purpose, parameters, and expected behavior.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +4 to +54
/**
* Loads plugins by environment variable `EMA_PLUGINS`
* @param server - The server instance
*/
export async function loadPlugins(server: Server): Promise<void> {
/**
* Comma-separated list of plugins to load
*
* @example
* EMA_PLUGINS=qq,discord
*/
const enabledPlugins = new Set(
(process.env.EMA_PLUGINS ?? "")
.split(",")
.map((name) => name.trim())
.filter((name) => name.length > 0),
);

await Promise.all(
getPluginModules()
.filter((name) => enabledPlugins.has(name))
.map(async (name: string) => {
try {
const m: EmaPluginModule = await import(`ema-plugin-${name}`);
if (m.Plugin.name in server.plugins) {
throw new Error(`Plugin ${m.Plugin.name} already loaded`);
}

const plugin = new m.Plugin(server);

server.plugins[m.Plugin.name] = plugin;
} catch (error) {
console.error(`Failed to load plugin package "${name}":`, error);
return;
}
}),
);

const plugins = Object.entries(server.plugins);
await Promise.all(
plugins.map(async ([name, plugin]) => {
if (!plugin) {
return;
}
try {
return await plugin.start();
} catch (error) {
console.error(`Failed to start plugin "${name}":`, error);
}
}),
);
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential race condition in plugin initialization. The code loads all plugin modules into server.plugins (line 34) before starting any of them (line 49). However, if the instrumentation file is called multiple times during server initialization, or if getServer() is called from multiple places, plugins could be loaded multiple times or started multiple times. There's a check for duplicate plugin names (line 28-30), but no protection against re-initialization of the entire plugin system.

Suggested change
/**
* Loads plugins by environment variable `EMA_PLUGINS`
* @param server - The server instance
*/
export async function loadPlugins(server: Server): Promise<void> {
/**
* Comma-separated list of plugins to load
*
* @example
* EMA_PLUGINS=qq,discord
*/
const enabledPlugins = new Set(
(process.env.EMA_PLUGINS ?? "")
.split(",")
.map((name) => name.trim())
.filter((name) => name.length > 0),
);
await Promise.all(
getPluginModules()
.filter((name) => enabledPlugins.has(name))
.map(async (name: string) => {
try {
const m: EmaPluginModule = await import(`ema-plugin-${name}`);
if (m.Plugin.name in server.plugins) {
throw new Error(`Plugin ${m.Plugin.name} already loaded`);
}
const plugin = new m.Plugin(server);
server.plugins[m.Plugin.name] = plugin;
} catch (error) {
console.error(`Failed to load plugin package "${name}":`, error);
return;
}
}),
);
const plugins = Object.entries(server.plugins);
await Promise.all(
plugins.map(async ([name, plugin]) => {
if (!plugin) {
return;
}
try {
return await plugin.start();
} catch (error) {
console.error(`Failed to start plugin "${name}":`, error);
}
}),
);
const initializingServers = new WeakMap<Server, Promise<void>>();
const initializedServers = new WeakSet<Server>();
/**
* Loads plugins by environment variable `EMA_PLUGINS`
* @param server - The server instance
*/
export async function loadPlugins(server: Server): Promise<void> {
if (initializedServers.has(server)) {
return;
}
const existingInitialization = initializingServers.get(server);
if (existingInitialization) {
await existingInitialization;
return;
}
const initializationPromise = (async () => {
/**
* Comma-separated list of plugins to load
*
* @example
* EMA_PLUGINS=qq,discord
*/
const enabledPlugins = new Set(
(process.env.EMA_PLUGINS ?? "")
.split(",")
.map((name) => name.trim())
.filter((name) => name.length > 0),
);
await Promise.all(
getPluginModules()
.filter((name) => enabledPlugins.has(name))
.map(async (name: string) => {
try {
const m: EmaPluginModule = await import(`ema-plugin-${name}`);
if (m.Plugin.name in server.plugins) {
throw new Error(`Plugin ${m.Plugin.name} already loaded`);
}
const plugin = new m.Plugin(server);
server.plugins[m.Plugin.name] = plugin;
} catch (error) {
console.error(`Failed to load plugin package "${name}":`, error);
return;
}
}),
);
const plugins = Object.entries(server.plugins);
await Promise.all(
plugins.map(async ([name, plugin]) => {
if (!plugin) {
return;
}
try {
return await plugin.start();
} catch (error) {
console.error(`Failed to start plugin "${name}":`, error);
}
}),
);
})();
initializingServers.set(server, initializationPromise);
try {
await initializationPromise;
initializedServers.add(server);
} finally {
initializingServers.delete(server);
}

Copilot uses AI. Check for mistakes.
* };
* ```
*/
name: string;
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name field should be marked as static in the interface definition. Currently, the interface defines name: string as an instance property, but the documentation examples and actual implementation in packages/ema-plugin-qq/src/index.ts use it as a static property (static name = "QQ"). This creates a mismatch between the interface contract and the intended usage.

Copilot uses AI. Check for mistakes.
Comment on lines +4 to +8
/**
* Loads plugins by environment variable `EMA_PLUGINS`
* @param server - The server instance
*/
export async function loadPlugins(server: Server): Promise<void> {
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing @returns JSDoc tag for the loadPlugins function. According to the codebase conventions (seen throughout packages/ema/src/*.ts files), functions should include @returns tags in their JSDoc comments even when they return Promise<void>.

Copilot generated this review using guidance from repository custom instructions.
@Myriad-Dreamin Myriad-Dreamin linked an issue Dec 22, 2025 that may be closed by this pull request
@Myriad-Dreamin Myriad-Dreamin force-pushed the napcat branch 3 times, most recently from 0605425 to f9d1511 Compare December 22, 2025 12:21
Myriad-Dreamin and others added 13 commits December 31, 2025 13:57
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

QQ接入

1 participant