-
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 1 commit
61d33ca
da57d73
7c28da4
473fb08
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 |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| 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) { | ||
| 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; | ||
| 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; | ||
| } | ||
| // Last definition wins, matching the loader's "a configured extension wins" ordering. | ||
| 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 { | ||
| return y.command( | ||
| node.children.size > 0 ? `${name} <subcommand>` : name, | ||
| 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 but no handler of its own must not silently succeed. | ||
| return node.children.size > 0 ? sub.demandCommand(1, `Specify a ${name} subcommand`) : sub; | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| }, | ||
| 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,40 @@ 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 = [ | ||
| () => import("../plugins/cloudflare/index.js"), | ||
| () => import("../plugins/linear/index.js"), | ||
| () => import("../plugins/neon/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.
Making these plugins active by default exposes an incompatible configuration path: Useful? React with 👍 / 👎. |
||
| ]; | ||
|
|
||
| for (const load of bundled) { | ||
| try { | ||
| const mod: any = await load(); | ||
| const plugin: ChittyPlugin = mod.default || mod; | ||
| if (!this.isValidPlugin(plugin)) continue; | ||
| if (this.plugins.has(plugin.metadata.name)) continue; // a configured extension wins | ||
| this.plugins.set(plugin.metadata.name, plugin); | ||
| if (plugin.init) await plugin.init(this.config); | ||
| } catch (error: any) { | ||
| // Never let a bundled plugin break every command. Report, do not throw. | ||
| console.warn(`[chitty] Failed to load bundled plugin: ${error.message}`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get a loaded plugin | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import { describe, it, expect } from "vitest"; | ||
| import { buildCommandTree, registerPluginCommands } from "../src/lib/plugin-commands.js"; | ||
| import { PluginLoader } from "../src/lib/plugin.js"; | ||
| import type { CommandDefinition } from "../src/lib/plugin.js"; | ||
|
|
||
| const cmd = (name: string): CommandDefinition => ({ name, description: `desc ${name}`, handler: async () => {} }); | ||
|
|
||
| describe("buildCommandTree", () => { | ||
| it("nests a space-separated name instead of treating it as positionals", () => { | ||
| // The whole reason this module exists: yargs would read "neon branch list" as a | ||
| // command `neon` taking two positionals, accepting `can neon foo bar`. | ||
| const root = buildCommandTree([cmd("neon branch list")]); | ||
| const neon = root.children.get("neon")!; | ||
| expect(neon).toBeDefined(); | ||
| expect(neon.leaf).toBeUndefined(); | ||
| const branch = neon.children.get("branch")!; | ||
| expect(branch.leaf).toBeUndefined(); | ||
| expect(branch.children.get("list")!.leaf!.name).toBe("neon branch list"); | ||
| }); | ||
|
|
||
| it("shares intermediate nodes across sibling commands", () => { | ||
| const root = buildCommandTree([cmd("neon branch list"), cmd("neon branch create"), cmd("neon db list")]); | ||
| const neon = root.children.get("neon")!; | ||
| expect([...neon.children.keys()].sort()).toEqual(["branch", "db"]); | ||
| expect([...neon.children.get("branch")!.children.keys()].sort()).toEqual(["create", "list"]); | ||
| }); | ||
|
|
||
| it("supports a single-word command", () => { | ||
| const root = buildCommandTree([cmd("linear teams")]); | ||
| expect(root.children.get("linear")!.children.get("teams")!.leaf!.name).toBe("linear teams"); | ||
| }); | ||
|
|
||
| it("allows a node to be both a leaf and a parent", () => { | ||
| const root = buildCommandTree([cmd("linear issues"), cmd("linear issue create")]); | ||
| const linear = root.children.get("linear")!; | ||
| expect(linear.children.get("issues")!.leaf).toBeDefined(); | ||
| expect(linear.children.get("issue")!.leaf).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("ignores an empty or whitespace-only name rather than creating a blank node", () => { | ||
| const root = buildCommandTree([cmd(" "), cmd("")]); | ||
| expect(root.children.size).toBe(0); | ||
| }); | ||
|
|
||
| it("collapses repeated whitespace", () => { | ||
| const root = buildCommandTree([cmd("cf worker list")]); | ||
| expect(root.children.get("cf")!.children.get("worker")!.children.get("list")!.leaf).toBeDefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("registerPluginCommands", () => { | ||
| it("registers one top-level yargs command per root child", () => { | ||
| const seen: string[] = []; | ||
| const fakeYargs: any = { command: (name: string) => { seen.push(name); return fakeYargs; } }; | ||
| registerPluginCommands(fakeYargs, [cmd("neon branch list"), cmd("cf worker list")], {} as any); | ||
| expect(seen.sort()).toEqual(["cf <subcommand>", "neon <subcommand>"]); | ||
| }); | ||
|
|
||
| it("passes the config through to the leaf handler", async () => { | ||
| let received: unknown = null; | ||
| const def: CommandDefinition = { name: "x go", description: "d", handler: async (_a, c) => { received = c; } }; | ||
| const captured: any[] = []; | ||
| const fakeYargs: any = { | ||
| command: (_n: string, _d: string, builder: any, handler: any) => { | ||
| captured.push({ builder, handler }); | ||
| builder({ command: fakeYargs.command, options: () => fakeYargs, demandCommand: () => fakeYargs }); | ||
| return fakeYargs; | ||
| }, | ||
| options: () => fakeYargs, | ||
| demandCommand: () => fakeYargs | ||
| }; | ||
| const config = { marker: 42 } as any; | ||
| registerPluginCommands(fakeYargs, [def], config); | ||
| const leaf = captured[captured.length - 1]; | ||
| await leaf.handler({}); | ||
| expect(received).toBe(config); | ||
| }); | ||
| }); | ||
|
|
||
| describe("PluginLoader bundled plugins — real modules, no mocks", () => { | ||
| it("loads the in-tree plugins that shipped unreachable before this fix", async () => { | ||
| const loader = new PluginLoader({} as any); | ||
| await loader.loadAll(); | ||
| const names = loader.getAllPlugins().map(p => p.metadata.name).sort(); | ||
| expect(names).toEqual(["@chitty/cloudflare", "@chitty/linear", "@chitty/neon"]); | ||
| }); | ||
|
|
||
| it("exposes their commands through getAllCommands, which nothing used to call", async () => { | ||
| const loader = new PluginLoader({} as any); | ||
| await loader.loadAll(); | ||
| const names = loader.getAllCommands().map(c => c.name); | ||
| expect(names).toContain("neon branch list"); | ||
| expect(names.length).toBeGreaterThanOrEqual(8); | ||
| }); | ||
| }); |
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.
When a configured plugin's
init()rejects,loadPlugin()has already inserted it into the loader's map before throwing;loadAll()catches that failure but leaves the plugin present. This new unconditional registration then exposes commands from a plugin that explicitly failed to initialize, and an unsuccessful configured override can even replace a working bundled plugin, so only successfully initialized plugins should reachgetAllCommands()or the failed entry should be removed.Useful? React with 👍 / 👎.