LCORE-2072: Add SkillsConfiguration model to config file#1736
Conversation
WalkthroughAdds a SkillsConfiguration model (paths: list[Path]), integrates optional ChangesSkills Configuration Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| 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 "/var/skills" in config.paths | ||
| assert "./local-skills" in config.paths | ||
| assert "/opt/skills" in config.paths |
There was a problem hiding this comment.
@radofuchs what do you think about this test? From the code's perspective, absolute/relative doesn't matter. It's just "str" at the end of the day - but I added this coz it signals that we'll work with both absolute and relative path - ie this is my attempt at BDD :)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/models/config.py`:
- Around line 1932-1936: Add validation for the paths field by adding Pydantic
validators for the paths attribute: implement a `@field_validator`("paths",
mode="before", each_item=True) that strips each string and raises ValueError for
blank/whitespace entries, and implement a second `@field_validator`("paths",
mode="after") that deduplicates the list while preserving order (e.g., using an
ordered set pattern) and returns the normalized list; reference the existing
paths: list[str] Field declaration and ensure all validators are annotated and
use Pydantic v2 style `@field_validator` for the "paths" field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ec5373b-046b-4cad-bd80-096b14aafda0
📒 Files selected for processing (3)
examples/lightspeed-stack-skills.yamlsrc/models/config.pytests/unit/models/config/test_skills_configuration.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/models/config/test_skills_configuration.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/config.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/config.py
🔇 Additional comments (3)
src/models/config.py (1)
2099-2103: LGTM!tests/unit/models/config/test_skills_configuration.py (1)
1-48: LGTM!examples/lightspeed-stack-skills.yaml (1)
1-32: LGTM!
| paths: list[str] = Field( | ||
| default_factory=list, | ||
| title="Skill paths", | ||
| description="Paths to skill directories or directories containing skill subdirectories.", | ||
| ) |
There was a problem hiding this comment.
Add validation for individual skills.paths entries.
skills.paths currently accepts blank/whitespace and duplicate values, which can lead to ambiguous or invalid path handling later. Add a field validator to normalize and reject invalid entries.
Suggested patch
class SkillsConfiguration(ConfigurationBase):
@@
paths: list[str] = Field(
default_factory=list,
title="Skill paths",
description="Paths to skill directories or directories containing skill subdirectories.",
)
+
+ `@field_validator`("paths")
+ `@classmethod`
+ def validate_paths(cls, value: list[str]) -> list[str]:
+ """Normalize and validate configured skill paths."""
+ seen: set[str] = set()
+ normalized_paths: list[str] = []
+ for path in value:
+ normalized = path.strip()
+ if not normalized:
+ raise ValueError("Skill paths must not contain empty values")
+ if normalized in seen:
+ raise ValueError(f"Duplicate skill path: '{normalized}'")
+ seen.add(normalized)
+ normalized_paths.append(normalized)
+ return normalized_pathsAs per coding guidelines: src/models/**/*.py: “Pydantic models must use @model_validator and @field_validator for validation and complete type annotations for all attributes, avoiding Any type”.
🤖 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 1932 - 1936, Add validation for the paths
field by adding Pydantic validators for the paths attribute: implement a
`@field_validator`("paths", mode="before", each_item=True) that strips each string
and raises ValueError for blank/whitespace entries, and implement a second
`@field_validator`("paths", mode="after") that deduplicates the list while
preserving order (e.g., using an ordered set pattern) and returns the normalized
list; reference the existing paths: list[str] Field declaration and ensure all
validators are annotated and use Pydantic v2 style `@field_validator` for the
"paths" field.
| # Skills provide domain-specific instructions and reference materials | ||
| # that the LLM can load on demand when relevant to the current task | ||
| skills: | ||
| paths: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
anik120
force-pushed
the
skills-configuration-model
branch
2 times, most recently
from
7203706 to
d44ac12
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@docs/openapi.json`:
- Around line 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.
- Around line 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.
In `@src/models/config.py`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 070de4da-07aa-458e-8249-428f9dd04dea
📒 Files selected for processing (5)
docs/openapi.jsonexamples/lightspeed-stack-skills.yamlsrc/models/config.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_skills_configuration.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build-pr
- GitHub Check: integration_tests (3.12)
- GitHub Check: radon
- GitHub Check: integration_tests (3.13)
- GitHub Check: Pylinter
- GitHub Check: spectral
- GitHub Check: unit_tests (3.12)
- GitHub Check: unit_tests (3.13)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_skills_configuration.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/config.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/config.py
🔇 Additional comments (6)
src/models/config.py (2)
1960-1964:skills.pathsentry validation is still missing (already reported).This remains unresolved: blank/whitespace and duplicate path entries are still accepted.
As per coding guidelines:
src/models/**/*.py: “Pydantic models must use@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype”.
2133-2137: LGTM!docs/openapi.json (1)
12084-12096: LGTM!tests/unit/models/config/test_skills_configuration.py (1)
1-48: LGTM!examples/lightspeed-stack-skills.yaml (1)
1-32: LGTM!tests/unit/models/config/test_dump_configuration.py (1)
238-238: LGTM!Also applies to: 610-610, 858-858, 1081-1081, 1289-1289
| "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." | ||
| }, |
There was a problem hiding this comment.
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.
| "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.
| "paths": { | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "type": "array", | ||
| "title": "Skill paths", | ||
| "description": "Paths to skill directories or directories containing skill subdirectories." | ||
| } |
There was a problem hiding this comment.
🧹 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.
| "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.
| Paths are validated at startup to ensure they exist and contain valid SKILL.md files. | ||
| """ |
There was a problem hiding this comment.
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.
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
One or two nits (str->Path + directory checks), LGTM in overall,
| Paths are validated at startup to ensure they exist and contain valid SKILL.md files. | ||
| """ | ||
|
|
||
| paths: list[str] = Field( |
| @@ -235,6 +235,7 @@ def test_dump_configuration(tmp_path: Path) -> None: | |||
| "enabled": False, | |||
There was a problem hiding this comment.
you can enhance the config file used in dump tests, and then it will be possible to check for actual skills path processing
There was a problem hiding this comment.
Added a test_dump_configuration_with_skills, is that what you meant?
| title="Skill paths", | ||
| description="Paths to skill directories or directories containing skill subdirectories.", | ||
| ) | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
Adds the `SkillsConfiguration` Pydantic model to enable configuring skill directory paths in `lightspeed-stack.yaml`. This is the first step in implementing Agent Skills support. For more info, refer to `docs/design/agent-skills/agent-skills.md`. **Scope**: This PR adds only the configuration model. Runtime skill loading (`load_skills()`, frontmatter parsing, tool registration) will be implemented in follow-up commits. Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
anik120
force-pushed
the
skills-configuration-model
branch
from
d44ac12 to
0f6a586
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/models/config.py (2)
1960-1964: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd field validator for paths to comply with coding guidelines.
The
pathsfield lacks validation despite the coding guideline requirement: "Pydantic models must use@model_validatorand@field_validatorfor validation." At minimum, validate that paths are not empty/whitespace and contain no duplicates. Consider also checking path existence and directory status for early failure detection.🛡️ Proposed validator implementation
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. """ paths: list[Path] = Field( default_factory=list, title="Skill paths", description="Paths to skill directories or directories containing skill subdirectories.", ) + + `@field_validator`("paths") + `@classmethod` + def validate_paths(cls, value: list[Path]) -> list[Path]: + """Validate and normalize skill paths.""" + seen: set[Path] = set() + validated_paths: list[Path] = [] + for path in value: + # Path objects don't have empty strings, but check for current directory + if not str(path).strip() or str(path).strip() == '.': + raise ValueError("Skill paths must not be empty or current directory") + if path in seen: + raise ValueError(f"Duplicate skill path: '{path}'") + seen.add(path) + validated_paths.append(path) + return validated_pathsAs per coding guidelines:
src/models/**/*.py: "Pydantic models must use@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype".🤖 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 1960 - 1964, Add a Pydantic validator for the paths Field: implement a `@field_validator`("paths", mode="before") to normalize input (convert strings to pathlib.Path, strip whitespace, reject empty/whitespace-only entries) and a `@field_validator`("paths") to remove duplicates while preserving order; additionally add a `@model_validator`(mode="after") on the same model class to optionally assert each Path.exists() and Path.is_dir() (or raise ValueError with a clear message) to catch invalid paths early. Target the existing paths Field symbol and use the decorators `@field_validator` and `@model_validator` to perform these checks and transformations so the paths list is validated, de-duplicated, and normalized before use.
1957-1958:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring claims validation that is not implemented.
The docstring states "Paths are validated at startup to ensure they exist and contain valid SKILL.md files," but the model contains no validation logic. This is misleading. Please update the docstring to reflect that validation is planned for a future commit, or that paths are expected but not enforced.
📝 Suggested docstring revision
- Paths are validated at startup to ensure they exist and contain valid SKILL.md files. + Skill metadata will be loaded from SKILL.md files at startup. Path validation + and SKILL.md parsing will be implemented in a future 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, The docstring in src/models/config.py currently claims "Paths are validated at startup to ensure they exist and contain valid SKILL.md files" but no validation logic exists; update that docstring (the module/class docstring in config.py that contains that sentence) to accurately state that path validation is not yet implemented or is planned for a future commit (e.g., "Paths are expected but not validated at startup; validation will be added in a future change"), so the comment reflects current behavior rather than claiming enforcement.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@examples/lightspeed-stack-skills.yaml`:
- 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.
In `@tests/unit/models/config/test_dump_configuration.py`:
- Around line 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.
---
Duplicate comments:
In `@src/models/config.py`:
- Around line 1960-1964: Add a Pydantic validator for the paths Field: implement
a `@field_validator`("paths", mode="before") to normalize input (convert strings
to pathlib.Path, strip whitespace, reject empty/whitespace-only entries) and a
`@field_validator`("paths") to remove duplicates while preserving order;
additionally add a `@model_validator`(mode="after") on the same model class to
optionally assert each Path.exists() and Path.is_dir() (or raise ValueError with
a clear message) to catch invalid paths early. Target the existing paths Field
symbol and use the decorators `@field_validator` and `@model_validator` to perform
these checks and transformations so the paths list is validated, de-duplicated,
and normalized before use.
- Around line 1957-1958: The docstring in src/models/config.py currently claims
"Paths are validated at startup to ensure they exist and contain valid SKILL.md
files" but no validation logic exists; update that docstring (the module/class
docstring in config.py that contains that sentence) to accurately state that
path validation is not yet implemented or is planned for a future commit (e.g.,
"Paths are expected but not validated at startup; validation will be added in a
future change"), so the comment reflects current behavior rather than claiming
enforcement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc27966b-8298-47c8-b6b7-a4c09e1ef07e
📒 Files selected for processing (5)
docs/openapi.jsonexamples/lightspeed-stack-skills.yamlsrc/models/config.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_skills_configuration.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: spectral
- GitHub Check: ruff
- GitHub Check: integration_tests (3.13)
- GitHub Check: pydocstyle
- GitHub Check: unit_tests (3.12)
- GitHub Check: Pylinter
- GitHub Check: unit_tests (3.13)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Usepytest.mark.asynciomarker for async tests
Files:
tests/unit/models/config/test_skills_configuration.pytests/unit/models/config/test_dump_configuration.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models must use
@model_validatorand@field_validatorfor validation and complete type annotations for all attributes, avoidingAnytype
Files:
src/models/config.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/config.py
🔇 Additional comments (6)
src/models/config.py (1)
2133-2137: LGTM!docs/openapi.json (2)
19396-19412: Makepathsmandatory inSkillsConfiguration.
SkillsConfigurationstill omits arequiredlist, so an empty object is accepted. Add"required": ["paths"]to keep the schema contract strict.
12085-12096: LGTM!tests/unit/models/config/test_dump_configuration.py (1)
239-239: LGTM!Also applies to: 611-611, 859-859, 1082-1082, 1290-1290
tests/unit/models/config/test_skills_configuration.py (1)
1-50: LGTM!examples/lightspeed-stack-skills.yaml (1)
1-31: The example configuration is complete and valid. Theinferencefield is optional in theConfigurationmodel and has adefault_factorythat automatically provides default values if omitted. Many other example files in the repository (e.g.,lightspeed-stack-byok-okp-rag.yaml,lightspeed-stack-a2a-state-pg.yaml) successfully omit this field, confirming that it is not required. While test cases explicitly set theinferencefield, this is standard practice for comprehensive test coverage and does not indicate the field is mandatory.> Likely an incorrect or invalid review comment.
| paths: | ||
| # Option A: Directory containing multiple skill subdirectories | ||
| # Each subdirectory must contain a SKILL.md file | ||
| - "/var/skills/" |
There was a problem hiding this comment.
🧹 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.
| - "/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.
| 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", | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧹 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 contentThen 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.
3 participants
Description
Adds the
SkillsConfigurationPydantic model to enable configuring skill directory paths inlightspeed-stack.yaml. This is the first step in implementing Agent Skills support.For more info, refer to
docs/design/agent-skills/agent-skills.md.Scope: This PR adds only the configuration model. Runtime skill loading (
load_skills(), frontmatter parsing, tool registration) will be implemented in follow-up commits.Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Tests