fix(tools): propagate tool registry to subagents

[paoloanzn]

Summary

• Fix: SubagentManager is created with an empty ToolRegistry (NewToolRegistry() at subagent.go:48) and SetTools() is never called after the multi-agent refactor in registerSharedTools(). This causes every tool invocation from a subagent to return "tool not found", making subagents completely non-functional.
• Root cause: When registerSharedTools() was introduced, the subagentManager.SetTools() call that populated the subagent's tools was dropped. The SetTools() and RegisterTool() methods exist on SubagentManager but are never invoked.
• Fix approach: Add ToolRegistry.Clone() to create an independent snapshot of the parent agent's tool registry, then call subagentManager.SetTools(agent.Tools.Clone()) immediately after manager creation but before spawn/spawn_status registration — giving subagents access to file, exec, web, and other tools while preventing recursive subagent spawning.

Changes

File	Change
pkg/tools/registry.go	Add Clone() method — creates independent shallow copy of a tool registry
pkg/agent/loop.go	Add subagentManager.SetTools(agent.Tools.Clone()) in registerSharedTools()
pkg/tools/registry_test.go	3 new tests: clone isolation, empty clone, hidden tool state preservation

Why Clone instead of sharing the pointer?

spawn and spawn_status tools are registered on agent.Tools after the subagent manager is created (lines 248-251 of loop.go). Sharing the registry pointer would give subagents access to spawn tools → recursive subagent spawning. Clone() captures a snapshot with only the tools registered at that point.

Test plan

• TestToolRegistry_Clone — verifies parent/clone isolation in both directions
• TestToolRegistry_Clone_Empty — verifies cloning an empty registry
• TestToolRegistry_Clone_PreservesHiddenToolState — verifies hidden tool TTL behavior is preserved
• All existing registry tests still pass
• pkg/tools and pkg/agent packages build cleanly

Note: pkg/tools test suite has a pre-existing build failure in cron_test.go:229 (SubscribeOutbound undefined on *bus.MessageBus) that exists on main before this change. Our new tests are verified passing on a branch where cron_test.go compiles.

🤖 Generated with Claude Code

[CLAassistant]

All committers have signed the CLA.

[yinwm]

Code Review

This is a critical bugfix that resolves the issue where subagents were completely non-functional due to missing tool propagation. The changes are minimal and well-targeted.

---

✅ What's Good

1. Accurate root cause analysis - Correctly identified that SubagentManager was created with an empty ToolRegistry and SetTools() was never called after the multi-agent refactor.

2. Correct solution - Using Clone() to create a snapshot rather than sharing the pointer effectively prevents recursive subagent spawning while giving subagents access to all necessary tools.

3. Proper lock usage - Using RLock() instead of Lock() since we're only reading.

4. Performance optimization - Pre-allocating the map with make(map[string]*ToolEntry, len(r.tools)) avoids multiple allocations.

5. Good test coverage - Tests verify bidirectional isolation, edge case (empty clone), and hidden tool state preservation.

---

⚠️ Items to Consider

1. version field not copied

clone := &ToolRegistry{
    tools: make(map[string]*ToolEntry, len(r.tools)),
    // version field defaults to 0
}

The version field in ToolRegistry is used for cache invalidation. The clone starts with version=0, which should be fine (the clone is a new independent registry managing its own version). However, it might be worth adding a note in the comment explaining this behavior.

2. Test doesn't verify TTL value is correctly copied

The current test TestToolRegistry_Clone_PreservesHiddenToolState only checks TTL=0 case. Consider adding a test to verify TTL > 0 values are correctly preserved:

func TestToolRegistry_Clone_PreservesTTL(t *testing.T) {
    r := NewToolRegistry()
    tool := newMockTool("hidden_tool", "hidden")
    r.RegisterHidden(tool)
    // Manually set TTL > 0
    if entry, ok := r.tools["hidden_tool"]; ok {
        entry.TTL = 5
    }

    clone := r.Clone()
    if entry, ok := clone.tools["hidden_tool"]; ok {
        if entry.TTL != 5 {
            t.Errorf("expected TTL=5, got %d", entry.TTL)
        }
    }
}

---

💡 Minor Suggestion (Non-blocking)

The comment could mention that version is reset:

// Clone creates an independent copy of the registry containing the same tool
// entries (shallow copy of each ToolEntry). This is used to give subagents a
// snapshot of the parent agent's tools without sharing the same registry —
// tools registered on the parent after cloning (e.g. spawn, spawn_status)
// will NOT be visible to the clone, preventing recursive subagent spawning.
// The version counter is reset to 0 in the clone as it's a new independent registry.

---

🎯 Verdict

Aspect	Rating
Correctness	✅ Fix logic is correct
Thread Safety	✅ No concurrency issues
Maintainability	✅ Code is clear
Test Coverage	⚠️ Consider adding TTL value test

LGTM with minor nits — Ready to merge, but recommend adding a test case for TTL value preservation for completeness.

🤖 Generated with Claude Code

[yinwm]

plz fix Linter and tests @paoloanzn

[paoloanzn]

@yinwm Thanks for the review! I've pushed a follow-up commit addressing both the CI failures and your review feedback:

Fixes:

• Linter + Tests: Fixed pre-existing build error in cron_test.go:229 — replaced non-existent SubscribeOutbound(ctx) with select on OutboundChan(), matching the MessageBus channel API
• TTL test: Added TestToolRegistry_Clone_PreservesTTLValue per your review suggestion
• Doc comment: Added version reset note to Clone() godoc

All tests pass locally (go test ./pkg/tools/ -count=1 -short ✅).

CI may need approval to re-run since this is a fork PR.

[yinwm]

@paoloanzn sorry , there's a conflicts, plz fix

[yinwm]

LGTM

[yinwm]

thanks for this pr

[paoloanzn]

thanks for this pr

my pleasure to contribute. best agent system out there rn 😄

[Orgmar]

@paoloanzn Great debugging work here. Tracking down the empty ToolRegistry after the multi-agent refactor and using Clone() to snapshot the parent tools while preventing recursive spawning is a well thought out solution. The isolation tests are thorough too.

We have a PicoClaw Dev Group on Discord for contributors to collaborate more directly. Want to join? Send an email to support@sipeed.com with subject [Join PicoClaw Dev Group] paoloanzn and we'll send you the invite link!

[paoloanzn]

@paoloanzn Great debugging work here. Tracking down the empty ToolRegistry after the multi-agent refactor and using Clone() to snapshot the parent tools while preventing recursive spawning is a well thought out solution. The isolation tests are thorough too.

We have a PicoClaw Dev Group on Discord for contributors to collaborate more directly. Want to join? Send an email to support@sipeed.com with subject [Join PicoClaw Dev Group] paoloanzn and we'll send you the invite link!

thank you for the invite, i've sent the email!