Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions src/app/endpoints/skills.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
"""Handler for REST API call to list loaded agent skills."""

from typing import Annotated, Any

from fastapi import APIRouter, Request
from fastapi.params import Depends

from authentication import get_auth_dependency
from authentication.interface import AuthTuple
from authorization.middleware import authorize
from configuration import configuration
from log import get_logger
from models.api.responses.constants import UNAUTHORIZED_OPENAPI_EXAMPLES
from models.api.responses.error import (
ForbiddenResponse,
InternalServerErrorResponse,
UnauthorizedResponse,
)
from models.api.responses.successful import SkillsResponse
from models.config import Action
from utils.endpoints import check_configuration_loaded
from utils.pydantic_ai_helpers import get_skills_metadata

logger = get_logger(__name__)
router = APIRouter(tags=["skills"])


skills_responses: dict[int | str, dict[str, Any]] = {
200: SkillsResponse.openapi_response(),
401: UnauthorizedResponse.openapi_response(examples=UNAUTHORIZED_OPENAPI_EXAMPLES),
403: ForbiddenResponse.openapi_response(examples=["endpoint"]),
500: InternalServerErrorResponse.openapi_response(examples=["configuration"]),
}


@router.get("/skills", responses=skills_responses)
@authorize(Action.GET_SKILLS)
async def skills_endpoint_handler(
request: Request,
auth: Annotated[AuthTuple, Depends(get_auth_dependency())],
) -> SkillsResponse:
"""Handle requests to the /skills endpoint.

Process GET requests to the /skills endpoint, returning a list of loaded
agent skills with their metadata (name, description).

### Parameters:
- request: The incoming HTTP request (used by middleware).
- auth: Authentication tuple from the auth dependency (used by middleware).

### Raises:
- HTTPException: with status 401 for unauthorized access.
- HTTPException: with status 403 if permission is denied.
- HTTPException: with status 500 and a detail object containing `response`
and `cause` when service configuration is wrong or incomplete.

### Returns:
- SkillsResponse: An object containing the list of loaded skills.
"""
_ = auth
_ = request

check_configuration_loaded(configuration)

skills_metadata = get_skills_metadata(configuration.configuration.skills)
return SkillsResponse(skills=skills_metadata)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
1 change: 1 addition & 0 deletions src/app/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
"description": "Saved prompts configuration and management.",
},
{"name": "shields", "description": "Safety shields."},
{"name": "skills", "description": "Agent skills."},
{"name": "streaming_query", "description": "Streaming query (SSE)."},
{"name": "streaming_query_interrupt", "description": "Streaming interrupt."},
{"name": "tools", "description": "Tools."},
Expand Down
2 changes: 2 additions & 0 deletions src/app/routers.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
root,
saved_prompts,
shields,
skills,
stream_interrupt,
streaming_query,
tools,
Expand Down Expand Up @@ -55,6 +56,7 @@ def include_routers(app: FastAPI) -> None:
app.include_router(mcp_auth.router, prefix="/v1")
app.include_router(mcp_servers.router, prefix="/v1")
app.include_router(shields.router, prefix="/v1")
app.include_router(skills.router, prefix="/v1")
app.include_router(providers.router, prefix="/v1")
app.include_router(prompts.router, prefix="/v1")
app.include_router(rags.router, prefix="/v1")
Expand Down
2 changes: 2 additions & 0 deletions src/models/api/responses/successful/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
RAGInfoResponse,
RAGListResponse,
ShieldsResponse,
SkillsResponse,
ToolsResponse,
)
from models.api.responses.successful.configuration import ConfigurationResponse
Expand Down Expand Up @@ -100,6 +101,7 @@
"SavedPromptsConfigResponse",
"SavedPromptsListResponse",
"ShieldsResponse",
"SkillsResponse",
"StatusResponse",
"StreamingInterruptResponse",
"StreamingQueryResponse",
Expand Down
27 changes: 27 additions & 0 deletions src/models/api/responses/successful/catalog.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,33 @@
from models.api.responses.successful.bases import AbstractSuccessfulResponse


