ultanio/cobot

Fork 4

Plugins have undeclared runtime dependencies — eliminate service locator pattern #57

New issue

Closed

opened 2026-02-22 10:21:43 +00:00 by nazim · 1 comment

nazim commented

2026-02-22 10:21:43 +00:00

Contributor

Problem

The plugin dependency graph is a lie. Out of 23 plugins, 10 only declare a dependency on config — but at runtime they're coupled to each other through:

registry.get_by_capability() — a service locator that hides dependencies behind string lookups
Environment variables — plugins share secrets/config through os.environ instead of declared dependencies
Filesystem conventions — plugins assume skill scripts exist at certain paths

Concrete example: `tools` → `wallet`

The tools plugin hardcodes wallet_balance, wallet_pay, wallet_receive as LLM tools. At runtime it calls:

def _get_wallet(self):
    return self._registry.get_by_capability("wallet")  # might return None!

But tools doesn't declare wallet as a dependency. The graph shows no connection. If wallet isn't loaded, tools silently degrades — the agent gets tool definitions that return "No wallet available."

Same pattern repeats for knowledge, and likely others.

Why this matters

Plugin graph (#54) can't show real architecture — edges are invisible
Secret management (#50) can't enforce scoping — plugins grab what they need from env at runtime
Failures are silent — missing optional deps aren't surfaced, just degraded
Testing is hard — can't know what a plugin actually needs without reading all its code
Third-party plugins — can't validate their dependency declarations are complete

Root cause

The tools plugin is a god object — it hardcodes tool definitions for other plugins instead of letting each plugin register its own tools. Combined with get_by_capability() as a service locator, this hides all real coupling.

Proposed solution

1. Plugin-owned tool registration via extension point

Each plugin registers its own tools:

# wallet/plugin.py
meta = PluginMeta(
    id="wallet",
    capabilities=["wallet"],
    dependencies=["config"],
    implements={"loop.tools": "get_tools"},
)

def get_tools(self):
    return [
        {"name": "wallet_balance", "description": "Check wallet balance", "fn": self.get_balance},
        {"name": "wallet_pay", "description": "Pay Lightning invoice", "fn": self.pay, "parameters": {...}},
        {"name": "wallet_receive", "description": "Get receive address", "fn": self.get_receive_address},
    ]

The loop collects tools from all plugins:

# loop/plugin.py
all_tools = self.call_extension("loop.tools")  # each plugin contributes its own

2. Eliminate `get_by_capability()` for direct coupling

Where plugins need each other, declare it:

# Before (hidden):
def _get_wallet(self):
    return self._registry.get_by_capability("wallet")

# After (declared):
meta = PluginMeta(
    dependencies=["config", "wallet"],  # explicit
)

Keep get_by_capability() only for truly optional/swappable providers (e.g., llm capability where ppq and ollama are interchangeable). But make optional deps explicit too:

meta = PluginMeta(
    dependencies=["config"],
    optional_dependencies=["wallet", "knowledge"],  # new field
)

3. Declare environment dependencies

For #50 (secrets), but also for graph completeness:

meta = PluginMeta(
    id="wallet",
    secrets=["NOSTR_NSEC"],  # from #50
    system_deps=["node"],     # external runtime requirements
)

Impact on other issues

Issue	How this helps
#50 (Secrets)	Plugins declare what secrets they need — vault can enforce scoping
#54 (Inspect)	Graph shows real dependencies, not just declared ones
#52 (Workflow)	Stories can be scoped per-plugin since boundaries are clear
#49 (Activity loop)	Autonomous plugins need explicit deps to run safely

Acceptance criteria

tools plugin no longer hardcodes tool definitions for other plugins
Each plugin with tools implements loop.tools extension point
get_by_capability() removed for direct plugin-to-plugin coupling (except swappable providers)
PluginMeta gains optional_dependencies field
Plugin graph (#54) shows all real edges without runtime scanning heuristics
No plugin uses os.environ directly for secrets (deferred to #50 if needed)

Phasing suggestion

Add loop.tools extension point — plugins register own tools, tools plugin becomes a thin executor
Add optional_dependencies to PluginMeta — declare what's optional vs required
Audit and fix all plugins — declare real dependencies, remove get_by_capability() where inappropriate
Add secrets and system_deps to PluginMeta — from #50

## Problem The plugin dependency graph is a lie. Out of 23 plugins, 10 only declare a dependency on `config` — but at runtime they're coupled to each other through: 1. **`registry.get_by_capability()`** — a service locator that hides dependencies behind string lookups 2. **Environment variables** — plugins share secrets/config through `os.environ` instead of declared dependencies 3. **Filesystem conventions** — plugins assume skill scripts exist at certain paths ### Concrete example: `tools` → `wallet` The `tools` plugin hardcodes `wallet_balance`, `wallet_pay`, `wallet_receive` as LLM tools. At runtime it calls: ```python def _get_wallet(self): return self._registry.get_by_capability("wallet") # might return None! ``` But `tools` doesn't declare `wallet` as a dependency. The graph shows no connection. If wallet isn't loaded, tools silently degrades — the agent gets tool definitions that return "No wallet available." Same pattern repeats for knowledge, and likely others. ### Why this matters - **Plugin graph (#54) can't show real architecture** — edges are invisible - **Secret management (#50) can't enforce scoping** — plugins grab what they need from env at runtime - **Failures are silent** — missing optional deps aren't surfaced, just degraded - **Testing is hard** — can't know what a plugin actually needs without reading all its code - **Third-party plugins** — can't validate their dependency declarations are complete ## Root cause The `tools` plugin is a **god object** — it hardcodes tool definitions for other plugins instead of letting each plugin register its own tools. Combined with `get_by_capability()` as a service locator, this hides all real coupling. ## Proposed solution ### 1. Plugin-owned tool registration via extension point Each plugin registers its own tools: ```python # wallet/plugin.py meta = PluginMeta( id="wallet", capabilities=["wallet"], dependencies=["config"], implements={"loop.tools": "get_tools"}, ) def get_tools(self): return [ {"name": "wallet_balance", "description": "Check wallet balance", "fn": self.get_balance}, {"name": "wallet_pay", "description": "Pay Lightning invoice", "fn": self.pay, "parameters": {...}}, {"name": "wallet_receive", "description": "Get receive address", "fn": self.get_receive_address}, ] ``` The loop collects tools from all plugins: ```python # loop/plugin.py all_tools = self.call_extension("loop.tools") # each plugin contributes its own ``` ### 2. Eliminate `get_by_capability()` for direct coupling Where plugins need each other, declare it: ```python # Before (hidden): def _get_wallet(self): return self._registry.get_by_capability("wallet") # After (declared): meta = PluginMeta( dependencies=["config", "wallet"], # explicit ) ``` Keep `get_by_capability()` only for truly optional/swappable providers (e.g., `llm` capability where ppq and ollama are interchangeable). But make optional deps explicit too: ```python meta = PluginMeta( dependencies=["config"], optional_dependencies=["wallet", "knowledge"], # new field ) ``` ### 3. Declare environment dependencies For #50 (secrets), but also for graph completeness: ```python meta = PluginMeta( id="wallet", secrets=["NOSTR_NSEC"], # from #50 system_deps=["node"], # external runtime requirements ) ``` ## Impact on other issues | Issue | How this helps | |-------|---------------| | #50 (Secrets) | Plugins declare what secrets they need — vault can enforce scoping | | #54 (Inspect) | Graph shows real dependencies, not just declared ones | | #52 (Workflow) | Stories can be scoped per-plugin since boundaries are clear | | #49 (Activity loop) | Autonomous plugins need explicit deps to run safely | ## Acceptance criteria - [ ] `tools` plugin no longer hardcodes tool definitions for other plugins - [ ] Each plugin with tools implements `loop.tools` extension point - [ ] `get_by_capability()` removed for direct plugin-to-plugin coupling (except swappable providers) - [ ] `PluginMeta` gains `optional_dependencies` field - [ ] Plugin graph (#54) shows all real edges without runtime scanning heuristics - [ ] No plugin uses `os.environ` directly for secrets (deferred to #50 if needed) ## Phasing suggestion 1. **Add `loop.tools` extension point** — plugins register own tools, `tools` plugin becomes a thin executor 2. **Add `optional_dependencies` to PluginMeta** — declare what's optional vs required 3. **Audit and fix all plugins** — declare real dependencies, remove `get_by_capability()` where inappropriate 4. **Add `secrets` and `system_deps` to PluginMeta** — from #50

doxios commented

2026-02-22 10:23:58 +00:00

Collaborator

Architecture Review: Plugin Dependencies

Verdict: NEEDS-REVISION ✏️

The problem is real and well-diagnosed. The solution direction is right, but there's a simpler path using existing patterns.

✅ Problem Analysis: Confirmed

I verified the issue in code:

tools/plugin.py (lines 83-117) hardcodes wallet tool definitions:

TOOL_DEFINITIONS = [
    # ... file/shell tools ...
    {
        "type": "function",
        "function": {
            "name": "wallet_balance",
            "description": "Check wallet balance in sats",
            ...
        },
    },
    # wallet_pay, wallet_receive also hardcoded here
]

tools/plugin.py (line 245) uses service locator without declaring the dependency:

def _get_wallet(self):
    if self._registry:
        return self._registry.get_by_capability("wallet")
    return None

Meanwhile, wallet/plugin.py declares:

meta = PluginMeta(
    id="wallet",
    capabilities=["wallet"],
    dependencies=["config"],  # No idea tools depends on it
)

This is exactly the hidden coupling described. The dependency graph shows no edge, but runtime coupling exists.

🔍 Existing Pattern: ToolProvider Interface

However, there's already a better pattern in the codebase that the proposal overlooks:

knowledge/plugin.py implements ToolProvider correctly:

class KnowledgePlugin(Plugin, ToolProvider):
    meta = PluginMeta(
        id="knowledge",
        capabilities=["knowledge", "tools"],  # <-- declares it provides tools
        ...
    )

    def get_definitions(self) -> list[dict]:
        return KNOWLEDGE_TOOLS  # <-- owns its own tools

    def execute(self, tool_name: str, args: dict) -> str:
        # <-- executes its own tools

And loop/plugin.py already aggregates via AggregatedToolProvider:

def _get_tools(self) -> Optional["AggregatedToolProvider"]:
    if self._registry:
        providers = self._registry.all_with_capability("tools")
        if providers:
            return AggregatedToolProvider(providers)

This pattern works. Knowledge plugin tools are discovered and executed correctly.

🛠️ Suggested Fix: Simpler Than Proposed

Skip loop.tools extension point. The ToolProvider interface already serves this purpose. Instead:

1. Make wallet implement ToolProvider (like knowledge does)

# wallet/plugin.py
from ..interfaces import ToolProvider

WALLET_TOOLS = [
    {"type": "function", "function": {"name": "wallet_balance", ...}},
    {"type": "function", "function": {"name": "wallet_pay", ...}},
    {"type": "function", "function": {"name": "wallet_receive", ...}},
]

class WalletPlugin(Plugin, WalletProvider, ToolProvider):
    meta = PluginMeta(
        id="wallet",
        capabilities=["wallet", "tools"],  # <-- add "tools"
        ...
    )

    def get_definitions(self) -> list[dict]:
        return WALLET_TOOLS

    def execute(self, tool_name: str, args: dict) -> str:
        if tool_name == "wallet_balance":
            return f"Balance: {self.get_balance()} sats"
        # etc.

    @property
    def restart_requested(self) -> bool:
        return False

2. Remove wallet tools from tools plugin

The tools plugin becomes a clean shell/file executor - no more coupling to wallet or knowledge.

3. Add `optional_dependencies` to PluginMeta (agree with proposal)

@dataclass
class PluginMeta:
    # existing fields...
    optional_dependencies: list[str] = field(default_factory=list)

This is valuable for cases where you want to declare "I can use this if available" without requiring it.

⚠️ Concerns With Original Proposal

loop.tools extension point is redundant — The ToolProvider interface + all_with_capability("tools") already does this. Adding another mechanism creates two ways to do the same thing.
"Eliminate get_by_capability()" — This is too aggressive. get_by_capability() is correct for swappable providers (llm, communication). The problem isn't the pattern, it's undeclared usage. With optional_dependencies, the graph becomes complete.
The tools plugin isn't purely a "god object" — It does aggregate from other ToolProviders correctly (line 154-168). The issue is specific: it hardcodes wallet tools instead of wallet providing them.

✅ What I Agree With

Proposed	My Take
`optional_dependencies` field	✅ Yes, add this
Each plugin owns its tools	✅ Yes, via existing `ToolProvider` interface
Audit and fix plugins	✅ Yes, starting with wallet
`secrets` / `system_deps` (deferred)	✅ Good to defer to #50

📋 Revised Acceptance Criteria

WalletPlugin implements ToolProvider, moves tools from tools plugin
PluginMeta gains optional_dependencies: list[str] field
tools plugin removes all wallet-related code (definitions + executors)
Plugin graph shows wallet→nothing (decoupled), tools→config only
Document the ToolProvider pattern in CONTRIBUTING.md for future plugin authors

🔄 Migration Path

Phase 1: Add optional_dependencies to PluginMeta (backwards compatible)

Phase 2: Make WalletPlugin implement ToolProvider, remove wallet tools from tools plugin

Phase 3: Audit other plugins for similar patterns (I didn't find others, but worth checking)

Reviewed by examining: base.py, interfaces.py, registry.py, tools/plugin.py, wallet/plugin.py, knowledge/plugin.py, loop/plugin.py

## Architecture Review: Plugin Dependencies ### Verdict: NEEDS-REVISION ✏️ The problem is real and well-diagnosed. The solution direction is right, but there's a simpler path using existing patterns. --- ### ✅ Problem Analysis: Confirmed I verified the issue in code: **tools/plugin.py (lines 83-117)** hardcodes wallet tool definitions: ```python TOOL_DEFINITIONS = [ # ... file/shell tools ... { "type": "function", "function": { "name": "wallet_balance", "description": "Check wallet balance in sats", ... }, }, # wallet_pay, wallet_receive also hardcoded here ] ``` **tools/plugin.py (line 245)** uses service locator without declaring the dependency: ```python def _get_wallet(self): if self._registry: return self._registry.get_by_capability("wallet") return None ``` Meanwhile, **wallet/plugin.py** declares: ```python meta = PluginMeta( id="wallet", capabilities=["wallet"], dependencies=["config"], # No idea tools depends on it ) ``` This is exactly the hidden coupling described. The dependency graph shows no edge, but runtime coupling exists. --- ### 🔍 Existing Pattern: ToolProvider Interface However, there's already a better pattern in the codebase that the proposal overlooks: **knowledge/plugin.py** implements `ToolProvider` correctly: ```python class KnowledgePlugin(Plugin, ToolProvider): meta = PluginMeta( id="knowledge", capabilities=["knowledge", "tools"], # <-- declares it provides tools ... ) def get_definitions(self) -> list[dict]: return KNOWLEDGE_TOOLS # <-- owns its own tools def execute(self, tool_name: str, args: dict) -> str: # <-- executes its own tools ``` And **loop/plugin.py** already aggregates via `AggregatedToolProvider`: ```python def _get_tools(self) -> Optional["AggregatedToolProvider"]: if self._registry: providers = self._registry.all_with_capability("tools") if providers: return AggregatedToolProvider(providers) ``` This pattern works. Knowledge plugin tools are discovered and executed correctly. --- ### 🛠️ Suggested Fix: Simpler Than Proposed **Skip `loop.tools` extension point.** The `ToolProvider` interface already serves this purpose. Instead: #### 1. Make wallet implement ToolProvider (like knowledge does) ```python # wallet/plugin.py from ..interfaces import ToolProvider WALLET_TOOLS = [ {"type": "function", "function": {"name": "wallet_balance", ...}}, {"type": "function", "function": {"name": "wallet_pay", ...}}, {"type": "function", "function": {"name": "wallet_receive", ...}}, ] class WalletPlugin(Plugin, WalletProvider, ToolProvider): meta = PluginMeta( id="wallet", capabilities=["wallet", "tools"], # <-- add "tools" ... ) def get_definitions(self) -> list[dict]: return WALLET_TOOLS def execute(self, tool_name: str, args: dict) -> str: if tool_name == "wallet_balance": return f"Balance: {self.get_balance()} sats" # etc. @property def restart_requested(self) -> bool: return False ``` #### 2. Remove wallet tools from tools plugin The tools plugin becomes a clean shell/file executor - no more coupling to wallet or knowledge. #### 3. Add `optional_dependencies` to PluginMeta (agree with proposal) ```python @dataclass class PluginMeta: # existing fields... optional_dependencies: list[str] = field(default_factory=list) ``` This is valuable for cases where you want to declare "I can use this if available" without requiring it. --- ### ⚠️ Concerns With Original Proposal 1. **`loop.tools` extension point is redundant** — The `ToolProvider` interface + `all_with_capability("tools")` already does this. Adding another mechanism creates two ways to do the same thing. 2. **"Eliminate `get_by_capability()`"** — This is too aggressive. `get_by_capability()` is correct for swappable providers (llm, communication). The problem isn't the pattern, it's undeclared usage. With `optional_dependencies`, the graph becomes complete. 3. **The tools plugin isn't purely a "god object"** — It does aggregate from other ToolProviders correctly (line 154-168). The issue is specific: it hardcodes wallet tools instead of wallet providing them. --- ### ✅ What I Agree With | Proposed | My Take | |----------|--------| | `optional_dependencies` field | ✅ Yes, add this | | Each plugin owns its tools | ✅ Yes, via existing `ToolProvider` interface | | Audit and fix plugins | ✅ Yes, starting with wallet | | `secrets` / `system_deps` (deferred) | ✅ Good to defer to #50 | --- ### 📋 Revised Acceptance Criteria - [ ] `WalletPlugin` implements `ToolProvider`, moves tools from tools plugin - [ ] `PluginMeta` gains `optional_dependencies: list[str]` field - [ ] `tools` plugin removes all wallet-related code (definitions + executors) - [ ] Plugin graph shows wallet→nothing (decoupled), tools→config only - [ ] Document the `ToolProvider` pattern in CONTRIBUTING.md for future plugin authors --- ### 🔄 Migration Path **Phase 1:** Add `optional_dependencies` to `PluginMeta` (backwards compatible) **Phase 2:** Make `WalletPlugin` implement `ToolProvider`, remove wallet tools from tools plugin **Phase 3:** Audit other plugins for similar patterns (I didn't find others, but worth checking) --- *Reviewed by examining: base.py, interfaces.py, registry.py, tools/plugin.py, wallet/plugin.py, knowledge/plugin.py, loop/plugin.py*