From 9f62f9c9c8a49e5cb57d1eac01c90f64380b2b4c Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Sun, 5 Apr 2026 08:59:59 +0000 Subject: [PATCH] fix(skills): replace get_tool() calls with get() in SkillRegistry validation methods SkillRegistry.validate_plan() and validate_skill() called the non-existent ToolRegistry.get_tool() method, causing AttributeError at runtime. The correct method is ToolRegistry.get(), which is already used by SkillRegistry.refresh(). Changes: - Replace self._tool_registry.get_tool(entry.name) with self._tool_registry.get(entry.name) in validate_plan() - Replace self._tool_registry.get_tool(ref) with self._tool_registry.get(ref) in validate_skill() - Update Behave BDD tests to use mock.get instead of mock.get_tool - Add new BDD scenarios covering validate_skill() happy path, not-found path, unregistered include path, and no-tool-registry path - Remove stale get_tool mock setup from test helpers ISSUES CLOSED: #2914 --- .../skills_registry_coverage_boost.feature | 34 +++++++++++++- .../skills_registry_coverage_boost_steps.py | 45 +++++++++++++------ src/cleveragents/skills/registry.py | 4 +- 3 files changed, 66 insertions(+), 17 deletions(-) diff --git a/features/skills_registry_coverage_boost.feature b/features/skills_registry_coverage_boost.feature index bed3f7a49..da3edef2d 100644 --- a/features/skills_registry_coverage_boost.feature +++ b/features/skills_registry_coverage_boost.feature @@ -2,7 +2,7 @@ Feature: Skill Registry coverage boost As a developer improving test coverage I want to exercise the uncovered code paths in SkillRegistry - So that lines 270-278, 314-315, and 324 are covered + So that validate_plan(), validate_skill(), refresh(), and related paths are covered Background: Given a fresh SkillRegistry instance @@ -33,7 +33,7 @@ Feature: Skill Registry coverage boost And a registered skill "local/inline-skill" having only an inline tool When I validate a plan referencing skill "local/inline-skill" Then the validation errors should be empty - And the tool registry mock get_tool should not have been called + And the tool registry mock get should not have been called @validate_plan_tool_registry Scenario: validate_plan skips mcp-prefixed entries during tool registry validation @@ -95,3 +95,33 @@ Feature: Skill Registry coverage boost And the tool registry mock returns a tool for "local/lint" When I refresh the coverage boost skill "local/agent-check" Then the refresh result should have "local/agent-check" in refreshed + + # --------------------------------------------------------------------------- + # validate_skill with tool registry configured + # --------------------------------------------------------------------------- + + @validate_skill_tool_registry + Scenario: validate_skill reports missing tool ref when tool registry returns None + Given the registry has a tool registry mock configured + And the tool registry mock returns None for tool "local/missing-tool" + When I validate a skill definition with tool ref "local/missing-tool" + Then the skills validation errors should contain "Tool reference 'local/missing-tool' not found in tool registry" + + @validate_skill_tool_registry + Scenario: validate_skill passes when all tool refs exist in tool registry + Given the registry has a tool registry mock configured + And the tool registry mock returns a tool for "local/existing-tool" + When I validate a skill definition with tool ref "local/existing-tool" + Then the validation errors should be empty + + @validate_skill_tool_registry + Scenario: validate_skill reports unregistered included skill + Given a fresh SkillRegistry instance + When I validate a skill definition that includes unregistered skill "local/ghost" + Then the skills validation errors should contain "Included skill 'local/ghost' is not registered" + + @validate_skill_tool_registry + Scenario: validate_skill passes with no tool registry configured + Given a fresh SkillRegistry instance + When I validate a skill definition with tool ref "local/any-tool" + Then the validation errors should be empty diff --git a/features/steps/skills_registry_coverage_boost_steps.py b/features/steps/skills_registry_coverage_boost_steps.py index 9739c0c9f..1ee7c52ba 100644 --- a/features/steps/skills_registry_coverage_boost_steps.py +++ b/features/steps/skills_registry_coverage_boost_steps.py @@ -1,9 +1,10 @@ """Step definitions for SkillRegistry coverage boost. Targets uncovered lines in src/cleveragents/skills/registry.py: - - Lines 270-278: validate_plan tool-registry validation loop - - Lines 314-315: refresh exception handler for ValueError/RuntimeError - - Line 324: refresh skip for inline/mcp/agent_skill entries + - validate_plan tool-registry validation loop (get() call) + - validate_skill tool-registry validation loop (get() call) + - refresh exception handler for ValueError/RuntimeError + - refresh skip for inline/mcp/agent_skill entries """ from __future__ import annotations @@ -90,7 +91,6 @@ def step_fresh_registry(context: Context) -> None: @given("the registry has a tool registry mock configured") def step_tool_registry_mock(context: Context) -> None: mock = MagicMock() - mock.get_tool.return_value = None mock.get.return_value = None context.tool_registry_mock = mock context.skill_reg = SkillRegistry(tool_registry=mock) @@ -174,7 +174,7 @@ def step_skill_agent_and_ref(context: Context, name: str, ref: str) -> None: @given('the tool registry mock returns None for tool "{tool_name}"') def step_mock_returns_none(context: Context, tool_name: str) -> None: mock: MagicMock = context.tool_registry_mock - mock.get_tool.return_value = None + mock.get.return_value = None @given('the tool registry mock returns a tool for "{tool_name}"') @@ -183,17 +183,11 @@ def step_mock_returns_tool(context: Context, tool_name: str) -> None: tool_obj = MagicMock() tool_obj.name = tool_name - def _get_tool(name: str) -> Any: - if name == tool_name: - return tool_obj - return None - def _get(name: str) -> Any: if name == tool_name: return tool_obj return None - mock.get_tool.side_effect = _get_tool mock.get.side_effect = _get @@ -241,10 +235,10 @@ def step_errors_contain(context: Context, message: str) -> None: # REMOVED: Duplicate @then step - already defined in skill_context_steps.py -@then("the tool registry mock get_tool should not have been called") +@then("the tool registry mock get should not have been called") def step_mock_not_called(context: Context) -> None: mock: MagicMock = context.tool_registry_mock - mock.get_tool.assert_not_called() + mock.get.assert_not_called() # --------------------------------------------------------------------------- @@ -285,3 +279,28 @@ def step_refresh_refreshed(context: Context, name: str) -> None: f"Expected '{name}' in refreshed, got refreshed={result.refreshed}, " f"failed={result.failed}" ) + + +# --------------------------------------------------------------------------- +# When: validate_skill +# --------------------------------------------------------------------------- + + +@when('I validate a skill definition with tool ref "{ref}"') +def step_validate_skill_with_ref(context: Context, ref: str) -> None: + skill = _make_skill(name="local/test-validate", tool_refs=[ref]) + defn = _make_definition(skill) + context.validation_errors = context.skill_reg.validate_skill(defn) + + + +@when('I validate a skill definition that includes unregistered skill "{include_name}"') +def step_validate_skill_unregistered_include( + context: Context, include_name: str +) -> None: + skill = _make_skill( + name="local/test-include", + includes=[SkillInclude(name=include_name)], + ) + defn = _make_definition(skill, resolve=False) + context.validation_errors = context.skill_reg.validate_skill(defn) diff --git a/src/cleveragents/skills/registry.py b/src/cleveragents/skills/registry.py index f0c0f7a3f..773cf6696 100644 --- a/src/cleveragents/skills/registry.py +++ b/src/cleveragents/skills/registry.py @@ -271,7 +271,7 @@ class SkillRegistry: if not entry.is_inline and not entry.name.startswith( ("mcp:", "agent_skill:") ): - tool = self._tool_registry.get_tool(entry.name) + tool = self._tool_registry.get(entry.name) if tool is None: errors.append( f"Tool '{entry.name}' in skill " @@ -368,7 +368,7 @@ class SkillRegistry: # Validate tool_refs against tool registry for ref in skill.skill.tool_refs: if self._tool_registry is not None: - tool = self._tool_registry.get_tool(ref) + tool = self._tool_registry.get(ref) if tool is None: errors.append(f"Tool reference '{ref}' not found in tool registry") -- 2.52.0