LCORE-2080: Added E2E Steps for Agent Skills#1941
Conversation
WalkthroughAdds skills e2e fixtures, compose mounts, and Lightspeed stack configs for server and library modes. Updates streaming response helpers to capture tool calls and results, and expands the skills feature scenarios to use the new load/read skill flows. ChangesSkills e2e wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
jrobertboos
force-pushed
the
lcore-2080
branch
3 times, most recently
from
c201e27 to
fe7754f
Compare
|
|
||
| @SkillsConfig | ||
| @SkillsConfig @skip | ||
| Scenario: Skill tools are registered when skills are configured |
There was a problem hiding this comment.
TODO: Need to reflect skill tools (list_skills, load_skill, read_skill_resource) in /tools.
| """ | ||
| And The token metrics have increased | ||
|
|
||
| # --- Error handling: unknown skill --- |
There was a problem hiding this comment.
The "Error Paths" will have to be skipped for now as the skill tools do fail and produce a result, but it's a different type that the response-building code silently discards.
Below I have helpful part of conversation with Claude about the issue.
Pydantic-ai catches ModelRetry and wraps the error in a RetryPromptPart (not a ToolReturnPart). The FunctionToolResultEvent.part is typed as ToolReturnPart | RetryPromptPart — it can be either.
Where LCS drops it:
In the non-streaming path, build_turn_summary_from_agent_run only processes ToolReturnPart:
query.py
Lines 266-269
elif isinstance(message, ModelRequest):
for request_part in message.parts:
if isinstance(request_part, ToolReturnPart):
process_function_tool_result(state, request_part)
In the streaming path, the same filter exists:
streaming.py
Lines 522-524
part = event.part
if not isinstance(part, ToolReturnPart):
return None
Both paths explicitly ignore RetryPromptPart, so the retry/error message for load_skill is never surfaced as a tool_result in the API response.
The result:
- Both tool calls appear (because both ToolCallPart instances from the ModelResponse are processed)
- Only the list_skills result appears (because it succeeded and produced a ToolReturnPart)
- The load_skill result is missing (because it raised ModelRetry → became a RetryPromptPart → silently dropped)
| ] | ||
| """ | ||
|
|
||
| # --- Full progressive disclosure flow --- |
There was a problem hiding this comment.
This will likely be quite flaky as the LLM (through appendage of system prompt, I think) is given only the "names" of skills so sometimes will result in just load_skill and read_skill_resource being used completely skipping list_skills.
jrobertboos
force-pushed
the
lcore-2080
branch
3 times, most recently
from
bd2b990 to
159e8ae
Compare
|
Please Review: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yaml`:
- Around line 24-26: The skills discovery config is using a relative path in the
`skills.paths` entry, which can break startup when the working directory
changes. Update the YAML to use the absolute mounted path expected by the stack,
and keep the change localized to the `skills` block in
`lightspeed-stack-skills-directory.yaml` so startup consistently finds the
skills directory.
In `@tests/e2e/configuration/server-mode/lightspeed-stack-skills.yaml`:
- Around line 24-26: The skills path in the stack config is CWD-sensitive and
should be pinned to the mounted absolute location instead. Update the
`skills.paths` entry in the YAML so it points to `/app-root/skills/echo` rather
than the relative `skills/echo`, keeping the `skills` configuration
deterministic under the compose mount.
In `@tests/e2e/features/steps/common_http.py`:
- Around line 334-335: The expected JSON in the step implementation still parses
context.text directly, so placeholder tokens like {MODEL} are not substituted
before validation. Update the relevant step in common_http.py to apply the same
placeholder resolution used by the existing partial-body handling before calling
json.loads and validate_json_partially. Keep the fix localized to the step that
consumes context.text and ensure the parsed expected_value reflects substituted
placeholders first.
🪄 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: 2beba6a7-1aff-4350-92f7-60524e66a1c4
📒 Files selected for processing (14)
docker-compose-library.yamldocker-compose.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills-directory.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills.yamltests/e2e/features/skills.featuretests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.pytests/e2e/skills/echo/SKILL.mdtests/e2e/skills/echo/references/guide.mdtests/e2e/skills/summarize/SKILL.mdtests/e2e/skills/summarize/references/guide.mdtests/e2e/test_list.txt
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
🧰 Additional context used
📓 Path-based instructions (2)
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/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
tests/e2e/**/*.{py,feature}
📄 CodeRabbit inference engine (AGENTS.md)
Use behave (BDD) framework for end-to-end testing with Gherkin feature files
Files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.pytests/e2e/features/skills.feature
🧠 Learnings (4)
📚 Learning: 2026-05-20T08:09:30.641Z
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: docs/design/llama-stack-config-merge/poc-results/library-mode/synthesized-run.yaml:107-110
Timestamp: 2026-05-20T08:09:30.641Z
Learning: In Llama-stack config YAMLs, when defining a Llama Guard safety shield entry, set `provider_shield_id` to the *guard model identifier* (e.g., `meta-llama/Llama-Guard-3-8B`). Do not use a chat/generative model id (e.g., `openai/gpt-4o-mini`): a chat-model id (or `native_override`) indicates only an override landed and does **not** mean the safety shield is actually gating queries. Ensure any E2E coverage for the related implementation (JIRA/E2E tests) exercises a real Llama Guard model to verify that the shield is effective.
Applied to files:
docker-compose-library.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills.yamldocker-compose.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills-directory.yaml
📚 Learning: 2026-04-07T09:20:26.590Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1467
File: tests/e2e/features/steps/common.py:36-49
Timestamp: 2026-04-07T09:20:26.590Z
Learning: For Behave-based Python tests, rely on Behave’s Context layered stack for attribute lifecycle: Behave pushes a new Context layer when entering feature scope (before_feature) and again for scenario scope (before_scenario). Attributes assigned inside given/when/then steps live on the current scenario layer and are automatically removed when the scenario ends. As a result, step-set attributes should not be expected to persist across scenarios or features, and manual cleanup in after_scenario/after_feature is generally unnecessary for attributes set in step functions. Only perform manual cleanup for attributes that you set explicitly in before_feature/before_scenario, since those live on the respective feature/scenario layers.
Applied to files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
📚 Learning: 2026-04-13T13:39:54.963Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1490
File: tests/e2e/features/environment.py:206-211
Timestamp: 2026-04-13T13:39:54.963Z
Learning: In lightspeed-stack E2E tests under tests/e2e/features, it is intentional to set context.feature_config inside Background/step functions (scenario-scoped Behave layer). The environment.py after_scenario restore logic should only restore configuration when context.scenario_lightspeed_override_active is True; this flag is set by configure_service only when a real config switch occurs (so restore does not run for scenarios without a switch). Additionally, steps/common.py’s module-level _active_lightspeed_stack_config_basename is used to prevent re-applying the same config across subsequent scenarios, ensuring scenario_lightspeed_override_active stays False after the first apply. Therefore, reviewers should not “fix” this flow as if feature_config were incorrectly scoped or if after_scenario restoration is missing—config switching and restoration are meant to happen exactly once per actual switch, not redundantly per scenario.
Applied to files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
🪛 LanguageTool
tests/e2e/skills/echo/SKILL.md
[style] ~17-~17: Using “back” with the verb “return” may be redundant.
Context: ...r's input text 2. Return the exact text back to the user without modification For f...
(RETURN_BACK)
🔇 Additional comments (8)
docker-compose-library.yaml (1)
23-23: LGTM!docker-compose.yaml (1)
90-90: LGTM!tests/e2e/skills/echo/SKILL.md (1)
1-19: LGTM!tests/e2e/skills/echo/references/guide.md (1)
1-20: LGTM!tests/e2e/skills/summarize/SKILL.md (1)
1-22: LGTM!tests/e2e/skills/summarize/references/guide.md (1)
1-21: LGTM!tests/e2e/configuration/library-mode/lightspeed-stack-skills-directory.yaml (1)
1-26: LGTM!tests/e2e/configuration/library-mode/lightspeed-stack-skills.yaml (1)
1-26: LGTM!
| skills: | ||
| paths: | ||
| - skills |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Use absolute skills path to avoid CWD-dependent startup failures.
skills is relative; if the service working directory changes, skills discovery can fail at startup. Use /app-root/skills to match the compose mount explicitly.
Proposed change
skills:
paths:
- - skills
+ - /app-root/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.
| skills: | |
| paths: | |
| - skills | |
| skills: | |
| paths: | |
| - /app-root/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 `@tests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yaml`
around lines 24 - 26, The skills discovery config is using a relative path in
the `skills.paths` entry, which can break startup when the working directory
changes. Update the YAML to use the absolute mounted path expected by the stack,
and keep the change localized to the `skills` block in
`lightspeed-stack-skills-directory.yaml` so startup consistently finds the
skills directory.
| skills: | ||
| paths: | ||
| - skills/echo |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Pin the skill path to the mounted absolute location.
skills/echo is CWD-sensitive. Prefer /app-root/skills/echo for deterministic resolution against the compose mount.
Proposed change
skills:
paths:
- - skills/echo
+ - /app-root/skills/echo📝 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.
| skills: | |
| paths: | |
| - skills/echo | |
| skills: | |
| paths: | |
| - /app-root/skills/echo |
🤖 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/e2e/configuration/server-mode/lightspeed-stack-skills.yaml` around
lines 24 - 26, The skills path in the stack config is CWD-sensitive and should
be pinned to the mounted absolute location instead. Update the `skills.paths`
entry in the YAML so it points to `/app-root/skills/echo` rather than the
relative `skills/echo`, keeping the `skills` configuration deterministic under
the compose mount.
| expected_value = json.loads(context.text) | ||
| validate_json_partially(actual_value, expected_value) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply placeholder substitution before parsing expected JSON.
At Line 334, this step parses context.text directly, so placeholders like {MODEL} won’t be resolved here (unlike the existing partial-body step). That can cause false failures in scenario assertions.
Proposed fix
- expected_value = json.loads(context.text)
+ json_str = replace_placeholders(context, context.text)
+ expected_value = json.loads(json_str)📝 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.
| expected_value = json.loads(context.text) | |
| validate_json_partially(actual_value, expected_value) | |
| json_str = replace_placeholders(context, context.text) | |
| expected_value = json.loads(json_str) | |
| validate_json_partially(actual_value, expected_value) |
🤖 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/e2e/features/steps/common_http.py` around lines 334 - 335, The expected
JSON in the step implementation still parses context.text directly, so
placeholder tokens like {MODEL} are not substituted before validation. Update
the relevant step in common_http.py to apply the same placeholder resolution
used by the existing partial-body handling before calling json.loads and
validate_json_partially. Keep the fix localized to the step that consumes
context.text and ensure the parsed expected_value reflects substituted
placeholders first.
anik120
left a comment
anik120
left a comment
There was a problem hiding this comment.
ps: squashing commits to have a single commit for a PR (unless having multiple commits is by design, in which case too, the question would be "why aren't they multiple PRs instead"), is the hygienic thing to do.
Otherwise they show up as
"fix"
"fix"
"address code rabbit"
when someone is searching through git history trying to figure out what changes were made.
Here's an article I highly recommend reading https://medium.com/@madhav2002/git-hygiene-commits-branching-and-rewriting-history-bc6dee5f953f
radofuchs
left a comment
radofuchs
left a comment
There was a problem hiding this comment.
LGTM in overall, just a few details
|
Just a conceptual question: Is the skill invocation really so strict that when you prompt to run a non-existing skill, the LLM really tries to execute it and ends up with failure? |
|
@asimurka when u prompt the LLM to use a skill, if u are direct enough, it will try to use the INPUT OUTPUT Does that answer your question? |
|
Is it possible that this is just model-specific behavior? Because I think you shouldn't be able to influence the model behavior like this (with bare prompt). |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/features/skills.feature (1)
59-92: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the advertised tool parameters with the asserted call args.
Line 63 exposes
load_skillwith parametername, and Line 87 exposesread_skill_resourcewith parameterpath, but the later call assertions in this same feature expectskill_nameandresource_name. Both contracts cannot be correct at once, so either/toolsis asserting stale metadata or thetool_callschecks will never match the real invocation shape.🤖 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/e2e/features/skills.feature` around lines 59 - 92, The tool metadata in skills.feature is inconsistent with the expected call arguments: load_skill currently advertises name while the assertions use skill_name, and read_skill_resource advertises path while the assertions use resource_name. Update the feature so the parameter names in the tool definitions and the tool_calls checks match exactly, using the same symbols load_skill and read_skill_resource throughout.
♻️ Duplicate comments (2)
tests/e2e/configuration/server-mode/lightspeed-stack-skills.yaml (1)
24-26: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPin skill path to absolute mounted location.
skills/echois CWD-sensitive. Use/app-root/skills/echofor deterministic resolution against the compose mount. This was flagged in a previous review and remains unaddressed.Proposed fix
skills: paths: - - skills/echo + - /app-root/skills/echo🤖 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/e2e/configuration/server-mode/lightspeed-stack-skills.yaml` around lines 24 - 26, The skills path in the configuration is still relative and depends on the working directory, so it should be pinned to the mounted absolute location instead. Update the `skills.paths` entry in the `lightspeed-stack-skills.yaml` config to use the compose mount target `/app-root/skills/echo` so resolution is deterministic. Make sure the change is applied in the `skills` section and not elsewhere in the e2e configuration.tests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yaml (1)
24-26: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUse absolute path for skills directory to prevent startup failures.
skillsis a relative path. If the service working directory differs from/app-root, skill discovery fails at startup. Change to/app-root/skillsto match the compose mount explicitly. This was flagged in a previous review and remains unaddressed.Proposed fix
skills: paths: - - skills + - /app-root/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 `@tests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yaml` around lines 24 - 26, The skills directory configuration is using a relative path, which can break startup when the working directory is not the expected root. Update the skills path in the lightspeed stack config to use the absolute mounted location instead of the current relative value, and make sure the change is applied in the skills discovery config that the startup flow reads so skill loading works reliably regardless of cwd.
🤖 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 `@docker-compose-library.yaml`:
- Line 23: Align the SELinux label used for the `./tests/e2e/skills` bind mount
in `docker-compose-library.yaml` with the one used in `docker-compose.yaml` to
avoid unnecessary divergence. Update the volume entry under the library compose
service to use the same `ro,z`/`ro,Z` convention consistently across both
compose files, or explicitly document why `docker-compose-library.yaml` should
differ. Use the shared skills mount definition as the reference point when
making the change.
In `@tests/e2e/features/skills.feature`:
- Around line 14-16: The skills feature scenarios are applying the new MCP
skills config before resetting toolgroups, which can leave stale server-mode
registrations in place. Update each affected scenario in skills.feature so
reset_mcp_toolgroups_for_new_configuration runs before The service uses the
lightspeed-stack-skills.yaml configuration, then restart the service afterward.
Keep the step order consistent in all listed scenarios so list_skills and
load_skill assertions always use the fresh toolgroup state.
In `@tests/e2e/skills/echo/SKILL.md`:
- Line 17: The SKILL.md guidance in the echo skill uses the redundant phrase
“return back”; update the wording in the instruction text to say “Return the
exact text to the user without modification” so it stays clear and concise. Make
this edit in the echo skill’s step that describes the response behavior, keeping
the rest of the instruction unchanged.
---
Outside diff comments:
In `@tests/e2e/features/skills.feature`:
- Around line 59-92: The tool metadata in skills.feature is inconsistent with
the expected call arguments: load_skill currently advertises name while the
assertions use skill_name, and read_skill_resource advertises path while the
assertions use resource_name. Update the feature so the parameter names in the
tool definitions and the tool_calls checks match exactly, using the same symbols
load_skill and read_skill_resource throughout.
---
Duplicate comments:
In `@tests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yaml`:
- Around line 24-26: The skills directory configuration is using a relative
path, which can break startup when the working directory is not the expected
root. Update the skills path in the lightspeed stack config to use the absolute
mounted location instead of the current relative value, and make sure the change
is applied in the skills discovery config that the startup flow reads so skill
loading works reliably regardless of cwd.
In `@tests/e2e/configuration/server-mode/lightspeed-stack-skills.yaml`:
- Around line 24-26: The skills path in the configuration is still relative and
depends on the working directory, so it should be pinned to the mounted absolute
location instead. Update the `skills.paths` entry in the
`lightspeed-stack-skills.yaml` config to use the compose mount target
`/app-root/skills/echo` so resolution is deterministic. Make sure the change is
applied in the `skills` section and not elsewhere in the e2e configuration.
🪄 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: 07dc80e8-7a8f-4ea7-a655-f00e2ffeee6d
📒 Files selected for processing (14)
docker-compose-library.yamldocker-compose.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills-directory.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills.yamltests/e2e/features/skills.featuretests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.pytests/e2e/skills/echo/SKILL.mdtests/e2e/skills/echo/references/guide.mdtests/e2e/skills/summarize/SKILL.mdtests/e2e/skills/summarize/references/guide.mdtests/e2e/test_list.txt
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: integration_tests (3.13)
- GitHub Check: integration_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E Tests for Lightspeed Evaluation job
⚠️ CI failures not shown inline (2)
GitHub Actions: OpenAPI (Spectral) / spectral: LCORE-2080: Added E2E Steps for Agent Skills
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1muv run python scripts/generate_openapi_schema.py /tmp/openapi-generated.json�[0m
�[36;1mif ! diff -u docs/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json"�[0m
GitHub Actions: OpenAPI (Spectral) / 0_spectral.txt: LCORE-2080: Added E2E Steps for Agent Skills
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1muv run python scripts/generate_openapi_schema.py /tmp/openapi-generated.json�[0m
�[36;1mif ! diff -u docs/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json"�[0m
🧰 Additional context used
📓 Path-based instructions (2)
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/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
tests/e2e/**/*.{py,feature}
📄 CodeRabbit inference engine (AGENTS.md)
Use behave (BDD) framework for end-to-end testing with Gherkin feature files
Files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.pytests/e2e/features/skills.feature
🧠 Learnings (4)
📚 Learning: 2026-05-20T08:09:30.641Z
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: docs/design/llama-stack-config-merge/poc-results/library-mode/synthesized-run.yaml:107-110
Timestamp: 2026-05-20T08:09:30.641Z
Learning: In Llama-stack config YAMLs, when defining a Llama Guard safety shield entry, set `provider_shield_id` to the *guard model identifier* (e.g., `meta-llama/Llama-Guard-3-8B`). Do not use a chat/generative model id (e.g., `openai/gpt-4o-mini`): a chat-model id (or `native_override`) indicates only an override landed and does **not** mean the safety shield is actually gating queries. Ensure any E2E coverage for the related implementation (JIRA/E2E tests) exercises a real Llama Guard model to verify that the shield is effective.
Applied to files:
docker-compose-library.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills.yamldocker-compose.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills-directory.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills.yaml
📚 Learning: 2026-04-07T09:20:26.590Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1467
File: tests/e2e/features/steps/common.py:36-49
Timestamp: 2026-04-07T09:20:26.590Z
Learning: For Behave-based Python tests, rely on Behave’s Context layered stack for attribute lifecycle: Behave pushes a new Context layer when entering feature scope (before_feature) and again for scenario scope (before_scenario). Attributes assigned inside given/when/then steps live on the current scenario layer and are automatically removed when the scenario ends. As a result, step-set attributes should not be expected to persist across scenarios or features, and manual cleanup in after_scenario/after_feature is generally unnecessary for attributes set in step functions. Only perform manual cleanup for attributes that you set explicitly in before_feature/before_scenario, since those live on the respective feature/scenario layers.
Applied to files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
📚 Learning: 2026-04-13T13:39:54.963Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1490
File: tests/e2e/features/environment.py:206-211
Timestamp: 2026-04-13T13:39:54.963Z
Learning: In lightspeed-stack E2E tests under tests/e2e/features, it is intentional to set context.feature_config inside Background/step functions (scenario-scoped Behave layer). The environment.py after_scenario restore logic should only restore configuration when context.scenario_lightspeed_override_active is True; this flag is set by configure_service only when a real config switch occurs (so restore does not run for scenarios without a switch). Additionally, steps/common.py’s module-level _active_lightspeed_stack_config_basename is used to prevent re-applying the same config across subsequent scenarios, ensuring scenario_lightspeed_override_active stays False after the first apply. Therefore, reviewers should not “fix” this flow as if feature_config were incorrectly scoped or if after_scenario restoration is missing—config switching and restoration are meant to happen exactly once per actual switch, not redundantly per scenario.
Applied to files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
🪛 LanguageTool
tests/e2e/skills/echo/SKILL.md
[style] ~17-~17: Using “back” with the verb “return” may be redundant.
Context: ...r's input text 2. Return the exact text back to the user without modification For f...
(RETURN_BACK)
🔇 Additional comments (8)
tests/e2e/features/steps/common_http.py (1)
331-333: Apply placeholder substitution before parsing the expected JSON.This step still calls
json.loads(context.text)directly, so{MODEL}-style placeholders here will fail even though the sibling partial-body step resolves them first.tests/e2e/features/steps/llm_query_response.py (1)
94-97: LGTM!Also applies to: 368-404
docker-compose.yaml (1)
90-90: LGTM!tests/e2e/skills/echo/references/guide.md (1)
1-20: LGTM!tests/e2e/skills/summarize/SKILL.md (1)
1-21: LGTM!tests/e2e/skills/summarize/references/guide.md (1)
1-21: LGTM!tests/e2e/configuration/library-mode/lightspeed-stack-skills-directory.yaml (1)
1-26: LGTM!tests/e2e/configuration/library-mode/lightspeed-stack-skills.yaml (1)
1-26: LGTM!
| ## Instructions | ||
|
|
||
| 1. Read the user's input text | ||
| 2. Return the exact text back to the user without modification |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove redundant "back" in "return back".
"Return" already implies giving back; "return back" is pleonastic.
Proposed fix
-2. Return the exact text back to the user without modification
+2. Return the exact text to the user without modification📝 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.
| 2. Return the exact text back to the user without modification | |
| 2. Return the exact text to the user without modification |
🧰 Tools
🪛 LanguageTool
[style] ~17-~17: Using “back” with the verb “return” may be redundant.
Context: ...r's input text 2. Return the exact text back to the user without modification For f...
(RETURN_BACK)
🤖 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/e2e/skills/echo/SKILL.md` at line 17, The SKILL.md guidance in the echo
skill uses the redundant phrase “return back”; update the wording in the
instruction text to say “Return the exact text to the user without modification”
so it stays clear and concise. Make this edit in the echo skill’s step that
describes the response behavior, keeping the rest of the instruction unchanged.
refined E2E tests for skills and added necessary step implementations. close: LCORE-2080
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/e2e/skills/echo/SKILL.md (1)
17-17: 📐 Maintainability & Code Quality | 🟡 Minor | 💤 Low valueRemove redundant "back" in "return back".
"Return" already implies giving back; "return back" is pleonastic. Note that
tests/e2e/features/skills.featurealso embeds this text and must stay in sync.📝 Proposed fix
-2. Return the exact text back to the user without modification +2. Return the exact text to the user without modification🤖 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/e2e/skills/echo/SKILL.md` at line 17, Update the echo skill wording in SKILL.md to remove the redundant “back” so the instruction reads naturally, and make the same wording change in tests/e2e/features/skills.feature to keep both copies in sync. Locate the text in the echo skill docs/feature content and ensure the phrase remains otherwise identical so the expected behavior stays unchanged.
🤖 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 `@tests/e2e/configuration/library-mode/lightspeed-stack-skills-directory.yaml`:
- Around line 23-25: The skills directory configuration is using a relative
path, which makes discovery depend on the current working directory. Update the
skills path in the library-mode YAML to use the mounted absolute path expected
by the feature (for example, the same /app-root/skills base used by skill URI
assertions) so resolution is deterministic regardless of where the service
starts.
In `@tests/e2e/configuration/library-mode/lightspeed-stack-skills.yaml`:
- Around line 23-25: The skill path in the skills configuration is still
relative and depends on the current working directory, so update the
`skills.paths` entry in `lightspeed-stack-skills.yaml` to use the mounted
absolute location instead. Keep the existing `skills` config structure, but
point the path to the deterministic `/app-root/skills/echo` location so it
matches the compose mount and the expectations in the feature scenarios.
In `@tests/e2e/features/skills.feature`:
- Around line 589-592: The `@SkillsMultiConfig` scenarios that switch to
lightspeed-stack-skills-directory.yaml need to clear previously registered MCP
toolgroups first, otherwise list_skills may use stale server-mode state from
skills.yaml. Update the scenario setup in skills.feature to follow the same
reset pattern used by the registration scenarios before the Given The service
uses... step, so the service restart starts from a clean toolgroup state.
---
Duplicate comments:
In `@tests/e2e/skills/echo/SKILL.md`:
- Line 17: Update the echo skill wording in SKILL.md to remove the redundant
“back” so the instruction reads naturally, and make the same wording change in
tests/e2e/features/skills.feature to keep both copies in sync. Locate the text
in the echo skill docs/feature content and ensure the phrase remains otherwise
identical so the expected behavior stays unchanged.
🪄 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: cee8504d-c14a-4cba-ad2d-fda33efa673c
📒 Files selected for processing (14)
docker-compose-library.yamldocker-compose.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills-directory.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills.yamltests/e2e/features/skills.featuretests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.pytests/e2e/skills/echo/SKILL.mdtests/e2e/skills/echo/references/guide.mdtests/e2e/skills/summarize/SKILL.mdtests/e2e/skills/summarize/references/guide.mdtests/e2e/test_list.txt
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: build-pr
- GitHub Check: integration_tests (3.12)
- GitHub Check: integration_tests (3.13)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 1
- 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
⚠️ CI failures not shown inline (4)
GitHub Actions: OpenAPI (Spectral) / spectral: LCORE-2080: Added E2E Steps for Agent Skills
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1muv run python scripts/generate_openapi_schema.py /tmp/openapi-generated.json�[0m
�[36;1mif ! diff -u docs/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json"�[0m
GitHub Actions: OpenAPI (Spectral) / 0_spectral.txt: LCORE-2080: Added E2E Steps for Agent Skills
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1muv run python scripts/generate_openapi_schema.py /tmp/openapi-generated.json�[0m
�[36;1mif ! diff -u docs/openapi.json /tmp/openapi-generated.json; then�[0m
�[36;1m echo "::error::docs/openapi.json is out of date. Regenerate with: uv run scripts/generate_openapi_schema.py docs/openapi.json"�[0m
GitHub Actions: Unit tests / 0_unit_tests (3.13).txt: LCORE-2080: Added E2E Steps for Agent Skills
Conclusion: failure
##[group]Run uv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing
�[36;1muv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing�[0m
shell: /usr/bin/bash -e {0}
env:
UV_PYTHON: 3.13
VIRTUAL_ENV: /home/runner/work/lightspeed-stack/lightspeed-stack/.venv
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Uninstalled 1 package in 4ms
Installed 1 package in 3ms
============================= test session starts ==============================
platform linux -- Python 3.13.14, pytest-9.1.1, pluggy-1.6.0
benchmark: 5.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/runner/work/lightspeed-stack/lightspeed-stack
configfile: pyproject.toml
plugins: asyncio-1.4.0, benchmark-5.2.3, anyio-4.14.1, order-1.5.0, mock-3.15.1, cov-7.1.0, logfire-4.37.0
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 2928 items
tests/unit/a2a_storage/test_in_memory_context_store.py ........ [ 0%]
tests/unit/a2a_storage/test_sqlite_context_store.py .......... [ 0%]
tests/unit/a2a_storage/test_storage_factory.py ........... [ 0%]
tests/unit/app/endpoints/test_a2a.py .............................. [ 2%]
tests/unit/app/endpoints/test_authorized.py ... [ 2%]
tests/unit/app/endpoints/test_config.py .. [ 2%]
tests/unit/app/endpoints/test_conversations.py ......................... [ 3%]
................. [ 3%]
tests/unit/app/endpoints/test_conversations_v2.py ...................... [ 4%]
............... [ 4%]
tests/unit/app/endpoints/test_feedback.py ....................... [ 5%]
tests/unit/ap...
GitHub Actions: Unit tests / unit_tests (3.13): LCORE-2080: Added E2E Steps for Agent Skills
Conclusion: failure
##[group]Run uv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing
�[36;1muv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing�[0m
shell: /usr/bin/bash -e {0}
env:
UV_PYTHON: 3.13
VIRTUAL_ENV: /home/runner/work/lightspeed-stack/lightspeed-stack/.venv
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Uninstalled 1 package in 4ms
Installed 1 package in 3ms
============================= test session starts ==============================
platform linux -- Python 3.13.14, pytest-9.1.1, pluggy-1.6.0
benchmark: 5.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/runner/work/lightspeed-stack/lightspeed-stack
configfile: pyproject.toml
plugins: asyncio-1.4.0, benchmark-5.2.3, anyio-4.14.1, order-1.5.0, mock-3.15.1, cov-7.1.0, logfire-4.37.0
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 2928 items
tests/unit/a2a_storage/test_in_memory_context_store.py ........ [ 0%]
tests/unit/a2a_storage/test_sqlite_context_store.py .......... [ 0%]
tests/unit/a2a_storage/test_storage_factory.py ........... [ 0%]
tests/unit/app/endpoints/test_a2a.py .............................. [ 2%]
tests/unit/app/endpoints/test_authorized.py ... [ 2%]
tests/unit/app/endpoints/test_config.py .. [ 2%]
tests/unit/app/endpoints/test_conversations.py ......................... [ 3%]
................. [ 3%]
tests/unit/app/endpoints/test_conversations_v2.py ...................... [ 4%]
............... [ 4%]
tests/unit/app/endpoints/test_feedback.py ....................... [ 5%]
tests/unit/ap...
🧰 Additional context used
📓 Path-based instructions (2)
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/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
tests/e2e/**/*.{py,feature}
📄 CodeRabbit inference engine (AGENTS.md)
Use behave (BDD) framework for end-to-end testing with Gherkin feature files
Files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.pytests/e2e/features/skills.feature
🧠 Learnings (4)
📚 Learning: 2026-05-20T08:09:30.641Z
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: docs/design/llama-stack-config-merge/poc-results/library-mode/synthesized-run.yaml:107-110
Timestamp: 2026-05-20T08:09:30.641Z
Learning: In Llama-stack config YAMLs, when defining a Llama Guard safety shield entry, set `provider_shield_id` to the *guard model identifier* (e.g., `meta-llama/Llama-Guard-3-8B`). Do not use a chat/generative model id (e.g., `openai/gpt-4o-mini`): a chat-model id (or `native_override`) indicates only an override landed and does **not** mean the safety shield is actually gating queries. Ensure any E2E coverage for the related implementation (JIRA/E2E tests) exercises a real Llama Guard model to verify that the shield is effective.
Applied to files:
docker-compose.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills-directory.yamldocker-compose-library.yamltests/e2e/configuration/library-mode/lightspeed-stack-skills.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yamltests/e2e/configuration/server-mode/lightspeed-stack-skills.yaml
📚 Learning: 2026-04-07T09:20:26.590Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1467
File: tests/e2e/features/steps/common.py:36-49
Timestamp: 2026-04-07T09:20:26.590Z
Learning: For Behave-based Python tests, rely on Behave’s Context layered stack for attribute lifecycle: Behave pushes a new Context layer when entering feature scope (before_feature) and again for scenario scope (before_scenario). Attributes assigned inside given/when/then steps live on the current scenario layer and are automatically removed when the scenario ends. As a result, step-set attributes should not be expected to persist across scenarios or features, and manual cleanup in after_scenario/after_feature is generally unnecessary for attributes set in step functions. Only perform manual cleanup for attributes that you set explicitly in before_feature/before_scenario, since those live on the respective feature/scenario layers.
Applied to files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
📚 Learning: 2026-04-13T13:39:54.963Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1490
File: tests/e2e/features/environment.py:206-211
Timestamp: 2026-04-13T13:39:54.963Z
Learning: In lightspeed-stack E2E tests under tests/e2e/features, it is intentional to set context.feature_config inside Background/step functions (scenario-scoped Behave layer). The environment.py after_scenario restore logic should only restore configuration when context.scenario_lightspeed_override_active is True; this flag is set by configure_service only when a real config switch occurs (so restore does not run for scenarios without a switch). Additionally, steps/common.py’s module-level _active_lightspeed_stack_config_basename is used to prevent re-applying the same config across subsequent scenarios, ensuring scenario_lightspeed_override_active stays False after the first apply. Therefore, reviewers should not “fix” this flow as if feature_config were incorrectly scoped or if after_scenario restoration is missing—config switching and restoration are meant to happen exactly once per actual switch, not redundantly per scenario.
Applied to files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.
Applied to files:
tests/e2e/features/steps/common_http.pytests/e2e/features/steps/llm_query_response.py
🪛 LanguageTool
tests/e2e/skills/echo/SKILL.md
[style] ~17-~17: Using “back” with the verb “return” may be redundant.
Context: ...r's input text 2. Return the exact text back to the user without modification For f...
(RETURN_BACK)
🔇 Additional comments (11)
tests/e2e/skills/echo/references/guide.md (1)
1-20: LGTM!tests/e2e/skills/summarize/SKILL.md (1)
1-22: LGTM!tests/e2e/skills/summarize/references/guide.md (1)
1-21: LGTM!tests/e2e/test_list.txt (1)
35-40: LGTM!tests/e2e/configuration/server-mode/lightspeed-stack-skills-directory.yaml (1)
24-26: 🩺 Stability & AvailabilityUse absolute skills path to avoid CWD-dependent startup failures.
skillsis relative; prefer/app-root/skillsto match the compose mount and the/app-root/skills/...URIs asserted in the feature.tests/e2e/configuration/server-mode/lightspeed-stack-skills.yaml (1)
24-26: 🩺 Stability & AvailabilityPin the skill path to the mounted absolute location.
skills/echois CWD-sensitive; prefer/app-root/skills/echofor deterministic resolution against the compose mount.tests/e2e/features/steps/common_http.py (2)
344-347: 🎯 Functional CorrectnessApply placeholder substitution before parsing expected JSON.
json.loads(context.text)parses raw text, so placeholders like{MODEL}won't be resolved here (unlikecheck_response_partiallyat Line 295). Runreplace_placeholdersfirst to avoid false failures.Proposed fix
- expected_value = json.loads(context.text) + json_str = replace_placeholders(context, context.text) + expected_value = json.loads(json_str)
115-125: LGTM!tests/e2e/features/skills.feature (1)
14-16: 🎯 Functional CorrectnessReset MCP toolgroups before applying the new skills config.
The reset step runs after
The service uses ... configuration; percommon.py, it should run before applying a different MCP-related config so later assertions don't exercise stale toolgroups.tests/e2e/features/steps/llm_query_response.py (2)
386-389: LGTM!Also applies to: 399-407
369-370: 🩺 Stability & AvailabilityNo action needed:
Anyis already imported attests/e2e/features/steps/llm_query_response.py:5, so thelist[dict[str, Any]]annotations won't raiseNameError.> Likely an incorrect or invalid review comment.
| skills: | ||
| paths: | ||
| - skills |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use an absolute skills path to avoid CWD-dependent resolution.
skills is relative, so discovery depends on the service working directory. The skills feature asserts skill URIs as /app-root/skills/..., so pin this to the mounted absolute path (e.g. /app-root/skills) for deterministic resolution.
Proposed change
skills:
paths:
- - skills
+ - /app-root/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.
| skills: | |
| paths: | |
| - skills | |
| skills: | |
| paths: | |
| - /app-root/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 `@tests/e2e/configuration/library-mode/lightspeed-stack-skills-directory.yaml`
around lines 23 - 25, The skills directory configuration is using a relative
path, which makes discovery depend on the current working directory. Update the
skills path in the library-mode YAML to use the mounted absolute path expected
by the feature (for example, the same /app-root/skills base used by skill URI
assertions) so resolution is deterministic regardless of where the service
starts.
| skills: | ||
| paths: | ||
| - skills/echo |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Pin the skill path to the mounted absolute location.
skills/echo is CWD-sensitive. The feature scenarios assert <uri>/app-root/skills/echo</uri>, so prefer /app-root/skills/echo for deterministic resolution against the compose mount.
Proposed change
skills:
paths:
- - skills/echo
+ - /app-root/skills/echo📝 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.
| skills: | |
| paths: | |
| - skills/echo | |
| skills: | |
| paths: | |
| - /app-root/skills/echo |
🤖 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/e2e/configuration/library-mode/lightspeed-stack-skills.yaml` around
lines 23 - 25, The skill path in the skills configuration is still relative and
depends on the current working directory, so update the `skills.paths` entry in
`lightspeed-stack-skills.yaml` to use the mounted absolute location instead.
Keep the existing `skills` config structure, but point the path to the
deterministic `/app-root/skills/echo` location so it matches the compose mount
and the expectations in the feature scenarios.
| @SkillsMultiConfig | ||
| Scenario: Skills directory path discovers all skills in subdirectories via query endpoint | ||
| Given The e2e-test-skill skill directory path is "skills/e2e-test-skill" | ||
| And The e2e-second-skill skill directory path is "skills/e2e-second-skill" | ||
| And The service uses the lightspeed-stack-skills-directory.yaml configuration | ||
| Given The service uses the lightspeed-stack-skills-directory.yaml configuration | ||
| And The service is restarted |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing MCP toolgroups reset before switching to a different skills config.
These @SkillsMultiConfig scenarios switch to lightspeed-stack-skills-directory.yaml, a different MCP config, but unlike the registration scenarios (Lines 15, 143) they don't reset toolgroups first. Without the reset, server-mode registrations can retain the prior skills.yaml toolgroups, making the list_skills assertions exercise stale state.
Example fix pattern
- Given The service uses the lightspeed-stack-skills-directory.yaml configuration
+ Given The service uses the lightspeed-stack-skills-directory.yaml configuration
+ And MCP toolgroups are reset for a new MCP configuration
And The service is restartedAlso applies to: 618-621
🤖 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/e2e/features/skills.feature` around lines 589 - 592, The
`@SkillsMultiConfig` scenarios that switch to
lightspeed-stack-skills-directory.yaml need to clear previously registered MCP
toolgroups first, otherwise list_skills may use stale server-mode state from
skills.yaml. Update the scenario setup in skills.feature to follow the same
reset pattern used by the registration scenarios before the Given The service
uses... step, so the service restart starts from a clean toolgroup state.
4 participants
Description
Added the missing E2E steps for testing agent skills.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit