fix: address critical issues found in code review

- Fix CLI tools write-back regression in main.py (silent tool dropping)
- Improve error handling in Agent toolset resolution (fail fast on invalid toolsets)
- Fix mutable state leakage in ToolsetRegistry (defensive copying)
- Fix cache collision in ToolRegistry (use registry keys not tool.name)
- Fix NameError in CLI commands (missing toolset parameters)
- Add deduplication in tool resolution
- Improve test reliability with cleanup and assertions
- Filter empty toolset names from CLI input

Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>

Author: praisonai-triage-agent[bot]

src/praisonai-agents/praisonaiagents/agent/agent.py

@@ -1664,12 +1664,24 @@ def __init__(
             try:
                 from ..toolsets import resolve_toolsets
                 toolset_tool_names = resolve_toolsets(toolsets)
-                toolset_tools = self._resolve_tool_names(toolset_tool_names)
+                # Remove duplicates with existing tools
+                existing_tool_names = set()
+                for tool in self.tools:
+                    name = getattr(tool, 'name', getattr(tool, '__name__', str(tool)))
+                    existing_tool_names.add(name)
+                
+                unique_tool_names = [name for name in toolset_tool_names if name not in existing_tool_names]
+                toolset_tools = self._resolve_tool_names(unique_tool_names)
                 self.tools.extend(toolset_tools)
                 logging.debug(f"Resolved toolsets {toolsets} to {len(toolset_tools)} tools: {[getattr(t, '__name__', str(t)) for t in toolset_tools]}")
-            except Exception as e:
+            except (ValueError, KeyError) as e:
+                raise ValueError(
+                    f"Agent '{getattr(self, 'display_name', 'unknown')}' failed to resolve toolsets {toolsets}: {e}. "
+                    "Verify names with praisonaiagents.toolsets.list_toolsets()."
+                ) from e
+            except ImportError as e:
                 logging.warning(f"Failed to resolve toolsets {toolsets}: {e}")
-                # Continue without toolsets rather than failing
+                # Continue without toolsets if module is unavailable
 
         # Inject default tools for autonomy mode (after self.tools is initialized)
         # ONLY inject if caller didn't provide tools - avoid duplicates with CLI/wrapper tools

src/praisonai-agents/praisonaiagents/tools/registry.py

@@ -111,9 +111,13 @@ def unregister(self, name: str) -> bool:
         with self._lock:
             if name in self._tools:
                 del self._tools[name]
+                # Evict cache entry to prevent stale growth
+                self._availability_cache.pop(name, None)
                 return True
             if name in self._functions:
                 del self._functions[name]
+                # Evict cache entry to prevent stale growth
+                self._availability_cache.pop(name, None)
                 return True
             return False
 
@@ -179,11 +183,11 @@ def list_available_tools(
             current_time = time.time()
 
             # Check BaseTool instances
-            for tool in self._tools.values():
+            for registered_name, tool in self._tools.items():
                 # Check if tool has availability checking capability
                 if hasattr(tool, 'check_availability'):
-                    # Check cache first
-                    cache_entry = self._availability_cache.get(tool.name)
+                    # Check cache first using registry key (not tool.name)
+                    cache_entry = self._availability_cache.get(registered_name)
                     if cache_entry is not None:
                         cached_result, cached_time = cache_entry
                         if current_time - cached_time < ttl_seconds:
@@ -195,17 +199,17 @@ def list_available_tools(
                     # Cache miss or expired - perform availability check
                     try:
                         is_available, reason = tool.check_availability()
-                        # Cache the result
-                        self._availability_cache[tool.name] = (is_available, current_time)
+                        # Cache the result using registry key
+                        self._availability_cache[registered_name] = (is_available, current_time)
 
                         if is_available:
                             available.append(tool)
                         elif reason:
-                            logging.debug(f"Tool '{tool.name}' unavailable: {reason}")
+                            logging.debug(f"Tool '{registered_name}' unavailable: {reason}")
                     except Exception as e:
-                        logging.warning(f"Availability check failed for tool '{tool.name}': {e}")
+                        logging.warning(f"Availability check failed for tool '{registered_name}': {e}")
                         # Cache as unavailable on error
-                        self._availability_cache[tool.name] = (False, current_time)
+                        self._availability_cache[registered_name] = (False, current_time)
                 else:
                     # No availability check = always available
                     available.append(tool)

src/praisonai-agents/praisonaiagents/toolsets.py

@@ -79,8 +79,8 @@ def register_toolset(
 
             toolset = ToolsetSpec(
                 name=name,
-                tools=tools or [],
-                includes=includes or [],
+                tools=list(tools) if tools else [],
+                includes=list(includes) if includes else [],
                 description=description
             )
             self._toolsets[name] = toolset
@@ -113,7 +113,16 @@ def get_toolset(self, name: str) -> Optional[ToolsetSpec]:
         """
         with self._lock:
             self._ensure_prebuilt_loaded()
-            return self._toolsets.get(name)
+            spec = self._toolsets.get(name)
+            if spec is None:
+                return None
+            # Return defensive copy to prevent external mutation
+            return ToolsetSpec(
+                name=spec.name,
+                tools=list(spec.tools),
+                includes=list(spec.includes),
+                description=spec.description,
+            )
 
     def list_toolsets(self) -> List[str]:
         """List all registered toolset names."""
@@ -138,7 +147,15 @@ def resolve_toolset(self, name: str) -> List[str]:
         """
         with self._lock:
             self._ensure_prebuilt_loaded()
-            return self._resolve_toolset_recursive(name, set())
+            tools = self._resolve_toolset_recursive(name, set())
+            # Ensure uniqueness while preserving order
+            seen = set()
+            unique_tools = []
+            for tool in tools:
+                if tool not in seen:
+                    seen.add(tool)
+                    unique_tools.append(tool)
+            return unique_tools
 
     def resolve_toolsets(self, names: List[str]) -> List[str]:
         """Resolve multiple toolset names to a flat list of tool names.

src/praisonai/praisonai/cli/commands/chat.py

@@ -286,6 +286,7 @@ def _run_legacy_terminal_chat(
     verbose: bool = False,
     memory: bool = False,
     tools: Optional[str] = None,
+    toolset: Optional[str] = None,
     user_id: Optional[str] = None,
     session_id: Optional[str] = None,
     continue_session: bool = False,

src/praisonai/praisonai/cli/commands/run.py

@@ -125,6 +125,7 @@ def run_main(
             trace=trace,
             memory=memory,
             tools=tools,
+            toolset=toolset,
             max_tokens=max_tokens,
             output_mode=output_mode,
             approval=approval,
@@ -188,6 +189,7 @@ def _run_prompt(
     trace: bool = False,
     memory: bool = False,
     tools: Optional[str] = None,
+    toolset: Optional[str] = None,
     max_tokens: int = 16000,
     output_mode: Optional[str] = None,
     approval: Optional[str] = None,

src/praisonai/praisonai/cli/main.py

@@ -4356,15 +4356,19 @@ def handle_direct_prompt(self, prompt):
                         existing_tools = agent_config.get('tools', [])
                         if isinstance(existing_tools, list):
                             existing_tools.extend(tools_list)
+                            agent_config["tools"] = existing_tools
+                        else:
+                            agent_config["tools"] = tools_list
 
                 # Load toolsets if specified (--toolset flag)
                 if getattr(self.args, 'toolset', None):
-                    toolset_names = [name.strip() for name in self.args.toolset.split(',')]
-                    toolset_tools = self._load_toolsets(toolset_names)
+                    toolset_names = [name.strip() for name in self.args.toolset.split(',') if name.strip()]
+                    toolset_tools = self._load_toolsets(toolset_names) if toolset_names else []
                     if toolset_tools:
                         existing_tools = agent_config.get('tools', [])
                         if isinstance(existing_tools, list):
                             existing_tools.extend(toolset_tools)
+                            agent_config["tools"] = existing_tools
                         else:
                             agent_config["tools"] = toolset_tools
                         print(f"[bold cyan]Toolsets loaded: {len(toolset_tools)} tool(s) from {self.args.toolset}[/bold cyan]")

src/praisonai/praisonai/tool_resolver.py

@@ -541,9 +541,14 @@ def resolve_toolsets(self, toolset_names: List[str]) -> List[Callable]:
             # Resolve tool names to callables
             return self.resolve_many(tool_names)
 
-        except Exception as e:
-            logger.warning(f"Failed to resolve toolsets {toolset_names}: {e}")
+        except ImportError as e:
+            logger.warning(f"Toolset support unavailable: {e}")
             return []
+        except (ValueError, KeyError) as e:
+            raise ValueError(
+                f"Failed to resolve toolsets {toolset_names}: {e}. "
+                "Check toolset names and includes."
+            ) from e
 
     def resolve_tools_and_toolsets(
         self, 
@@ -569,7 +574,16 @@ def resolve_tools_and_toolsets(
         if toolset_names:
             all_tools.extend(self.resolve_toolsets(toolset_names))
 
-        return all_tools
+        # Deduplicate while preserving order
+        deduped: List[Callable] = []
+        seen: set[int] = set()
+        for tool in all_tools:
+            marker = id(tool)
+            if marker in seen:
+                continue
+            seen.add(marker)
+            deduped.append(tool)
+        return deduped
 
 
 # Context-local resolver for multi-project safety

test_toolsets.py

@@ -20,37 +20,50 @@ def test_toolset_basic():
         )
         print("✓ Successfully imported toolset functions")
 
-        # Test registering a custom toolset
-        register_toolset(
-            "test_toolset",
-            tools=["tool1", "tool2"],
-            description="Test toolset"
-        )
-        print("✓ Successfully registered custom toolset")
-        
-        # Test resolving the toolset
-        tools = resolve_toolset("test_toolset")
-        assert tools == ["tool1", "tool2"]
-        print("✓ Successfully resolved toolset")
-        
-        # Test listing toolsets (should include prebuilt ones)
-        toolset_list = list_toolsets()
-        assert "test_toolset" in toolset_list
-        assert "web" in toolset_list
-        assert "research" in toolset_list
-        print(f"✓ Found {len(toolset_list)} toolsets: {toolset_list}")
-        
-        # Test composition via includes
-        register_toolset(
-            "composed_test",
-            tools=["tool3"],
-            includes=["test_toolset"]
-        )
-        composed_tools = resolve_toolset("composed_test")
-        assert "tool1" in composed_tools
-        assert "tool2" in composed_tools
-        assert "tool3" in composed_tools
-        print("✓ Successfully tested toolset composition")
+        # Test registering a custom toolset with unique names to avoid pollution
+        import uuid
+        test_toolset_name = f"test_toolset_{uuid.uuid4().hex[:8]}"
+        composed_toolset_name = f"composed_test_{uuid.uuid4().hex[:8]}"
+        
+        try:
+            register_toolset(
+                test_toolset_name,
+                tools=["tool1", "tool2"],
+                description="Test toolset"
+            )
+            print("✓ Successfully registered custom toolset")
+            
+            # Test resolving the toolset
+            tools = resolve_toolset(test_toolset_name)
+            assert tools == ["tool1", "tool2"]
+            print("✓ Successfully resolved toolset")
+            
+            # Test listing toolsets (should include prebuilt ones)
+            toolset_list = list_toolsets()
+            assert test_toolset_name in toolset_list
+            assert "web" in toolset_list
+            assert "research" in toolset_list
+            print(f"✓ Found {len(toolset_list)} toolsets: {toolset_list}")
+            
+            # Test composition via includes
+            register_toolset(
+                composed_toolset_name,
+                tools=["tool3"],
+                includes=[test_toolset_name]
+            )
+            composed_tools = resolve_toolset(composed_toolset_name)
+            assert "tool1" in composed_tools
+            assert "tool2" in composed_tools
+            assert "tool3" in composed_tools
+            print("✓ Successfully tested toolset composition")
+        finally:
+            # Cleanup to avoid registry pollution
+            from praisonaiagents.toolsets import unregister_toolset
+            try:
+                unregister_toolset(composed_toolset_name)
+                unregister_toolset(test_toolset_name)
+            except Exception:
+                pass
 
         print("All basic toolset tests passed!\n")
         return True
@@ -95,6 +108,11 @@ def test_agent_integration():
 
         print(f"✓ Agent has {len(agent.tools)} tools: {tool_names}")
 
+        # Verify expected tools are present
+        assert "internet_search" in tool_names, f"internet_search was not resolved from toolset. Available: {tool_names}"
+        assert "read_file" in tool_names, f"read_file was not resolved from toolset. Available: {tool_names}"
+        print("✓ Verified expected tools are present")
+        
         # Test mixing tools and toolsets
         agent2 = Agent(
             name="test_agent2", 
@@ -104,6 +122,18 @@ def test_agent_integration():
         )
         print("✓ Successfully created Agent with both tools and toolsets")
 
+        # Check tools in agent2
+        tool_names2 = []
+        for tool in agent2.tools:
+            if hasattr(tool, '__name__'):
+                tool_names2.append(tool.__name__)
+            elif hasattr(tool, 'name'):
+                tool_names2.append(tool.name)
+        
+        assert "write_file" in tool_names2, f"explicit tool write_file missing. Available: {tool_names2}"
+        assert "internet_search" in tool_names2, f"toolset tool internet_search missing. Available: {tool_names2}"
+        print("✓ Verified mixed tools and toolsets work correctly")
+        
         print("All agent integration tests passed!\n")
         return True