Add plugin system for reusable agent extensions

[parshvadaftari]

Summary

Adds a plugin system that lets gitclaw agents load reusable extensions from local directories or git URLs. Plugins can provide tools, hooks, skills, and prompt
additions — all using gitclaw's existing YAML-first, git-native patterns.

What's included:

• Three-tier plugin discovery (local → global → installed)
• plugin.yaml manifest with config schema and env var support
• Declarative YAML tools/hooks/skills + optional programmatic entry point
• CLI commands: gitclaw plugin install|list|remove|enable|disable|init
• SDK integration via existing query() API

New files: plugin-types.ts, plugins.ts, plugin-cli.ts, plugin-sdk.ts
Modified: loader.ts, sdk.ts, hooks.ts, index.ts, exports.ts

Test Plan

1. npm install && npm run build
2. Create a sample plugin: gitclaw plugin init my-plugin
3. Add a tool YAML + script to plugins/my-plugin/tools/
4. Enable in agent.yaml under plugins:
5. Run gitclaw and verify the tool is available

[shreyas-lyzr]

Plugin System Review

Strong foundation — the three-tier discovery, YAML-first design, and config schema with env fallback are well thought out. However, since this is the public API that plugin authors will build against, there are several issues that need to be fixed before merge.

Critical (blocks merge)

1. Command injection in installPlugin() — source and version are interpolated into a shell string via execSync. Double quotes don't protect against $(...) or backtick subshells. Use execFileSync (no shell).
2. Programmatic hooks are broken — plugin-sdk.ts creates synthetic __programmatic_* script names with _handler, but executeHook() still spawns sh on the path. There's no code path that invokes _handler — all programmatic hooks will crash at runtime.
3. agent.yaml destroyed on every plugin CLI operation — yaml.dump() strips all comments, reorders keys, reformats. Plugin install/remove/enable/disable will corrupt users' hand-written YAML.

High

4. Tool name collisions only warn — Two plugins providing a same-named tool both load. Should error or auto-namespace.
5. Divergent programmatic tool wrapping — index.ts inlines conversion (missing details), sdk.ts uses toAgentTool(). Extract to shared module.
6. Fragile --dir parsing in plugin subcommand — Manual argv.indexOf breaks on edge cases. Reuse parseArgs().

Medium

7. No path traversal guard on plugin hook scripts — script: "../../../.env" escapes the plugin directory.
8. installPlugin silently returns stale dirs — Corrupt installs (no plugin.yaml) are accepted without validation.
9. engine field defined but never enforced — Plugins requiring newer gitclaw versions load and break silently.
10. Silent catch {} in manifest helpers — Plugin CLI operations fail with zero feedback.

See inline comments for specific locations and suggested fixes.

[shreyas-lyzr]

Critical: Command injection. source and version are user-controlled strings interpolated into a shell command. Double quotes don't prevent $(...) or backtick subshell expansion.

Use execFileSync("git", ["clone", "--depth", "1", ...branchArgs, source, pluginDir], { stdio: "pipe" }) instead of execSync with string interpolation. execFileSync bypasses the shell entirely.

[shreyas-lyzr]

Critical: Programmatic hooks are dead code. These synthetic __programmatic_* script names and _handler properties are never consumed. executeHook() in hooks.ts spawns sh on the script path, which will fail for these synthetic names.

You need to either:

1. Add a check in executeHook() / runHooks() for _handler and call it directly instead of spawning a shell, or
2. Use a completely separate execution path for programmatic hooks

Right now, any plugin using api.registerHook() will crash at runtime when the hook fires.

[shreyas-lyzr]

High: yaml.dump() destroys agent.yaml formatting. This rewrites the entire file — all comments are stripped, key order changes, quoting style changes. Users' hand-written YAML gets mangled on every plugin install/remove/enable/disable.

Consider using the yaml package's parseDocument() + toString() which preserves comments, or do a targeted string append/replace instead of a full parse-dump roundtrip. This applies to both addPluginToManifest and removePluginFromManifest.

[shreyas-lyzr]

High: Tool collision should be an error, not a warning. Plugin authors won't notice a console.warn during development. Two plugins with a tool named search = the agent calls whichever was loaded last, silently shadowing the other.

Either:

• Error and refuse to load the conflicting plugin
• Auto-namespace tools as pluginId:toolName
• At minimum, skip the duplicate tool instead of loading both

[shreyas-lyzr]

High: Duplicated tool conversion logic. This inlines what toAgentTool() in sdk.ts already does, but with a subtle difference — the sdk.ts version handles result.details, this one hardcodes details: undefined.

Extract toAgentTool to a shared module and use it in both places. Divergent conversion = divergent behavior between CLI and SDK modes.

[shreyas-lyzr]

