Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
29 changes: 29 additions & 0 deletions docs/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -12081,6 +12081,18 @@
"$ref": "#/components/schemas/RerankerConfiguration",
"title": "Reranker configuration",
"description": "Configuration for neural reranking of RAG chunks using cross-encoder."
},
"skills": {
"anyOf": [
{
"$ref": "#/components/schemas/SkillsConfiguration"
},
{
"type": "null"
}
],
"title": "Agent skills",
"description": "Agent skills configuration. Specifies paths to skill directories."
}
},
"additionalProperties": false,
Expand Down Expand Up @@ -19381,6 +19393,23 @@
}
]
},
"SkillsConfiguration": {
"properties": {
"paths": {
"items": {
"type": "string",
"format": "path"
},
"type": "array",
"title": "Skill paths",
"description": "Paths to skill directories or directories containing skill subdirectories."
}
Comment on lines +19398 to +19406
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.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Consider adding minItems: 1 constraint to the paths array.

An empty paths array would make the SkillsConfiguration object effectively useless. Adding a minItems: 1 constraint would enforce that at least one skill path is provided.

📐 Proposed refinement
                     "paths": {
                         "items": {
                             "type": "string"
                         },
                         "type": "array",
+                        "minItems": 1,
                         "title": "Skill paths",
                         "description": "Paths to skill directories or directories containing skill subdirectories."
                     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"paths": {
"items": {
"type": "string"
},
"type": "array",
"title": "Skill paths",
"description": "Paths to skill directories or directories containing skill subdirectories."
}
"paths": {
"items": {
"type": "string"
},
"type": "array",
"minItems": 1,
"title": "Skill paths",
"description": "Paths to skill directories or directories containing skill subdirectories."
}
🤖 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 `@docs/openapi.json` around lines 19398 - 19405, The OpenAPI schema for
SkillsConfiguration allows an empty paths array; add a minItems: 1 constraint to
the "paths" array schema (the "paths" property under SkillsConfiguration) so the
array must contain at least one entry, ensuring "paths" cannot be empty and the
configuration is meaningful.

},
"additionalProperties": false,
"type": "object",
"title": "SkillsConfiguration",
"description": "Agent skills configuration.\n\nSpecifies paths to skill directories. Skill metadata (name, description)\nis read from SKILL.md frontmatter at startup.\n\nEach path can point to either:\n- A directory containing a SKILL.md file (single skill)\n- A directory containing subdirectories with SKILL.md files (multiple skills)\n\nPaths are validated at startup to ensure they exist and contain valid SKILL.md files."
},
Comment on lines +19396 to +19412
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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add required field to enforce paths as mandatory.

The SkillsConfiguration schema does not specify a required array, making paths optional. Based on the Python model (paths: list[str] per the AI summary), paths should be a required field when SkillsConfiguration is provided.

✨ Proposed fix
             "SkillsConfiguration": {
                 "properties": {
                     "paths": {
                         "items": {
                             "type": "string"
                         },
                         "type": "array",
                         "title": "Skill paths",
                         "description": "Paths to skill directories or directories containing skill subdirectories."
                     }
                 },
+                "required": ["paths"],
                 "additionalProperties": false,
                 "type": "object",
                 "title": "SkillsConfiguration",
                 "description": "Agent skills configuration.\n\nSpecifies paths to skill directories. Skill metadata (name, description)\nis read from SKILL.md frontmatter at startup.\n\nEach path can point to either:\n- A directory containing a SKILL.md file (single skill)\n- A directory containing subdirectories with SKILL.md files (multiple skills)\n\nPaths are validated at startup to ensure they exist and contain valid SKILL.md files."
             },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"SkillsConfiguration": {
"properties": {
"paths": {
"items": {
"type": "string"
},
"type": "array",
"title": "Skill paths",
"description": "Paths to skill directories or directories containing skill subdirectories."
}
},
"additionalProperties": false,
"type": "object",
"title": "SkillsConfiguration",
"description": "Agent skills configuration.\n\nSpecifies paths to skill directories. Skill metadata (name, description)\nis read from SKILL.md frontmatter at startup.\n\nEach path can point to either:\n- A directory containing a SKILL.md file (single skill)\n- A directory containing subdirectories with SKILL.md files (multiple skills)\n\nPaths are validated at startup to ensure they exist and contain valid SKILL.md files."
},
"SkillsConfiguration": {
"properties": {
"paths": {
"items": {
"type": "string"
},
"type": "array",
"title": "Skill paths",
"description": "Paths to skill directories or directories containing skill subdirectories."
}
},
"required": ["paths"],
"additionalProperties": false,
"type": "object",
"title": "SkillsConfiguration",
"description": "Agent skills configuration.\n\nSpecifies paths to skill directories. Skill metadata (name, description)\nis read from SKILL.md frontmatter at startup.\n\nEach path can point to either:\n- A directory containing a SKILL.md file (single skill)\n- A directory containing subdirectories with SKILL.md files (multiple skills)\n\nPaths are validated at startup to ensure they exist and contain valid SKILL.md files."
},
🤖 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 `@docs/openapi.json` around lines 19396 - 19411, The SkillsConfiguration schema
currently allows omission of paths; update the JSON schema for
SkillsConfiguration to include a required array that lists "paths" so that the
paths property is mandatory when a SkillsConfiguration object is provided (keep
existing properties like "paths" (array of string), "additionalProperties":
false, and the descriptions about SKILL.md intact); modify the
SkillsConfiguration definition to add "required": ["paths"] to enforce the model
contract.

"SolrVectorSearchRequest": {
"properties": {
"mode": {
Expand Down
31 changes: 31 additions & 0 deletions examples/lightspeed-stack-skills.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
name: Lightspeed Core Service (LCS) with Skills
service:
host: localhost
port: 8080
auth_enabled: false
workers: 1
color_log: true
access_log: true
llama_stack:
use_as_library_client: true
library_client_config_path: run.yaml
user_data_collection:
feedback_enabled: true
feedback_storage: "/tmp/data/feedback"
transcripts_enabled: true
transcripts_storage: "/tmp/data/transcripts"
authentication:
module: "noop"
# Agent skills configuration
# Skills provide domain-specific instructions and reference materials
# that the LLM can load on demand when relevant to the current task
skills:
paths:
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.

is there a reason that we need the paths? It would make sense if we had other data fields under skills but if paths is the only one then I think skills can just be a list. wdyt?

Copy link
Copy Markdown
Contributor Author

@anik120 anik120 May 13, 2026

Choose a reason for hiding this comment

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

Yea I think that makes total sense. I was going off of what's here https://github.com/lightspeed-core/lightspeed-stack/blob/main/docs/design/agent-skills/agent-skills.md#configuration

but I do remember this discussion and AFAIR we'd decided we don't need path. I'll update the design doc too

Based on further discussions:

It's a little weird to look at now, but the current layout is the safest approach - I can't think of anything else that might be needed under the skills tab (eg, settings etc), but keeping it tabbe-ed future proofs it so keeping it as is

# Option A: Directory containing multiple skill subdirectories
# Each subdirectory must contain a SKILL.md file
- "/var/skills/"
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.

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Minor: trailing slash inconsistency with test examples.

Line 26 uses "/var/skills/" with a trailing slash, while the test examples in test_dump_configuration_with_skills use paths without trailing slashes ("/var/skills/openshift-troubleshooting"). While both should work with pathlib.Path, consistency across documentation and tests improves clarity.

Consider removing the trailing slash to match the test patterns:

-    - "/var/skills/"
+    - "/var/skills"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- "/var/skills/"
- "/var/skills"
🤖 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 `@examples/lightspeed-stack-skills.yaml` at line 26, Change the skills path
string from "/var/skills/" to "/var/skills" in the YAML so it matches the
pattern used in tests (see test_dump_configuration_with_skills); update the
value in the examples/lightspeed-stack-skills.yaml entry that currently contains
"/var/skills/" to remove the trailing slash for consistency with test examples.


# Option B: Individual skill paths for fine-grained control
# - "/var/skills/openshift-troubleshooting/"
# - "/var/skills/code-review/"
# - "/opt/custom-skills/deployment-guide/"
26 changes: 26 additions & 0 deletions src/models/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -1944,6 +1944,26 @@ class AzureEntraIdConfiguration(ConfigurationBase):
)


class SkillsConfiguration(ConfigurationBase):
"""Agent skills configuration.

Specifies paths to skill directories. Skill metadata (name, description)
is read from SKILL.md frontmatter at startup.

Each path can point to either:
- A directory containing a SKILL.md file (single skill)
- A directory containing subdirectories with SKILL.md files (multiple skills)

Paths are validated at startup to ensure they exist and contain valid SKILL.md files.
"""
Comment on lines +1957 to +1958
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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Docstring states validation that is not implemented yet.

The text says paths are validated at startup, but this model currently has no validation logic. Please reword this to avoid implying enforced behavior in this commit.

🤖 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 `@src/models/config.py` around lines 1957 - 1958, Update the docstring text
that currently reads "Paths are validated at startup to ensure they exist and
contain valid SKILL.md files." so it no longer implies validation occurs; edit
the module/class docstring in src.models.config (the string containing "Paths
are validated at startup") to state that paths are expected to contain SKILL.md
files or that validation is planned/not implemented yet, without claiming
enforcement at startup.


paths: list[Path] = Field(
default_factory=list,
title="Skill paths",
description="Paths to skill directories or directories containing skill subdirectories.",
)

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.

prob. check if the directory is accessible. It might be too early to do it during config loading, but probably better from UX perspective (fail early...). WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm leaning towards keeping filesystem validation out of the Pedantic config layer - eg in containerized environments volumes may not be mounted yet when the config is parsed.

It also makes unit testing harder since we have to create actual directories.

Fwiw, load_skills() will need to have this validation function, and that looks like the earliest we can validate safely?


class Configuration(ConfigurationBase):
"""Global service configuration."""

Expand Down Expand Up @@ -2110,6 +2130,12 @@ class Configuration(ConfigurationBase):
description="Configuration for neural reranking of RAG chunks using cross-encoder.",
)

skills: Optional[SkillsConfiguration] = Field(
default=None,
title="Agent skills",
description="Agent skills configuration. Specifies paths to skill directories.",
)

@model_validator(mode="after")
def validate_mcp_auth_headers(self) -> Self:
"""
Expand Down
85 changes: 85 additions & 0 deletions tests/unit/models/config/test_dump_configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
QuotaLimiterConfiguration,
QuotaSchedulerConfiguration,
ServiceConfiguration,
SkillsConfiguration,
TLSConfiguration,
UserDataCollection,
)
Expand Down Expand Up @@ -235,6 +236,7 @@ def test_dump_configuration(tmp_path: Path) -> None:
"enabled": False,
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.

you can enhance the config file used in dump tests, and then it will be possible to check for actual skills path processing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a test_dump_configuration_with_skills, is that what you meant?

"model": "cross-encoder/ms-marco-MiniLM-L6-v2",
},
"skills": None,
}


Expand Down Expand Up @@ -606,6 +608,7 @@ def test_dump_configuration_with_quota_limiters(tmp_path: Path) -> None:
"enabled": False,
"model": "cross-encoder/ms-marco-MiniLM-L6-v2",
},
"skills": None,
}


Expand Down Expand Up @@ -853,6 +856,7 @@ def test_dump_configuration_with_quota_limiters_different_values(
"enabled": False,
"model": "cross-encoder/ms-marco-MiniLM-L6-v2",
},
"skills": None,
}


Expand Down Expand Up @@ -1075,6 +1079,7 @@ def test_dump_configuration_byok(tmp_path: Path) -> None:
"enabled": False,
"model": "cross-encoder/ms-marco-MiniLM-L6-v2",
},
"skills": None,
}


Expand Down Expand Up @@ -1282,4 +1287,84 @@ def test_dump_configuration_pg_namespace(tmp_path: Path) -> None:
"enabled": False,
"model": "cross-encoder/ms-marco-MiniLM-L6-v2",
},
"skills": None,
}


def test_dump_configuration_with_skills(tmp_path: Path) -> None:
"""
Test that Configuration with skills paths can be serialized to JSON.

Verifies that skills paths are properly dumped and serialized as strings.
"""
cfg = Configuration(
name="test_name",
service=ServiceConfiguration(
tls_config=TLSConfiguration(
tls_certificate_path=Path("tests/configuration/server.crt"),
tls_key_path=Path("tests/configuration/server.key"),
tls_key_password=Path("tests/configuration/password"),
),
cors=CORSConfiguration(
allow_origins=["foo_origin", "bar_origin", "baz_origin"],
allow_credentials=False,
allow_methods=["foo_method", "bar_method", "baz_method"],
allow_headers=["foo_header", "bar_header", "baz_header"],
),
),
llama_stack=LlamaStackConfiguration(
use_as_library_client=True,
library_client_config_path="tests/configuration/run.yaml",
api_key=SecretStr("whatever"),
),
user_data_collection=UserDataCollection(
feedback_enabled=False, feedback_storage=None
),
database=DatabaseConfiguration(
sqlite=None,
postgres=PostgreSQLDatabaseConfiguration(
db="lightspeed_stack",
user="ls_user",
password=SecretStr("ls_password"),
port=5432,
ca_cert_path=None,
ssl_mode="require",
gss_encmode="disable",
),
),
mcp_servers=[],
customization=None,
inference=InferenceConfiguration(
default_provider="default_provider",
default_model="default_model",
),
skills=SkillsConfiguration(
paths=[
"/var/skills/openshift-troubleshooting",
"/var/skills/code-review",
"/opt/custom-skills",
]
),
)
assert cfg is not None
dump_file = tmp_path / "test.json"
cfg.dump(dump_file)

with open(dump_file, "r", encoding="utf-8") as fin:
content = json.load(fin)
# content should be loaded
assert content is not None

# skills section must exist
assert "skills" in content
assert content["skills"] is not None
assert "paths" in content["skills"]

# verify skills paths are properly serialized
assert content["skills"] == {
"paths": [
"/var/skills/openshift-troubleshooting",
"/var/skills/code-review",
"/opt/custom-skills",
]
}
Comment on lines +1294 to 1370
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.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Inconsistent test pattern: partial assertion instead of full configuration.

All other dump tests in this file (test_dump_configuration, test_dump_configuration_with_quota_limiters, test_dump_configuration_byok, test_dump_configuration_pg_namespace) follow a consistent pattern: they assert the complete deserialized JSON structure, not just the section under test. This test only verifies the skills section (lines 1358-1370), which is less thorough and makes the test inconsistent with the established file pattern.

Consider adding a complete JSON assertion like the other tests, verifying all expected sections exist and match their expected values.

📋 Suggested structure to match existing test pattern

After line 1357, before the skills-specific assertions, add:

         assert content is not None
 
+        # all sections must exist
+        assert "name" in content
+        assert "service" in content
+        assert "llama_stack" in content
+        assert "user_data_collection" in content
+        assert "mcp_servers" in content
+        assert "authentication" in content
+        assert "authorization" in content
+        assert "customization" in content
+        assert "inference" in content
+        assert "database" in content
+        assert "byok_rag" in content
+        assert "quota_handlers" in content
+        assert "azure_entra_id" in content
+        assert "reranker" in content
+
         # skills section must exist
         assert "skills" in content

Then replace the partial assertion (lines 1364-1370) with a complete JSON comparison following the pattern from test_dump_configuration.

🤖 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/models/config/test_dump_configuration.py` around lines 1294 -
1370, The test test_dump_configuration_with_skills currently only asserts the
skills block; update it to assert the full deserialized JSON matches the
expected configuration like the other tests: build an expected dict representing
the entire Configuration (include keys/service with tls and cors, llama_stack,
user_data_collection, database/postgres, mcp_servers, customization, inference,
and skills.paths) and replace the partial skills-only assertions with a single
equality assertion (e.g., assert content == expected). Locate the test function
test_dump_configuration_with_skills and the Configuration/SkillsConfiguration
objects to construct the expected JSON structure consistent with how cfg.dump()
serializes those classes.

49 changes: 49 additions & 0 deletions tests/unit/models/config/test_skills_configuration.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
"""Unit tests for SkillsConfiguration model."""

# pylint: disable=no-member
# Pydantic Field(default_factory=...) pattern confuses pylint's static analysis

from pathlib import Path

import pytest
from pydantic import ValidationError

from models.config import SkillsConfiguration


class TestSkillsConfiguration:
"""Tests for SkillsConfiguration model."""

def test_empty_paths_list(self) -> None:
"""Test that an explicit empty paths list is allowed."""
config = SkillsConfiguration(paths=[])
assert config.paths == []

def test_no_unknown_fields_allowed(self) -> None:
"""Test that SkillsConfiguration rejects unknown fields."""
with pytest.raises(ValidationError, match="Extra inputs are not permitted"):
SkillsConfiguration(unknown_field="value") # type: ignore[call-arg]

def test_skill_paths(self) -> None:
"""Test configuration with multiple skill paths."""
config = SkillsConfiguration(
paths=[
"/var/skills/openshift-troubleshooting",
"/var/skills/code-review",
"/opt/custom-skills",
]
)
assert len(config.paths) == 3
assert Path("/var/skills/openshift-troubleshooting") in config.paths
assert Path("/var/skills/code-review") in config.paths
assert Path("/opt/custom-skills") in config.paths

def test_mixed_absolute_and_relative_paths(self) -> None:
"""Test that both absolute and relative paths can be mixed."""
config = SkillsConfiguration(
paths=["/var/skills", "./local-skills", "/opt/skills"]
)
assert len(config.paths) == 3
assert Path("/var/skills") in config.paths
assert Path("./local-skills") in config.paths
assert Path("/opt/skills") in config.paths
Loading