-
Notifications
You must be signed in to change notification settings - Fork 0
fix(plugins): make the plugin system actually work — 5 breaks, closes #165 #166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
61d33ca
da57d73
7c28da4
473fb08
95b029a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import { loadConfig, saveConfig, getConfigPath, type NotionRemote, type GitHubRe | |
| import fs from "fs"; | ||
| import { execSync } from "child_process"; | ||
| import os from "os"; | ||
| import { PluginLoader, type RemoteTypeDefinition } from "../lib/plugin.js"; | ||
|
|
||
| import { installMcpConfig } from "./mcp-config.js"; | ||
| import { connectSetup } from "./connect.js"; | ||
|
|
@@ -150,6 +151,8 @@ export async function configMenu(): Promise<void> { | |
| } | ||
|
|
||
| async function addRemote(cfg: Config): Promise<void> { | ||
| const pluginRemoteTypes = await loadPluginRemoteTypes(cfg); | ||
|
|
||
| const { remoteType } = await inquirer.prompt([{ | ||
| type: "list", | ||
| name: "remoteType", | ||
|
|
@@ -164,7 +167,11 @@ async function addRemote(cfg: Config): Promise<void> { | |
| { name: "Notion database", value: "notion-database" }, | ||
| { name: "Notion page", value: "notion-page" }, | ||
| { name: "Notion view", value: "notion-view" }, | ||
| { name: "GitHub project", value: "github-project" } | ||
| { name: "GitHub project", value: "github-project" }, | ||
| ...pluginRemoteTypes.map(rt => ({ | ||
| name: `${rt.name ?? rt.type}${rt.description ? ` — ${rt.description}` : ""} (plugin)`, | ||
| value: rt.type | ||
| })) | ||
| ] | ||
| }]); | ||
|
|
||
|
|
@@ -184,9 +191,93 @@ async function addRemote(cfg: Config): Promise<void> { | |
| await addNotionRemote(cfg, remoteType); | ||
| } else if (remoteType === "github-project") { | ||
| await addGitHubRemote(cfg); | ||
| } else { | ||
| const definition = pluginRemoteTypes.find(rt => rt.type === remoteType); | ||
| if (definition) await addPluginRemote(cfg, definition); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Remote types contributed by plugins. | ||
| * | ||
| * getAllRemoteTypes() existed but had no callers, so a plugin could declare a remote type | ||
| * and `can config` would never offer it — making every command that depends on that remote | ||
| * unreachable in practice. | ||
| */ | ||
| async function loadPluginRemoteTypes(cfg: Config): Promise<RemoteTypeDefinition[]> { | ||
| try { | ||
| const loader = new PluginLoader(cfg); | ||
| await loader.loadAll(); | ||
| const seen = new Set<string>(); | ||
| // Builtin handlers win: a plugin must not silently replace `neon` or `cloudflare`. | ||
| const builtin = new Set(["ai-platform", "ssh", "mcp-server", "cloudflare", "neon", | ||
| "rclone", "notion-database", "notion-page", "notion-view", "github-project"]); | ||
| return loader.getAllRemoteTypes().filter(rt => { | ||
| if (!rt?.type || builtin.has(rt.type) || seen.has(rt.type)) return false; | ||
| seen.add(rt.type); | ||
| return true; | ||
| }); | ||
| } catch (error: any) { | ||
| console.warn(`[chitty] Could not load plugin remote types: ${error.message}`); | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Prompt for a plugin-defined remote using its declared schema/configFields. | ||
| * | ||
| * Sensitive fields are NOT stored in the config file. They are masked at the prompt and | ||
| * recorded as an env-var reference, matching the existing `NEON_API_KEY` fallback rather | ||
| * than widening plaintext credential storage across every plugin. Moving these to | ||
| * ChittySecrets is follow-up work owned by the credential lane, not this change. | ||
| */ | ||
| async function addPluginRemote(cfg: Config, definition: RemoteTypeDefinition): Promise<void> { | ||
| const fields = definition.configFields ?? Object.entries(definition.schema ?? {}).map( | ||
| ([name, spec]: [string, any]) => ({ | ||
| name, | ||
| description: name, | ||
| required: Boolean(spec?.required), | ||
| sensitive: /key|token|secret|password/i.test(name) | ||
| }) | ||
| ); | ||
|
|
||
| const { name } = await inquirer.prompt([{ type: "input", name: "name", message: "Remote name" }]); | ||
| if (!name) return; | ||
|
|
||
| const remote: Record<string, any> = { type: definition.type }; | ||
| for (const field of fields) { | ||
| const sensitive = (field as any).sensitive === true; | ||
| const envVar = `${definition.type.replace(/[^a-z0-9]+/gi, "_").toUpperCase()}_${field.name.replace(/[^a-z0-9]+/gi, "_").toUpperCase()}`; | ||
| const answer = await inquirer.prompt([{ | ||
| type: sensitive ? "password" : "input", | ||
| name: "value", | ||
| message: sensitive | ||
| ? `${field.description} (leave blank to read ${envVar} at run time)` | ||
| : `${field.description}${field.required ? "" : " (optional)"}`, | ||
| default: (field as any).default | ||
| }]); | ||
| if (!answer.value) continue; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Handle blank answers according to the field metadata. This line skips all blank answers. It does not enforce If 🤖 Prompt for AI Agents |
||
| // A supplied secret is referenced, never written to the config file. | ||
| remote[field.name] = sensitive ? `\${${envVar}}` : answer.value; | ||
| if (sensitive) { | ||
| console.log(` Set ${envVar} in your environment; the value was not written to the config.`); | ||
|
Comment on lines
+263
to
+265
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Resolve sensitive plugin credentials before plugin use
🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
|
|
||
| if (definition.validate) { | ||
| const verdict = definition.validate(remote); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Catch exceptions from
🤖 Prompt for AI Agents |
||
| if (verdict !== true) { | ||
| console.error(`[chitty] Invalid ${definition.type} remote: ${typeof verdict === "string" ? verdict : "validation failed"}`); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| cfg.remotes = cfg.remotes || {}; | ||
| (cfg.remotes as any)[name] = remote; | ||
| saveConfig(cfg); | ||
| console.log(`Added ${definition.type} remote "${name}"`); | ||
| } | ||
|
|
||
| async function addNotionRemote(cfg: Config, type: string): Promise<void> { | ||
| const ans = await inquirer.prompt([ | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import { installZsh, uninstallZsh } from "./commands/hook.js"; | |
| import { syncSetup, syncRun, syncStatus } from "./commands/sync.js"; | ||
| import { listExtensions, enableExtension, disableExtension, installExtension } from "./commands/extension.js"; | ||
| import { PluginLoader } from "./lib/plugin.js"; | ||
| import { registerPluginCommands } from "./lib/plugin-commands.js"; | ||
| import { doctor } from "./commands/doctor.js"; | ||
| import { briefCommand } from "./commands/brief.js"; | ||
| import { chittyCommand } from "./commands/chitty.js"; | ||
|
|
@@ -123,7 +124,7 @@ if (firstArg && firstArg in CLI_CONFIGS) { | |
| process.exit(0); | ||
| } | ||
|
|
||
| yargs(args) | ||
| const cli = yargs(args) | ||
| .scriptName("can") | ||
| .usage("$0 <command> [options]") | ||
| .command( | ||
|
|
@@ -1246,6 +1247,37 @@ yargs(args) | |
| process.exit(1); | ||
| }) | ||
| .demandCommand(1, "You must provide a command") | ||
| ; | ||
|
|
||
| // Plugin commands must be registered BEFORE .strict(), which rejects anything unknown. | ||
| // loadAll() populated the loader above; without this call getAllCommands() was never | ||
| // consumed and every plugin-supplied command was unreachable. | ||
| try { | ||
| // yargs exposes the registered command list only through internal methods, which are | ||
| // untyped. If that shape ever changes this must fail LOUDLY rather than silently | ||
| // permitting a plugin to shadow a builtin. | ||
| const internal = (cli as any).getInternalMethods?.()?.getCommandInstance?.(); | ||
| const builtinNames = new Set<string>(internal?.getCommands?.() ?? []); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Reserve the direct-routing command names.
Add As per coding guidelines, " 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| if (builtinNames.size === 0) { | ||
| console.warn("[chitty] Could not enumerate built-in commands; plugin shadowing checks are disabled."); | ||
| } | ||
| const pluginCommands = pluginLoader.getAllCommands().filter(cmd => { | ||
| const head = typeof cmd?.name === "string" ? cmd.name.trim().split(/\s+/)[0] : ""; | ||
| if (head && builtinNames.has(head)) { | ||
|
Comment on lines
+1269
to
+1271
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This collision set only contains yargs-registered built-ins and omits the command names in Useful? React with 👍 / 👎. |
||
| // Silently shadowing `can config` / `can connect` / `can export` would let any | ||
| // installed extension take over a builtin, including the credential store command. | ||
| console.warn(`[chitty] Plugin command "${cmd.name}" would shadow the built-in "${head}"; ignoring it.`); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because Useful? React with 👍 / 👎. |
||
| return false; | ||
| } | ||
| return true; | ||
| }); | ||
| registerPluginCommands(cli, pluginCommands, config); | ||
| } catch (error: any) { | ||
| // A malformed plugin must not take down every command, including --version. | ||
| console.warn(`[chitty] Failed to register plugin commands: ${error.message}`); | ||
| } | ||
|
|
||
| cli | ||
| .strict() | ||
| .help() | ||
| .alias("h", "help") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| import type { Argv } from "yargs"; | ||
| import type { CommandDefinition } from "./plugin.js"; | ||
| import type { Config } from "./config.js"; | ||
|
|
||
| /** | ||
| * Register plugin-supplied commands with yargs. | ||
| * | ||
| * Plugin command names are space-separated paths ("neon branch list"), not yargs command | ||
| * strings. Passing one straight to `.command()` would declare a command `neon` taking two | ||
| * positionals called `branch` and `list` — accepting `can neon foo bar` and rejecting | ||
| * nothing. So the names are first assembled into a tree and registered as nested commands. | ||
| * | ||
| * Leaf handlers receive yargs' argv plus the CLI config, matching CommandDefinition. | ||
| */ | ||
|
|
||
| interface Node { | ||
| children: Map<string, Node>; | ||
| leaf?: CommandDefinition; | ||
| } | ||
|
|
||
| export function buildCommandTree(commands: CommandDefinition[]): Node { | ||
| const root: Node = { children: new Map() }; | ||
| for (const cmd of commands) { | ||
| if (typeof cmd?.name !== "string") continue; | ||
| const parts = cmd.name.trim().split(/\s+/).filter(Boolean); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Preserve Line 24 only converts Flatten 🤖 Prompt for AI Agents
Comment on lines
+23
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For any configured plugin using the existing Useful? React with 👍 / 👎. |
||
| if (parts.length === 0) continue; | ||
| // yargs metacharacters would be parsed as positional syntax and yield a command that | ||
| // is listed in --help but can never be invoked. | ||
| if (parts.some(p => /[<>[\]|.]/.test(p))) { | ||
| console.warn(`[chitty] Ignoring plugin command with reserved characters in its name: ${cmd.name}`); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The reserved-character check still accepts Useful? React with 👍 / 👎. |
||
| continue; | ||
| } | ||
| let node = root; | ||
| for (const part of parts) { | ||
| let next = node.children.get(part); | ||
| if (!next) { | ||
| next = { children: new Map() }; | ||
| node.children.set(part, next); | ||
| } | ||
| node = next; | ||
| } | ||
| if (node.leaf && node.leaf !== cmd) { | ||
| console.warn(`[chitty] Two plugins declare the command "${cmd.name}"; the later one wins.`); | ||
| } | ||
| node.leaf = cmd; | ||
| } | ||
| return root; | ||
| } | ||
|
|
||
| function describe(node: Node, name: string): string { | ||
| if (node.leaf) return node.leaf.description; | ||
| const kids = Array.from(node.children.keys()).join(", "); | ||
| return `${name} subcommands: ${kids}`; | ||
| } | ||
|
|
||
| function registerNode(y: Argv, name: string, node: Node, config: Config): Argv { | ||
| const hasChildren = node.children.size > 0; | ||
| const hasOwnHandler = Boolean(node.leaf?.handler); | ||
| // A node with children AND its own handler must accept both forms, so the positional is | ||
| // optional and demandCommand is not applied — otherwise its handler is dead code. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When plugins declare both a handled command such as Useful? React with 👍 / 👎. |
||
| const commandString = hasChildren ? `${name} ${hasOwnHandler ? "[subcommand]" : "<subcommand>"}` : name; | ||
| return y.command( | ||
| commandString, | ||
| describe(node, name), | ||
| (sub: Argv) => { | ||
| for (const [childName, child] of node.children) { | ||
| registerNode(sub, childName, child, config); | ||
| } | ||
| if (node.leaf?.options) sub.options(node.leaf.options); | ||
| // A branch with children and no handler of its own must not silently succeed. | ||
| return hasChildren && !hasOwnHandler | ||
| ? sub.demandCommand(1, `Specify a ${name} subcommand`) | ||
| : sub; | ||
| }, | ||
| async (argv: any) => { | ||
| if (!node.leaf?.handler) return; | ||
| await node.leaf.handler(argv, config); | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| export function registerPluginCommands( | ||
| cli: Argv, | ||
| commands: CommandDefinition[], | ||
| config: Config | ||
| ): Argv { | ||
| const root = buildCommandTree(commands); | ||
| for (const [name, node] of root.children) { | ||
| registerNode(cli, name, node, config); | ||
| } | ||
| return cli; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,6 +102,8 @@ export class PluginLoader { | |
| * Load all plugins from config | ||
| */ | ||
| async loadAll(): Promise<void> { | ||
| await this.loadBundledPlugins(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an existing configuration lists a bundled name such as Useful? React with 👍 / 👎. |
||
|
|
||
| const extensions = this.config.extensions || {}; | ||
|
|
||
| for (const [name, extConfig] of Object.entries(extensions)) { | ||
|
|
@@ -116,6 +118,58 @@ export class PluginLoader { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Load the plugins that ship inside this package. | ||
| * | ||
| * These are listed explicitly rather than discovered by scanning: a directory scan at | ||
| * startup costs a stat per entry on every `can` invocation, and an explicit list fails | ||
| * loudly in review when a plugin is added without being registered. | ||
| * | ||
| * Measured cost of importing all of these: ~17ms against a ~280ms baseline startup. | ||
| * | ||
| * `ai/` and `chittyos/` are deliberately absent — their index modules are barrel files | ||
| * that re-export helpers and expose no `metadata`, so they are not plugins. | ||
| */ | ||
| private async loadBundledPlugins(): Promise<void> { | ||
| const bundled: Array<[string, () => Promise<any>]> = [ | ||
| ["ai", () => import("../plugins/ai/index.js")], | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Bundling the AI collection exposes Useful? React with 👍 / 👎. |
||
| ["chittyos", () => import("../plugins/chittyos/index.js")], | ||
| ["cloudflare", () => import("../plugins/cloudflare/index.js")], | ||
| ["linear", () => import("../plugins/linear/index.js")], | ||
| ["neon", () => import("../plugins/neon/index.js")] | ||
| ]; | ||
|
|
||
| for (const [dir, load] of bundled) { | ||
| let candidates: unknown[]; | ||
| try { | ||
| const mod: any = await load(); | ||
| const exported = mod.default ?? mod; | ||
| // ai/ and chittyos/ export an ARRAY of plugins; the rest export one. | ||
| candidates = Array.isArray(exported) ? exported : [exported]; | ||
| } catch (error: any) { | ||
| console.warn(`[chitty] Bundled plugin "${dir}" failed to import: ${error.message}`); | ||
| continue; | ||
| } | ||
|
|
||
| for (const candidate of candidates) { | ||
| const plugin = candidate as ChittyPlugin; | ||
| if (!this.isValidPlugin(plugin)) { | ||
| console.warn(`[chitty] Bundled plugin in "${dir}" is malformed; skipping.`); | ||
| continue; | ||
| } | ||
| const name = plugin.metadata.name; | ||
| try { | ||
| // init() BEFORE registering. Registering first left a live, uninitialised plugin | ||
| // whose handlers ran against state init() never built. | ||
| if (plugin.init) await plugin.init(this.config); | ||
| this.plugins.set(name, plugin); | ||
| } catch (error: any) { | ||
| console.warn(`[chitty] Bundled plugin "${name}" (${dir}) failed to initialise, and will not be registered: ${error.message}`); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get a loaded plugin | ||
| */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: chittyos/chittycan
Length of output: 10947
🏁 Script executed:
Repository: chittyos/chittycan
Length of output: 8343
🤖 get_repo_knowledge executed:
get_repo_knowledge chittyos/chittycan /tmp/coderabbit-repo-knowledge/chittyos-chittycan-e02851f7/conventions /tmp/coderabbit-repo-knowledge/chittyos-chittycan-e02851f7/architectureLength of output: 24666
Avoid initializing plugins twice during remote discovery.
src/index.tscreates onePluginLoaderand callsloadAll()before theconfigcommand runs.loadPlugin()andloadBundledPlugins()both call each plugin'sinit().loadPluginRemoteTypes()creates a second loader and callsloadAll(), so a new-remote action can callinit()twice. Reuse the startupPluginLoaderor add metadata-only discovery unless every plugin'sinit()is idempotent.🤖 Prompt for AI Agents