fix: address code review findings for plugin command API
Blockers fixed: - Fix documentation: requireAuth defaults to true (not false) - Add command name validation (must start with letter, alphanumeric only) - Add reserved commands list to prevent shadowing built-in commands - Emit diagnostic errors for invalid/duplicate command registration Other improvements: - Return user-friendly message for unauthorized commands (instead of silence) - Sanitize error messages to avoid leaking internal details - Document acceptsArgs behavior when arguments are provided - Add notes about reserved commands and validation rules to docs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
committed by
Peter Steinberger
parent
4ee808dbcb
commit
b56587f26e
@@ -526,8 +526,8 @@ Command options:
|
|||||||
|
|
||||||
- `name`: Command name (without the leading `/`)
|
- `name`: Command name (without the leading `/`)
|
||||||
- `description`: Help text shown in command lists
|
- `description`: Help text shown in command lists
|
||||||
- `acceptsArgs`: Whether the command accepts arguments (default: false)
|
- `acceptsArgs`: Whether the command accepts arguments (default: false). If false and arguments are provided, the command won't match and the message falls through to other handlers
|
||||||
- `requireAuth`: Whether to require authorized sender (default: false)
|
- `requireAuth`: Whether to require authorized sender (default: true)
|
||||||
- `handler`: Function that returns `{ text: string }` (can be async)
|
- `handler`: Function that returns `{ text: string }` (can be async)
|
||||||
|
|
||||||
Example with authorization and arguments:
|
Example with authorization and arguments:
|
||||||
@@ -550,6 +550,9 @@ Notes:
|
|||||||
- Plugin commands are processed **before** built-in commands and the AI agent
|
- Plugin commands are processed **before** built-in commands and the AI agent
|
||||||
- Commands are registered globally and work across all channels
|
- Commands are registered globally and work across all channels
|
||||||
- Command names are case-insensitive (`/MyStatus` matches `/mystatus`)
|
- Command names are case-insensitive (`/MyStatus` matches `/mystatus`)
|
||||||
|
- Command names must start with a letter and contain only letters, numbers, hyphens, and underscores
|
||||||
|
- Reserved command names (like `help`, `status`, `reset`, etc.) cannot be overridden by plugins
|
||||||
|
- Duplicate command registration across plugins will fail with a diagnostic error
|
||||||
|
|
||||||
### Register background services
|
### Register background services
|
||||||
|
|
||||||
|
|||||||
@@ -16,21 +16,104 @@ type RegisteredPluginCommand = ClawdbotPluginCommandDefinition & {
|
|||||||
// Registry of plugin commands
|
// Registry of plugin commands
|
||||||
const pluginCommands: Map<string, RegisteredPluginCommand> = new Map();
|
const pluginCommands: Map<string, RegisteredPluginCommand> = new Map();
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Reserved command names that plugins cannot override.
|
||||||
|
* These are built-in commands from commands-registry.data.ts.
|
||||||
|
*/
|
||||||
|
const RESERVED_COMMANDS = new Set([
|
||||||
|
// Core commands
|
||||||
|
"help",
|
||||||
|
"commands",
|
||||||
|
"status",
|
||||||
|
"whoami",
|
||||||
|
"context",
|
||||||
|
// Session management
|
||||||
|
"stop",
|
||||||
|
"restart",
|
||||||
|
"reset",
|
||||||
|
"new",
|
||||||
|
"compact",
|
||||||
|
// Configuration
|
||||||
|
"config",
|
||||||
|
"debug",
|
||||||
|
"allowlist",
|
||||||
|
"activation",
|
||||||
|
// Agent control
|
||||||
|
"skill",
|
||||||
|
"subagents",
|
||||||
|
"model",
|
||||||
|
"models",
|
||||||
|
"queue",
|
||||||
|
// Messaging
|
||||||
|
"send",
|
||||||
|
// Execution
|
||||||
|
"bash",
|
||||||
|
"exec",
|
||||||
|
// Mode toggles
|
||||||
|
"think",
|
||||||
|
"verbose",
|
||||||
|
"reasoning",
|
||||||
|
"elevated",
|
||||||
|
// Billing
|
||||||
|
"usage",
|
||||||
|
]);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Validate a command name.
|
||||||
|
* Returns an error message if invalid, or null if valid.
|
||||||
|
*/
|
||||||
|
export function validateCommandName(name: string): string | null {
|
||||||
|
const trimmed = name.trim().toLowerCase();
|
||||||
|
|
||||||
|
if (!trimmed) {
|
||||||
|
return "Command name cannot be empty";
|
||||||
|
}
|
||||||
|
|
||||||
|
// Must start with a letter, contain only letters, numbers, hyphens, underscores
|
||||||
|
if (!/^[a-z][a-z0-9_-]*$/i.test(trimmed)) {
|
||||||
|
return "Command name must start with a letter and contain only letters, numbers, hyphens, and underscores";
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check reserved commands
|
||||||
|
if (RESERVED_COMMANDS.has(trimmed)) {
|
||||||
|
return `Command name "${trimmed}" is reserved by a built-in command`;
|
||||||
|
}
|
||||||
|
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
export type CommandRegistrationResult = {
|
||||||
|
ok: boolean;
|
||||||
|
error?: string;
|
||||||
|
};
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Register a plugin command.
|
* Register a plugin command.
|
||||||
|
* Returns an error if the command name is invalid or reserved.
|
||||||
*/
|
*/
|
||||||
export function registerPluginCommand(
|
export function registerPluginCommand(
|
||||||
pluginId: string,
|
pluginId: string,
|
||||||
command: ClawdbotPluginCommandDefinition,
|
command: ClawdbotPluginCommandDefinition,
|
||||||
): void {
|
): CommandRegistrationResult {
|
||||||
const key = `/${command.name.toLowerCase()}`;
|
const validationError = validateCommandName(command.name);
|
||||||
if (pluginCommands.has(key)) {
|
if (validationError) {
|
||||||
logVerbose(
|
return { ok: false, error: validationError };
|
||||||
`Plugin command ${key} already registered, overwriting with plugin ${pluginId}`,
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const key = `/${command.name.toLowerCase()}`;
|
||||||
|
|
||||||
|
// Check for duplicate registration
|
||||||
|
if (pluginCommands.has(key)) {
|
||||||
|
const existing = pluginCommands.get(key)!;
|
||||||
|
return {
|
||||||
|
ok: false,
|
||||||
|
error: `Command "${command.name}" already registered by plugin "${existing.pluginId}"`,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
pluginCommands.set(key, { ...command, pluginId });
|
pluginCommands.set(key, { ...command, pluginId });
|
||||||
logVerbose(`Registered plugin command: ${key} (plugin: ${pluginId})`);
|
logVerbose(`Registered plugin command: ${key} (plugin: ${pluginId})`);
|
||||||
|
return { ok: true };
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -55,6 +138,10 @@ export function clearPluginCommandsForPlugin(pluginId: string): void {
|
|||||||
/**
|
/**
|
||||||
* Check if a command body matches a registered plugin command.
|
* Check if a command body matches a registered plugin command.
|
||||||
* Returns the command definition and parsed args if matched.
|
* Returns the command definition and parsed args if matched.
|
||||||
|
*
|
||||||
|
* Note: If a command has `acceptsArgs: false` and the user provides arguments,
|
||||||
|
* the command will not match. This allows the message to fall through to
|
||||||
|
* built-in handlers or the agent. Document this behavior to plugin authors.
|
||||||
*/
|
*/
|
||||||
export function matchPluginCommand(
|
export function matchPluginCommand(
|
||||||
commandBody: string,
|
commandBody: string,
|
||||||
@@ -99,7 +186,8 @@ export async function executePluginCommand(params: {
|
|||||||
logVerbose(
|
logVerbose(
|
||||||
`Plugin command /${command.name} blocked: unauthorized sender ${senderId || "<unknown>"}`,
|
`Plugin command /${command.name} blocked: unauthorized sender ${senderId || "<unknown>"}`,
|
||||||
);
|
);
|
||||||
return null; // Silently ignore unauthorized commands
|
// Return a message instead of silently ignoring
|
||||||
|
return { text: "⚠️ This command requires authorization." };
|
||||||
}
|
}
|
||||||
|
|
||||||
const ctx: PluginCommandContext = {
|
const ctx: PluginCommandContext = {
|
||||||
@@ -117,7 +205,8 @@ export async function executePluginCommand(params: {
|
|||||||
} catch (err) {
|
} catch (err) {
|
||||||
const error = err as Error;
|
const error = err as Error;
|
||||||
logVerbose(`Plugin command /${command.name} error: ${error.message}`);
|
logVerbose(`Plugin command /${command.name} error: ${error.message}`);
|
||||||
return { text: `⚠️ Command failed: ${error.message}` };
|
// Don't leak internal error details - return a safe generic message
|
||||||
|
return { text: "⚠️ Command failed. Please try again later." };
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -374,14 +374,25 @@ export function createPluginRegistry(registryParams: PluginRegistryParams) {
|
|||||||
});
|
});
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Register with the plugin command system (validates name and checks for duplicates)
|
||||||
|
const result = registerPluginCommand(record.id, command);
|
||||||
|
if (!result.ok) {
|
||||||
|
pushDiagnostic({
|
||||||
|
level: "error",
|
||||||
|
pluginId: record.id,
|
||||||
|
source: record.source,
|
||||||
|
message: `command registration failed: ${result.error}`,
|
||||||
|
});
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
record.commands.push(name);
|
record.commands.push(name);
|
||||||
registry.commands.push({
|
registry.commands.push({
|
||||||
pluginId: record.id,
|
pluginId: record.id,
|
||||||
command,
|
command,
|
||||||
source: record.source,
|
source: record.source,
|
||||||
});
|
});
|
||||||
// Register with the plugin command system
|
|
||||||
registerPluginCommand(record.id, command);
|
|
||||||
};
|
};
|
||||||
|
|
||||||
const registerTypedHook = <K extends PluginHookName>(
|
const registerTypedHook = <K extends PluginHookName>(
|
||||||
|
|||||||
Reference in New Issue
Block a user