class SkillsResponse(AbstractSuccessfulResponse):
"""Model representing a response to skills request."""
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

skills: list[dict[str, Any]] = Field(
description="List of loaded skills with metadata",
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

model_config = {
"json_schema_extra": {
"examples": [
{
"skills": [
{
"name": "code-review",
"description": "Review code for quality and security",
},
{
"name": "openshift-troubleshooting",
"description": "Troubleshoot OpenShift cluster issues",
},
],
}
]
}
}


class ModelsResponse(AbstractSuccessfulResponse):
"""Model representing a response to models request."""

Expand Down
1 change: 1 addition & 0 deletions src/models/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -1273,6 +1273,7 @@ class Action(str, Enum):
FEEDBACK = "feedback"
GET_MODELS = "get_models"
GET_TOOLS = "get_tools"
GET_SKILLS = "get_skills"
GET_SHIELDS = "get_shields"
LIST_PROVIDERS = "list_providers"
GET_PROVIDER = "get_provider"
Expand Down
20 changes: 20 additions & 0 deletions src/utils/pydantic_ai_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,26 @@ def _capability_tools_from_toolset(toolset: Any) -> list[dict[str, Any]]:
return tool_dicts


def get_skills_metadata(
skills: Optional[SkillsConfiguration],
) -> list[dict[str, Any]]:
"""Return metadata for all loaded skills.

Parameters:
skills: Agent skills configuration from LCS, or None when skills are disabled.

Returns:
List of dicts with ``name`` and ``description`` for each loaded skill.
"""
capability = _skills_capability(skills)
if capability is None:
return []
return [
{"name": skill.name, "description": skill.description}
for skill in capability.toolset.skills.values()
]


def get_agent_capability_tools(
skills: Optional[SkillsConfiguration],
) -> list[dict[str, Any]]:
Expand Down
117 changes: 117 additions & 0 deletions tests/unit/app/endpoints/test_skills.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
"""Unit tests for skills endpoint."""

from pathlib import Path

import pytest
from fastapi import Request
from pytest_mock import MockerFixture

from app.endpoints.skills import skills_endpoint_handler
from authentication.interface import AuthTuple
from models.api.responses.successful import SkillsResponse
from models.config import SkillsConfiguration
from tests.unit.utils.auth_helpers import mock_authorization_resolvers

MOCK_AUTH: AuthTuple = ("mock_user_id", "mock_username", True, "mock_token")


@pytest.mark.asyncio
async def test_skills_loaded(
mocker: MockerFixture,
tmp_path: Path,
) -> None:
"""Test that loaded skills are returned with name and description."""
mock_authorization_resolvers(mocker)

skills_root = tmp_path / "skills"
for name, desc in [
("code-review", "Review code for quality and security"),
("openshift-troubleshooting", "Troubleshoot OpenShift cluster issues"),
]:
skill_dir = skills_root / name
skill_dir.mkdir(parents=True)
(skill_dir / "SKILL.md").write_text(
f"---\nname: {name}\ndescription: {desc}\n---\n\nInstructions.\n",
encoding="utf-8",
)

skills_config = SkillsConfiguration(paths=[skills_root])
mock_config = mocker.patch("app.endpoints.skills.configuration")
mock_config.configuration.skills = skills_config

request = Request(scope={"type": "http"})
response = await skills_endpoint_handler(auth=MOCK_AUTH, request=request)
Comment on lines +38 to +63

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add route-level authorization coverage.

These tests call the handler directly with a hand-built request and auth tuple, so they do not verify /v1/skills registration, FastAPI serialization, or dependency wiring. Add at least one client-level test covering the mounted route and denied access; keep the direct filesystem tests for metadata cases.

Based on the PR objective, this endpoint must be both versioned and authorized.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/app/endpoints/test_skills.py` around lines 18 - 43, The existing
test_skills_loaded direct-handler coverage does not validate route registration,
serialization, or authorization wiring. Add a client-level test for the mounted
/v1/skills route that verifies an authorized request succeeds and a denied
request is rejected, while retaining the direct filesystem-based tests for
metadata behavior. Ensure the route remains versioned under /v1 and uses the
configured authorization dependency.


assert isinstance(response, SkillsResponse)
assert len(response.skills) == 2
names = {s["name"] for s in response.skills}
assert names == {"code-review", "openshift-troubleshooting"}
for skill in response.skills:
assert "name" in skill
assert "description" in skill


@pytest.mark.asyncio
async def test_no_skills_configured(
mocker: MockerFixture,
) -> None:
"""Test that an empty list is returned when no skills are configured."""
mock_authorization_resolvers(mocker)

mock_config = mocker.patch("app.endpoints.skills.configuration")
mock_config.configuration.skills = None

request = Request(scope={"type": "http"})
response = await skills_endpoint_handler(auth=MOCK_AUTH, request=request)

assert isinstance(response, SkillsResponse)
assert response.skills == []


@pytest.mark.asyncio
async def test_empty_skills_paths(
mocker: MockerFixture,
) -> None:
"""Test that an empty list is returned when skills paths are empty."""
mock_authorization_resolvers(mocker)

mock_config = mocker.patch("app.endpoints.skills.configuration")
mock_config.configuration.skills = SkillsConfiguration(paths=[])

request = Request(scope={"type": "http"})
response = await skills_endpoint_handler(auth=MOCK_AUTH, request=request)

assert isinstance(response, SkillsResponse)
assert response.skills == []


@pytest.mark.asyncio
async def test_skills_with_references(
mocker: MockerFixture,
tmp_path: Path,
) -> None:
"""Test that skills with references/ subdirectory are listed correctly."""
mock_authorization_resolvers(mocker)

skills_root = tmp_path / "skills"
skill_dir = skills_root / "rhdh-dynamic-plugins"
skill_dir.mkdir(parents=True)
(skill_dir / "SKILL.md").write_text(
"---\nname: rhdh-dynamic-plugins\ndescription: RHDH dynamic plugins guide\n---\n\nInstructions.\n",
encoding="utf-8",
)
refs_dir = skill_dir / "references"
refs_dir.mkdir()
(refs_dir / "plugin-list.md").write_text("# Plugins\n- plugin-a\n", encoding="utf-8")

skills_config = SkillsConfiguration(paths=[skills_root])
mock_config = mocker.patch("app.endpoints.skills.configuration")
mock_config.configuration.skills = skills_config

request = Request(scope={"type": "http"})
response = await skills_endpoint_handler(auth=MOCK_AUTH, request=request)

assert isinstance(response, SkillsResponse)
assert len(response.skills) == 1
assert response.skills[0]["name"] == "rhdh-dynamic-plugins"
assert response.skills[0]["description"] == "RHDH dynamic plugins guide"
20 changes: 20 additions & 0 deletions tests/unit/utils/test_pydantic_ai.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
_skills_capability,
build_agent,
get_agent_capability_tools,
get_skills_metadata,
)


Expand Down Expand Up @@ -183,6 +184,25 @@ def test_agent_excludes_tool_capabilities_when_no_tools(
assert SkillsCapability not in capability_types


class TestGetSkillsMetadata:
"""Tests for get_skills_metadata."""

def test_returns_empty_list_when_skills_not_configured(self) -> None:
"""Test that missing skills configuration yields no metadata."""
assert get_skills_metadata(None) == []
assert get_skills_metadata(SkillsConfiguration(paths=[])) == []

def test_returns_metadata_when_configured(
self, mock_skills_configuration: SkillsConfiguration
) -> None:
"""Test that configured skills return name and description."""
metadata = get_skills_metadata(mock_skills_configuration)

assert len(metadata) == 1
assert metadata[0]["name"] == "test-skill"
assert metadata[0]["description"] == "Test skill."


class TestGetAgentCapabilityTools:
"""Tests for get_agent_capability_tools."""

Expand Down
Loading