Medium: Fragile --dir parsing. This manual process.argv.indexOf approach breaks when:

• --dir appears as a value to another flag
• The user writes gitclaw plugin install --dir (missing value)
• -d appears before plugin in argv

Reuse the existing parseArgs() function, or at least extract the dir-parsing logic into a shared helper.

[shreyas-lyzr]

Medium: Returns stale/corrupt directory. If the plugin dir exists but is missing plugin.yaml (e.g., previous install was interrupted), this silently returns the broken path. loadPlugin will then return null, and the user gets a vague "not found" message despite the directory existing.

Suggest: check for plugin.yaml before returning, and if missing, remove the directory and re-clone.

[shreyas-lyzr]

Medium: engine field is never checked. A plugin declaring engine: ">=2.0.0" loads on gitclaw 1.x and breaks in confusing ways.

Either add a semver check in loadPlugin() or remove the field from the type until you're ready to enforce it.

[shreyas-lyzr]

Code Review — Plugin System

Overall this is well-architected — YAML-first, reuses existing loaders (loadDeclarativeTools, discoverSkills), clean three-tier discovery, and good security with the path traversal guard on hooks. A few issues to address before merge:

---

1. Plugin skills bypass manifest.skills whitelist

src/loader.ts ~line 279-284

Plugin skills are merged AFTER the whitelist filter, so they bypass it. If this is intentional (plugins are explicitly enabled in agent.yaml so their skills are trusted), add a comment explaining it. If not, apply the same filter.

---

2. Tool name collision check is incomplete

src/plugins.ts ~line 340-357

Plugin tools are checked against other plugin tools but NOT against builtin tools (cli, read, write, memory) or declarative tools from tools/*.yaml. A plugin could silently override read or write.

Fix: After adding plugin tools in src/index.ts and src/sdk.ts, warn on collisions:

const existingNames = new Set(tools.map(t => t.name));
for (const plugin of loaded.plugins) {
    for (const t of [...plugin.tools, ...plugin.programmaticTools]) {
        if (existingNames.has(t.name)) {
            console.warn(`[plugin:${plugin.manifest.id}] Tool "${t.name}" collides with existing tool — skipping`);
        }
    }
}

---

3. handleInstall always auto-enables

src/plugin-cli.ts ~line 85

addPluginToManifest hardcodes enabled: true. Add a --no-enable flag so users can install without auto-enabling.

---

4. No upgrade/reinstall path

src/plugin-cli.ts ~line 130-138

If a plugin is already installed, it's silently skipped. Add a --force flag to re-clone, and print a message when skipping:

Plugin "foo" already installed. Use --force to reinstall.

---

5. Memory extension point missing

src/plugin-sdk.ts

The plugin API supports tools, hooks, skills, and prompts — but not memory layers. GitClaw's memory system (src/tools/memory.ts) already supports layers via memory/memory.yaml. Add registerMemoryLayer() to GitclawPluginApi:

registerMemoryLayer(layer: { name: string; path: string; description: string }): void;

And add memoryLayers to LoadedPlugin in plugin-types.ts. This lets plugins extend the agent's memory system cleanly.

---

6. Minor: Add comment on yaml vs js-yaml dep

package.json

The PR adds yaml (v2) alongside existing js-yaml. This is justified — parseDocument() from yaml preserves formatting when editing agent.yaml, which js-yaml can't do. Add a comment in plugin-cli.ts explaining why yaml is used instead of js-yaml here.

---

Everything else looks good — hooks baseDir isolation, programmatic hook support, toAgentTool extraction, and the SDK/voice integration path all work correctly. Plugin tools flow through query() → loadAgent() so they work in both CLI and voice mode without needing voice server changes. 👍

[shreyas-lyzr]

Re-review — All Items Addressed ✅

Checked commit fb24718 ("Fix PR review round 2") against all 6 review items:

#	Issue	Status
1	Plugin skills whitelist comment	✅ Clear trust model comment in loader.ts
2	Tool name collision check	✅ Both index.ts and sdk.ts warn + skip colliding tools
3	--no-enable flag	✅ Added to plugin-cli.ts, wired to addPluginToManifest
4	--force reinstall	✅ Added to both git URL and local path install flows, prints "already installed" hint
5	Memory extension point	✅ Full chain: MemoryLayerDef type → registerMemoryLayer() API → BuiltinToolsConfig → memory.ts merge
6	yaml vs js-yaml comment	✅ Comment added in plugin-cli.ts

Memory integration is clean — plugin layers flow through loaded.plugins.flatMap(p => p.memoryLayers) → createBuiltinTools({ pluginMemoryLayers }) → createMemoryTool(cwd, pluginLayers) → loadMemoryConfig() merge. No unnecessary abstractions.

LGTM — ready to merge.