LCORE-:Restructure pydantic_ai utils into LlamaStack model and provider classes#2025
LCORE-:Restructure pydantic_ai utils into LlamaStack model and provider classes#2025Jazzcort wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Llama Stack client-based factory methods, moves response-parameter conversion into the model module, and updates ChangesLlamaStack factory methods and call-site migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/pydantic_ai_lightspeed/llamastack/_model.py`:
- Around line 69-72: Update the docstrings for
`_model_settings_from_responses_params` and `from_llama_stack_client` to match
the repo’s required Google-style sectioned format.
`_model_settings_from_responses_params` currently lacks the required section
breakdown, and `from_llama_stack_client` should replace `Args:` with
`Parameters:` while also including the appropriate `Returns:` and `Raises:`
sections if applicable. Keep the docstrings descriptive and aligned with the
existing convention used in `_model.py`.
- Around line 389-404: Move the mutual-exclusivity validation for
responses_params and model_settings ahead of
LlamaStackProvider.from_llama_stack_client(client) in the
LlamaStackResponsesModel construction path, so the ValueError is raised before
any provider/client resources are created. Keep the existing logic in the same
method that builds LlamaStackResponsesModel, but check the two inputs first and
only instantiate the provider after the validation passes.
In `@src/pydantic_ai_lightspeed/llamastack/_provider.py`:
- Around line 57-68: The docstring on the LlamaStackProvider factory still uses
the non-standard Args header; update it to the repo-standard Parameters section.
Keep the existing description for the client parameter and ensure the function
docstring in LlamaStackProvider.from_client follows the project’s Google-style
convention with Parameters and Returns sections.
In `@tests/unit/pydantic_ai_lightspeed/llamastack/test_model.py`:
- Around line 330-348: The MCP replay test in
test_replays_mcp_buffered_deltas_with_suffixed_id is asserting a malformed
combined delta string; update the expected replayed.delta value to match the
actual concatenation of delta1 and delta2 produced by _FilteredResponseStream,
so the test reflects the correct MCP buffered replay output rather than an extra
closing brace.
🪄 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: 7ba5e735-ee16-4bdd-99d6-6a9bf0dfef54
📒 Files selected for processing (7)
src/pydantic_ai_lightspeed/capabilities/question_validity/_capability.pysrc/pydantic_ai_lightspeed/llamastack/_model.pysrc/pydantic_ai_lightspeed/llamastack/_provider.pysrc/utils/pydantic_ai.pytests/unit/pydantic_ai_lightspeed/llamastack/test_model.pytests/unit/pydantic_ai_lightspeed/llamastack/test_provider.pytests/unit/utils/test_pydantic_ai.py
💤 Files with no reviewable changes (1)
- tests/unit/utils/test_pydantic_ai.py
📜 Review details
⏰ Context from checks skipped due to timeout. (14)
- GitHub Check: unit_tests (3.12)
- GitHub Check: integration_tests (3.12)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: integration_tests (3.13)
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 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
- GitHub Check: E2E Tests for Lightspeed Evaluation job
⚠️ CI failures not shown inline (5)
GitHub Actions: Type checks / mypy: Restructure pydantic_ai utils into LlamaStack model and provider classes
Conclusion: failure
##[group]Run uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/
�[36;1muv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/�[0m
shell: /usr/bin/bash -e {0}
env:
UV_PYTHON: 3.12
VIRTUAL_ENV: /home/runner/work/lightspeed-stack/lightspeed-stack/.venv
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
src/pydantic_ai_lightspeed/llamastack/_model.py:398: error: Incompatible types in assignment (expression has type "ModelSettings", variable has type "OpenAIResponsesModelSettings") [assignment]
Found 1 error in 1 file (checked 198 source files)
##[error]Process completed with exit code 1.
GitHub Actions: PR Title Checker / check: Restructure pydantic_ai utils into LlamaStack model and provider classes
Conclusion: failure
##[group]Run thehanimo/pr-title-checker@v1.4.3
with:
GITHUB_***REDACTED***
pass_on_octokit_error: false
configuration_path: .github/pr-title-checker-config.json
##[endgroup]
(node:2224) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
Using config file .github/pr-title-checker-config.json from repo lightspeed-core/lightspeed-stack [ref: 8733dfe89799f448c3661a4c283a959dc44f67f7]
(node:2224) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
Creating label (title needs formatting)...
Label (title needs formatting) already created.
Adding label (title needs formatting) to PR...
HttpError: Resource not accessible by integration
##[error]Failed to add label (title needs formatting) to PR
GitHub Actions: PR Title Checker / 0_check.txt: Restructure pydantic_ai utils into LlamaStack model and provider classes
Conclusion: failure
##[group]Run thehanimo/pr-title-checker@v1.4.3
with:
GITHUB_***REDACTED***
pass_on_octokit_error: false
configuration_path: .github/pr-title-checker-config.json
##[endgroup]
(node:2224) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
Using config file .github/pr-title-checker-config.json from repo lightspeed-core/lightspeed-stack [ref: 8733dfe89799f448c3661a4c283a959dc44f67f7]
(node:2224) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
Creating label (title needs formatting)...
Label (title needs formatting) already created.
Adding label (title needs formatting) to PR...
HttpError: Resource not accessible by integration
##[error]Failed to add label (title needs formatting) to PR
GitHub Actions: OpenAPI (Spectral) / spectral: Restructure pydantic_ai utils into LlamaStack model and provider classes
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: Restructure pydantic_ai utils into LlamaStack model and provider classes
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)
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/pydantic_ai_lightspeed/llamastack/_provider.pysrc/utils/pydantic_ai.pysrc/pydantic_ai_lightspeed/llamastack/_model.pysrc/pydantic_ai_lightspeed/capabilities/question_validity/_capability.py
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/pydantic_ai_lightspeed/llamastack/test_model.pytests/unit/pydantic_ai_lightspeed/llamastack/test_provider.py
🧠 Learnings (1)
📚 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:
src/pydantic_ai_lightspeed/llamastack/_provider.pysrc/utils/pydantic_ai.pysrc/pydantic_ai_lightspeed/llamastack/_model.pytests/unit/pydantic_ai_lightspeed/llamastack/test_model.pysrc/pydantic_ai_lightspeed/capabilities/question_validity/_capability.pytests/unit/pydantic_ai_lightspeed/llamastack/test_provider.py
🪛 ast-grep (0.44.0)
tests/unit/pydantic_ai_lightspeed/llamastack/test_provider.py
[warning] 167-167: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 179-179: Do not make http calls without encryption
Context: "http://my-server:8321"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 192-192: Do not make http calls without encryption
Context: "http://my-server:8321/"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 204-204: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 217-217: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 228-228: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🪛 GitHub Actions: Type checks / 0_mypy.txt
src/pydantic_ai_lightspeed/llamastack/_model.py
[error] 398-398: mypy error: Incompatible types in assignment (expression has type "ModelSettings", variable has type "OpenAIResponsesModelSettings"). [assignment]
[error] 1-1: Command failed: uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/ (exit code 1).
🪛 GitHub Actions: Type checks / mypy
src/pydantic_ai_lightspeed/llamastack/_model.py
[error] 398-398: mypy failed: Incompatible types in assignment (expression has type "ModelSettings", variable has type "OpenAIResponsesModelSettings"). [assignment]
| provider = LlamaStackProvider.from_llama_stack_client(client) | ||
|
|
||
| if responses_params is not None and model_settings is not None: | ||
| raise ValueError( | ||
| "You can only pass either ResponsesApiParams or ModelSetting not both." | ||
| ) | ||
| elif responses_params is not None: | ||
| _settings = _model_settings_from_responses_params(responses_params) | ||
| elif model_settings is not None: | ||
| _settings = model_settings | ||
| else: | ||
| _settings = None | ||
|
|
||
| return LlamaStackResponsesModel( | ||
| model_name, provider=provider, profile=profile, settings=_settings | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate exclusivity before constructing the provider.
LlamaStackProvider.from_llama_stack_client(client) runs before the responses_params/model_settings check. On the library-client path, provider construction creates a new httpx.AsyncClient, so the error path allocates resources before immediately raising ValueError.
Suggested fix
- provider = LlamaStackProvider.from_llama_stack_client(client)
-
if responses_params is not None and model_settings is not None:
raise ValueError(
"You can only pass either ResponsesApiParams or ModelSetting not both."
)
elif responses_params is not None:
_settings = _model_settings_from_responses_params(responses_params)
elif model_settings is not None:
_settings = model_settings
else:
_settings = None
+
+ provider = LlamaStackProvider.from_llama_stack_client(client)
return LlamaStackResponsesModel(
model_name, provider=provider, profile=profile, settings=_settings
)📝 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.
| provider = LlamaStackProvider.from_llama_stack_client(client) | |
| if responses_params is not None and model_settings is not None: | |
| raise ValueError( | |
| "You can only pass either ResponsesApiParams or ModelSetting not both." | |
| ) | |
| elif responses_params is not None: | |
| _settings = _model_settings_from_responses_params(responses_params) | |
| elif model_settings is not None: | |
| _settings = model_settings | |
| else: | |
| _settings = None | |
| return LlamaStackResponsesModel( | |
| model_name, provider=provider, profile=profile, settings=_settings | |
| ) | |
| if responses_params is not None and model_settings is not None: | |
| raise ValueError( | |
| "You can only pass either ResponsesApiParams or ModelSetting not both." | |
| ) | |
| elif responses_params is not None: | |
| _settings = _model_settings_from_responses_params(responses_params) | |
| elif model_settings is not None: | |
| _settings = model_settings | |
| else: | |
| _settings = None | |
| provider = LlamaStackProvider.from_llama_stack_client(client) | |
| return LlamaStackResponsesModel( | |
| model_name, provider=provider, profile=profile, settings=_settings | |
| ) |
🧰 Tools
🪛 GitHub Actions: Type checks / 0_mypy.txt
[error] 398-398: mypy error: Incompatible types in assignment (expression has type "ModelSettings", variable has type "OpenAIResponsesModelSettings"). [assignment]
🪛 GitHub Actions: Type checks / mypy
[error] 398-398: mypy failed: Incompatible types in assignment (expression has type "ModelSettings", variable has type "OpenAIResponsesModelSettings"). [assignment]
🤖 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/pydantic_ai_lightspeed/llamastack/_model.py` around lines 389 - 404, Move
the mutual-exclusivity validation for responses_params and model_settings ahead
of LlamaStackProvider.from_llama_stack_client(client) in the
LlamaStackResponsesModel construction path, so the ValueError is raised before
any provider/client resources are created. Keep the existing logic in the same
method that builds LlamaStackResponsesModel, but check the two inputs first and
only instantiate the provider after the validation passes.
|
Will fix the failing CI later. Please hold on to review this one. |
Jazzcort
force-pushed
the
restructure-pydantic-ai-utils-function
branch
3 times, most recently
from
9c6da47 to
3a15fcb
Compare
asimurka
left a comment
asimurka
left a comment
There was a problem hiding this comment.
LGTM, please rebase and fix CI
Jazzcort
force-pushed
the
restructure-pydantic-ai-utils-function
branch
from
3a15fcb to
a0a9154
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/pydantic_ai_lightspeed/llamastack/test_model.py (1)
715-733: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the malformed MCP replay expectation.
Line 733 still expects an extra
}. The replayed delta should be the concatenation ofdelta1anddelta2, so this assertion currently locks in invalid output and will fail against correct behavior.Suggested fix
- assert replayed.delta == '{"arg":"v"}}' + assert replayed.delta == '{"arg":"v"}'🤖 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/pydantic_ai_lightspeed/llamastack/test_model.py` around lines 715 - 733, The MCP replay test in test_replays_mcp_buffered_deltas_with_suffixed_id has a malformed expected delta string: it currently asserts an extra closing brace in the replayed ResponseFunctionCallArgumentsDeltaEvent. Update the assertion to match the actual concatenation of delta1 and delta2 produced by _FilteredResponseStream, and keep the item_id check for the "-call" suffix 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.
Duplicate comments:
In `@tests/unit/pydantic_ai_lightspeed/llamastack/test_model.py`:
- Around line 715-733: The MCP replay test in
test_replays_mcp_buffered_deltas_with_suffixed_id has a malformed expected delta
string: it currently asserts an extra closing brace in the replayed
ResponseFunctionCallArgumentsDeltaEvent. Update the assertion to match the
actual concatenation of delta1 and delta2 produced by _FilteredResponseStream,
and keep the item_id check for the "-call" suffix unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c095c7f-ebba-42fa-ba5d-c65e424624e2
📒 Files selected for processing (8)
src/pydantic_ai_lightspeed/capabilities/question_validity/_capability.pysrc/pydantic_ai_lightspeed/llamastack/_model.pysrc/pydantic_ai_lightspeed/llamastack/_provider.pysrc/utils/pydantic_ai.pytests/unit/pydantic_ai_lightspeed/capabilities/question_validity/test_capability.pytests/unit/pydantic_ai_lightspeed/llamastack/test_model.pytests/unit/pydantic_ai_lightspeed/llamastack/test_provider.pytests/unit/utils/test_pydantic_ai.py
💤 Files with no reviewable changes (1)
- tests/unit/utils/test_pydantic_ai.py
📜 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/unit/pydantic_ai_lightspeed/llamastack/test_provider.pytests/unit/pydantic_ai_lightspeed/llamastack/test_model.pytests/unit/pydantic_ai_lightspeed/capabilities/question_validity/test_capability.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/pydantic_ai_lightspeed/capabilities/question_validity/_capability.pysrc/pydantic_ai_lightspeed/llamastack/_provider.pysrc/pydantic_ai_lightspeed/llamastack/_model.pysrc/utils/pydantic_ai.py
🧠 Learnings (1)
📚 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/unit/pydantic_ai_lightspeed/llamastack/test_provider.pysrc/pydantic_ai_lightspeed/capabilities/question_validity/_capability.pysrc/pydantic_ai_lightspeed/llamastack/_provider.pysrc/pydantic_ai_lightspeed/llamastack/_model.pytests/unit/pydantic_ai_lightspeed/llamastack/test_model.pytests/unit/pydantic_ai_lightspeed/capabilities/question_validity/test_capability.pysrc/utils/pydantic_ai.py
🪛 ast-grep (0.44.0)
tests/unit/pydantic_ai_lightspeed/llamastack/test_provider.py
[warning] 167-167: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 179-179: Do not make http calls without encryption
Context: "http://my-server:8321"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 192-192: Do not make http calls without encryption
Context: "http://my-server:8321/"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 204-204: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 217-217: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 228-228: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🔇 Additional comments (10)
src/pydantic_ai_lightspeed/llamastack/_provider.py (2)
65-69: 📐 Maintainability & Code Quality | 💤 Low valueUse the repo-standard
Parameters:docstring header.This factory's docstring still uses
Args:. Based on learnings, this repo requires theParameters:section header. As per coding guidelines, functions must "include descriptive docstrings" following Google conventions withParameters/Returnssections.Sources: Coding guidelines, Learnings
71-80: LGTM!src/pydantic_ai_lightspeed/llamastack/_model.py (3)
69-72: 📐 Maintainability & Code Quality | 💤 Low valueBoth new docstrings need the repo's sectioned
Parameters:format.
_model_settings_from_responses_paramshas only a one-line docstring (missingParameters/Returns), andfrom_llama_stack_client(lines 372-384) usesArgs:instead ofParameters:. As per coding guidelines, all functions need descriptive docstrings following Google conventions; based on learnings the repo usesParameters:.Also applies to: 372-384
Sources: Coding guidelines, Learnings
389-394: 🩺 Stability & Availability | ⚡ Quick winValidate mutual exclusivity before constructing the provider.
LlamaStackProvider.from_llama_stack_client(client)runs before theresponses_params/model_settingscheck. On the library-client path, provider construction allocates a newhttpx.AsyncClient, so an invalid call leaks resources before theValueErroris raised. Move the validation ahead of provider construction.Suggested fix
- provider = LlamaStackProvider.from_llama_stack_client(client) - if responses_params is not None and model_settings is not None: raise ValueError( "You can only pass either ResponsesApiParams or ModelSetting not both." ) _settings: OpenAIResponsesModelSettings | ModelSettings | None = None if responses_params is not None: _settings = _model_settings_from_responses_params(responses_params) elif model_settings is not None: _settings = model_settings + provider = LlamaStackProvider.from_llama_stack_client(client) + return LlamaStackResponsesModel( model_name, provider=provider, profile=profile, settings=_settings )
52-91: LGTM!src/utils/pydantic_ai.py (1)
76-85: LGTM!src/pydantic_ai_lightspeed/capabilities/question_validity/_capability.py (1)
78-84: LGTM!tests/unit/pydantic_ai_lightspeed/llamastack/test_model.py (1)
1-714: LGTM!Also applies to: 735-762
tests/unit/pydantic_ai_lightspeed/llamastack/test_provider.py (1)
7-8: LGTM!Also applies to: 148-238
tests/unit/pydantic_ai_lightspeed/capabilities/question_validity/test_capability.py (1)
9-9: LGTM!Also applies to: 27-28, 109-137, 152-154, 214-216
Jazzcort
force-pushed
the
restructure-pydantic-ai-utils-function
branch
5 times, most recently
from
33380ad to
71bc46a
Compare
Move llama_stack_provider_from_client and _model_settings_from_responses_params out of utils/pydantic_ai.py into their owning classes as static factory methods: - LlamaStackProvider.from_llama_stack_client() in _provider.py - LlamaStackResponsesModel.from_llama_stack_client() in _model.py - _model_settings_from_responses_params and _LLS_RESPONSES_EXTRA_FIELDS into _model.py Update callers (QuestionValidity, build_agent) to use the new factory methods and relocate corresponding tests to test_model.py and test_provider.py.
Jazzcort
force-pushed
the
restructure-pydantic-ai-utils-function
branch
from
71bc46a to
18a7f41
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/pydantic_ai_lightspeed/llamastack/_model.py (3)
357-388: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring still uses
Args:instead ofParameters:.This was previously flagged for this same function (also referenced as "Also applies to: 366-388"). As per coding guidelines and based on learnings, the repo standard is
Parameters:, notArgs:.🤖 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/pydantic_ai_lightspeed/llamastack/_model.py` around lines 357 - 388, Update the docstring for from_llama_stack_client to use the repo-standard “Parameters:” section header instead of “Args:”. Keep the existing parameter descriptions and the Raises/Returns sections intact, and ensure the wording matches the rest of the module’s docstrings.Sources: Coding guidelines, Learnings
389-405: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProvider is still constructed before the mutual-exclusion check.
LlamaStackProvider.from_llama_stack_client(client)runs before validating thatresponses_paramsandmodel_settingsaren't both set. For the library-client path this allocates anhttpx.AsyncClient(and wraps it inAsyncOpenAI) that is immediately discarded — and never closed — when the subsequentValueErrorfires. This was flagged in a prior review and does not appear to have been addressed yet.🐛 Proposed fix
- provider = LlamaStackProvider.from_llama_stack_client(client) - if responses_params is not None and model_settings is not None: raise ValueError( "You can only pass either ResponsesApiParams or ModelSetting not both." ) _settings: OpenAIResponsesModelSettings | ModelSettings | None = None - if responses_params is not None: _settings = _model_settings_from_responses_params(responses_params) elif model_settings is not None: _settings = model_settings + provider = LlamaStackProvider.from_llama_stack_client(client) + return LlamaStackResponsesModel( model_name, provider=provider, profile=profile, settings=_settings )🤖 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/pydantic_ai_lightspeed/llamastack/_model.py` around lines 389 - 405, The mutual-exclusion validation in the LlamaStack response model factory must happen before constructing the provider. Move the LlamaStackProvider.from_llama_stack_client(client) call in the LlamaStackResponsesModel creation path so responses_params and model_settings are checked first, then only build the provider after the ValueError guard passes. Keep the fix in the same factory method that returns LlamaStackResponsesModel and uses _model_settings_from_responses_params.
69-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required sectioned docstring.
_model_settings_from_responses_paramsonly has a one-line docstring with noParameters:/Returns:sections. As per coding guidelines, "All functions must have complete type annotations for parameters and return types... and include descriptive docstrings" and must "Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes."📐 Proposed fix
def _model_settings_from_responses_params( responses_params: ResponsesApiParams, ) -> OpenAIResponsesModelSettings: - """Map ``ResponsesApiParams`` into Pydantic AI OpenAI Responses model settings.""" + """Map ``ResponsesApiParams`` into Pydantic AI OpenAI Responses model settings. + + Parameters: + responses_params: The Responses API parameters to convert. + + Returns: + The corresponding ``OpenAIResponsesModelSettings`` mapping. + """🤖 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/pydantic_ai_lightspeed/llamastack/_model.py` around lines 69 - 92, The helper `_model_settings_from_responses_params` needs a Google-style docstring with the required sections instead of the current one-line summary. Update the docstring to describe the `responses_params` parameter under a Parameters section and the returned `OpenAIResponsesModelSettings` under a Returns section, matching the function’s purpose and existing type annotations. Keep the implementation unchanged and ensure the docstring follows the project’s sectioned convention for this mapping helper.Source: Coding guidelines
tests/unit/pydantic_ai_lightspeed/llamastack/test_model.py (1)
713-731: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMalformed expected delta string (extra closing brace).
delta1 + delta2='{"arg":' + '"v"}'='{"arg":"v"}', not'{"arg":"v"}}'. This locks in a malformed expectation rather than verifying correct buffered-delta concatenation. Same issue was raised in a prior review.🐛 Proposed fix
- assert replayed.delta == '{"arg":"v"}}' + assert replayed.delta == '{"arg":"v"}'🤖 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/pydantic_ai_lightspeed/llamastack/test_model.py` around lines 713 - 731, The buffered MCP delta assertion in test_replays_mcp_buffered_deltas_with_suffixed_id is expecting a malformed concatenated payload with an extra closing brace. Update the assertion so the replayed ResponseFunctionCallArgumentsDeltaEvent delta matches the actual combined output from _make_delta("mcp-1", ...) inputs, and keep the item_id check for the "-call" suffix intact. This will verify correct delta buffering/concatenation rather than encoding an invalid string expectation.src/pydantic_ai_lightspeed/llamastack/_provider.py (1)
55-81: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStill using
Args:instead of the repo-standardParameters:header.A prior review on this exact function flagged this and was marked "Addressed in commit 33380ad," but the docstring shown here still uses
Args:(line 65). As per coding guidelines, "Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes." Based on learnings, this repo usesParameters:rather thanArgs:.📐 Proposed fix
- Args: + Parameters: client: A Llama Stack client (server or library variant).🤖 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/pydantic_ai_lightspeed/llamastack/_provider.py` around lines 55 - 81, The docstring in from_llama_stack_client still uses the nonstandard Args: section; update it to the repo’s Google-style Parameters: header while keeping the existing parameter description for client and the Returns: section unchanged. Make sure the change is applied in the from_llama_stack_client method of LlamaStackProvider so the docstring matches the project convention.Sources: Coding guidelines, Learnings
🤖 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/utils/pydantic_ai.py`:
- Around line 45-104: The helper functions _json_schema_to_parameters,
_capability_tool_description, and _capability_tools_from_toolset need full
Google-style docstrings instead of single-line summaries. Update each docstring
to include clear Parameters and Returns sections that describe the inputs and
outputs, using the function names above to locate them easily, and keep the
existing type annotations unchanged.
---
Duplicate comments:
In `@src/pydantic_ai_lightspeed/llamastack/_model.py`:
- Around line 357-388: Update the docstring for from_llama_stack_client to use
the repo-standard “Parameters:” section header instead of “Args:”. Keep the
existing parameter descriptions and the Raises/Returns sections intact, and
ensure the wording matches the rest of the module’s docstrings.
- Around line 389-405: The mutual-exclusion validation in the LlamaStack
response model factory must happen before constructing the provider. Move the
LlamaStackProvider.from_llama_stack_client(client) call in the
LlamaStackResponsesModel creation path so responses_params and model_settings
are checked first, then only build the provider after the ValueError guard
passes. Keep the fix in the same factory method that returns
LlamaStackResponsesModel and uses _model_settings_from_responses_params.
- Around line 69-92: The helper `_model_settings_from_responses_params` needs a
Google-style docstring with the required sections instead of the current
one-line summary. Update the docstring to describe the `responses_params`
parameter under a Parameters section and the returned
`OpenAIResponsesModelSettings` under a Returns section, matching the function’s
purpose and existing type annotations. Keep the implementation unchanged and
ensure the docstring follows the project’s sectioned convention for this mapping
helper.
In `@src/pydantic_ai_lightspeed/llamastack/_provider.py`:
- Around line 55-81: The docstring in from_llama_stack_client still uses the
nonstandard Args: section; update it to the repo’s Google-style Parameters:
header while keeping the existing parameter description for client and the
Returns: section unchanged. Make sure the change is applied in the
from_llama_stack_client method of LlamaStackProvider so the docstring matches
the project convention.
In `@tests/unit/pydantic_ai_lightspeed/llamastack/test_model.py`:
- Around line 713-731: The buffered MCP delta assertion in
test_replays_mcp_buffered_deltas_with_suffixed_id is expecting a malformed
concatenated payload with an extra closing brace. Update the assertion so the
replayed ResponseFunctionCallArgumentsDeltaEvent delta matches the actual
combined output from _make_delta("mcp-1", ...) inputs, and keep the item_id
check for the "-call" suffix intact. This will verify correct delta
buffering/concatenation rather than encoding an invalid string expectation.
🪄 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: d54d0efd-913d-4be6-971f-ed25cb9f6afd
📒 Files selected for processing (8)
src/pydantic_ai_lightspeed/capabilities/question_validity/_capability.pysrc/pydantic_ai_lightspeed/llamastack/_model.pysrc/pydantic_ai_lightspeed/llamastack/_provider.pysrc/utils/pydantic_ai.pytests/unit/pydantic_ai_lightspeed/capabilities/question_validity/test_capability.pytests/unit/pydantic_ai_lightspeed/llamastack/test_model.pytests/unit/pydantic_ai_lightspeed/llamastack/test_provider.pytests/unit/utils/test_pydantic_ai.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 1
- 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: server mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
⚠️ CI failures not shown inline (2)
GitHub Actions: PR Title Checker / check: Restructure pydantic_ai utils into LlamaStack model and provider classes
Conclusion: failure
##[group]Run thehanimo/pr-title-checker@v1.4.3
with:
GITHUB_***REDACTED***
pass_on_octokit_error: false
configuration_path: .github/pr-title-checker-config.json
##[endgroup]
(node:2198) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
Using config file .github/pr-title-checker-config.json from repo lightspeed-core/lightspeed-stack [ref: 56ae5fdc6bce2da7499bfeffad1c0c30baf32c8e]
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:2198) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
Creating label (title needs formatting)...
Label (title needs formatting) already created.
Adding label (title needs formatting) to PR...
HttpError: Resource not accessible by integration
##[error]Failed to add label (title needs formatting) to PR
GitHub Actions: PR Title Checker / 0_check.txt: Restructure pydantic_ai utils into LlamaStack model and provider classes
Conclusion: failure
##[group]Run thehanimo/pr-title-checker@v1.4.3
with:
GITHUB_***REDACTED***
pass_on_octokit_error: false
configuration_path: .github/pr-title-checker-config.json
##[endgroup]
(node:2198) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
Using config file .github/pr-title-checker-config.json from repo lightspeed-core/lightspeed-stack [ref: 56ae5fdc6bce2da7499bfeffad1c0c30baf32c8e]
(Use `node --trace-deprecation ...` to show where the warning was created)
(node:2198) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
Creating label (title needs formatting)...
Label (title needs formatting) already created.
Adding label (title needs formatting) to PR...
HttpError: Resource not accessible by integration
##[error]Failed to add label (title needs formatting) to PR
🧰 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/unit/pydantic_ai_lightspeed/capabilities/question_validity/test_capability.pytests/unit/utils/test_pydantic_ai.pytests/unit/pydantic_ai_lightspeed/llamastack/test_provider.pytests/unit/pydantic_ai_lightspeed/llamastack/test_model.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/pydantic_ai_lightspeed/llamastack/_model.pysrc/pydantic_ai_lightspeed/capabilities/question_validity/_capability.pysrc/pydantic_ai_lightspeed/llamastack/_provider.pysrc/utils/pydantic_ai.py
🧠 Learnings (1)
📚 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/unit/pydantic_ai_lightspeed/capabilities/question_validity/test_capability.pytests/unit/utils/test_pydantic_ai.pysrc/pydantic_ai_lightspeed/llamastack/_model.pysrc/pydantic_ai_lightspeed/capabilities/question_validity/_capability.pysrc/pydantic_ai_lightspeed/llamastack/_provider.pytests/unit/pydantic_ai_lightspeed/llamastack/test_provider.pytests/unit/pydantic_ai_lightspeed/llamastack/test_model.pysrc/utils/pydantic_ai.py
🪛 ast-grep (0.44.0)
tests/unit/pydantic_ai_lightspeed/llamastack/test_provider.py
[warning] 167-167: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 179-179: Do not make http calls without encryption
Context: "http://my-server:8321"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 192-192: Do not make http calls without encryption
Context: "http://my-server:8321/"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 204-204: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 217-217: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 228-228: Do not make http calls without encryption
Context: "http://my-server:8321/v1"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🔇 Additional comments (8)
src/pydantic_ai_lightspeed/llamastack/_provider.py (1)
8-9: LGTM!Also applies to: 19-21
tests/unit/pydantic_ai_lightspeed/llamastack/test_provider.py (1)
7-8: LGTM!Also applies to: 148-238
src/pydantic_ai_lightspeed/llamastack/_model.py (1)
22-25: LGTM!tests/unit/pydantic_ai_lightspeed/llamastack/test_model.py (1)
1-711: LGTM!Also applies to: 733-760
src/pydantic_ai_lightspeed/capabilities/question_validity/_capability.py (1)
76-84: LGTM!src/utils/pydantic_ai.py (1)
5-23: LGTM!Also applies to: 107-129, 159-193
tests/unit/pydantic_ai_lightspeed/capabilities/question_validity/test_capability.py (1)
9-9: LGTM!Also applies to: 24-28, 109-137, 150-154, 212-216
tests/unit/utils/test_pydantic_ai.py (1)
5-5: LGTM!Also applies to: 14-20, 184-224
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/pydantic_ai_lightspeed/llamastack/_model.py (3)
357-388: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring still uses
Args:instead ofParameters:.This was previously flagged for this same function (also referenced as "Also applies to: 366-388"). As per coding guidelines and based on learnings, the repo standard is
Parameters:, notArgs:.🤖 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/pydantic_ai_lightspeed/llamastack/_model.py` around lines 357 - 388, Update the docstring for from_llama_stack_client to use the repo-standard “Parameters:” section header instead of “Args:”. Keep the existing parameter descriptions and the Raises/Returns sections intact, and ensure the wording matches the rest of the module’s docstrings.Sources: Coding guidelines, Learnings
389-405: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winProvider is still constructed before the mutual-exclusion check.
LlamaStackProvider.from_llama_stack_client(client)runs before validating thatresponses_paramsandmodel_settingsaren't both set. For the library-client path this allocates anhttpx.AsyncClient(and wraps it inAsyncOpenAI) that is immediately discarded — and never closed — when the subsequentValueErrorfires. This was flagged in a prior review and does not appear to have been addressed yet.🐛 Proposed fix
- provider = LlamaStackProvider.from_llama_stack_client(client) - if responses_params is not None and model_settings is not None: raise ValueError( "You can only pass either ResponsesApiParams or ModelSetting not both." ) _settings: OpenAIResponsesModelSettings | ModelSettings | None = None - if responses_params is not None: _settings = _model_settings_from_responses_params(responses_params) elif model_settings is not None: _settings = model_settings + provider = LlamaStackProvider.from_llama_stack_client(client) + return LlamaStackResponsesModel( model_name, provider=provider, profile=profile, settings=_settings )🤖 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/pydantic_ai_lightspeed/llamastack/_model.py` around lines 389 - 405, The mutual-exclusion validation in the LlamaStack response model factory must happen before constructing the provider. Move the LlamaStackProvider.from_llama_stack_client(client) call in the LlamaStackResponsesModel creation path so responses_params and model_settings are checked first, then only build the provider after the ValueError guard passes. Keep the fix in the same factory method that returns LlamaStackResponsesModel and uses _model_settings_from_responses_params.
69-92: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required sectioned docstring.
_model_settings_from_responses_paramsonly has a one-line docstring with noParameters:/Returns:sections. As per coding guidelines, "All functions must have complete type annotations for parameters and return types... and include descriptive docstrings" and must "Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes."📐 Proposed fix
def _model_settings_from_responses_params( responses_params: ResponsesApiParams, ) -> OpenAIResponsesModelSettings: - """Map ``ResponsesApiParams`` into Pydantic AI OpenAI Responses model settings.""" + """Map ``ResponsesApiParams`` into Pydantic AI OpenAI Responses model settings. + + Parameters: + responses_params: The Responses API parameters to convert. + + Returns: + The corresponding ``OpenAIResponsesModelSettings`` mapping. + """🤖 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/pydantic_ai_lightspeed/llamastack/_model.py` around lines 69 - 92, The helper `_model_settings_from_responses_params` needs a Google-style docstring with the required sections instead of the current one-line summary. Update the docstring to describe the `responses_params` parameter under a Parameters section and the returned `OpenAIResponsesModelSettings` under a Returns section, matching the function’s purpose and existing type annotations. Keep the implementation unchanged and ensure the docstring follows the project’s sectioned convention for this mapping helper.Source: Coding guidelines
tests/unit/pydantic_ai_lightspeed/llamastack/test_model.py (1)
713-731: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMalformed expected delta string (extra closing brace).
delta1 + delta2='{"arg":' + '"v"}'='{"arg":"v"}', not'{"arg":"v"}}'. This locks in a malformed expectation rather than verifying correct buffered-delta concatenation. Same issue was raised in a prior review.🐛 Proposed fix
- assert replayed.delta == '{"arg":"v"}}' + assert replayed.delta == '{"arg":"v"}'🤖 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/pydantic_ai_lightspeed/llamastack/test_model.py` around lines 713 - 731, The buffered MCP delta assertion in test_replays_mcp_buffered_deltas_with_suffixed_id is expecting a malformed concatenated payload with an extra closing brace. Update the assertion so the replayed ResponseFunctionCallArgumentsDeltaEvent delta matches the actual combined output from _make_delta("mcp-1", ...) inputs, and keep the item_id check for the "-call" suffix intact. This will verify correct delta buffering/concatenation rather than encoding an invalid string expectation.src/pydantic_ai_lightspeed/llamastack/_provider.py (1)
55-81: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStill using
Args:instead of the repo-standardParameters:header.A prior review on this exact function flagged this and was marked "Addressed in commit 33380ad," but the docstring shown here still uses
Args:(line 65). As per coding guidelines, "Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes." Based on learnings, this repo usesParameters:rather thanArgs:.📐 Proposed fix
- Args: + Parameters: client: A Llama Stack client (server or library variant).🤖 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/pydantic_ai_lightspeed/llamastack/_provider.py` around lines 55 - 81, The docstring in from_llama_stack_client still uses the nonstandard Args: section; update it to the repo’s Google-style Parameters: header while keeping the existing parameter description for client and the Returns: section unchanged. Make sure the change is applied in the from_llama_stack_client method of LlamaStackProvider so the docstring matches the project convention.Sources: Coding guidelines, Learnings
🤖 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/utils/pydantic_ai.py`:
- Around line 45-104: The helper functions _json_schema_to_parameters,
_capability_tool_description, and _capability_tools_from_toolset need full
Google-style docstrings instead of single-line summaries. Update each docstring
to include clear Parameters and Returns sections that describe the inputs and
outputs, using the function names above to locate them easily, and keep the
existing type annotations unchanged.
---
Duplicate comments:
In `@src/pydantic_ai_lightspeed/llamastack/_model.py`:
- Around line 357-388: Update the docstring for from_llama_stack_client to use
the repo-standard “Parameters:” section header instead of “Args:”. Keep the
existing parameter descriptions and the Raises/Returns sections intact, and
ensure the wording matches the rest of the module’s docstrings.
- Around line 389-405: The mutual-exclusion validation in the LlamaStack
response model factory must happen before constructing the provider. Move the
LlamaStackProvider.from_llama_stack_client(client) call in the
LlamaStackResponsesModel creation path so responses_params and model_settings
are checked first, then only build the provider after the ValueError guard
passes. Keep the fix in the same factory method that returns
LlamaStackResponsesModel and uses _model_settings_from_responses_params.
- Around line 69-92: The helper `_model_settings_from_responses_params` needs a
Google-style docstring with the required sections instead of the current
one-line summary. Update the docstring to describe the `responses_params`
parameter under a Parameters section and the returned
`OpenAIResponsesModelSettings` under a Returns section, matching the function’s
purpose and existing type annotations. Keep the implementation unchanged and
ensure the docstring follows the project’s sectioned convention for this mapping
helper.
In `@src/pydantic_ai_lightspeed/llamastack/_provider.py`:
- Around line 55-81: The docstring in from_llama_stack_client still uses the
nonstandard Args: section; update it to the repo’s Google-style Parameters:
header while keeping the existing parameter description for client and the
Returns: section unchanged. Make sure the change is applied in the
from_llama_stack_client method of LlamaStackProvider so the docstring matches
the project convention.
In `@tests/unit/pydantic_ai_lightspeed/llamastack/test_model.py`:
- Around line 713-731: The buffered MCP delta assertion in
test_replays_mcp_buffered_deltas_with_suffixed_id is expecting a malformed
concatenated payload with an extra closing brace. Update the assertion so the
replayed ResponseFunctionCallArgumentsDeltaEvent delta matches the actual
combined output from _make_delta("mcp-1", ...) inputs, and keep the item_id
check for the "-call" suffix intact. This will verify correct delta
buffering/concatenation rather than encoding an invalid string expectation.
🪄 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: d54d0efd-913d-4be6-971f-ed25cb9f6afd
📒 Files selected for processing (8)
src/pydantic_ai_lightspeed/capabilities/question_validity/_capability.pysrc/pydantic_ai_lightspeed/llamastack/_model.pysrc/pydantic_ai_lightspeed/llamastack/_provider.pysrc/utils/pydantic_ai.pytests/unit/pydantic_ai_lightspeed/capabilities/question_validity/test_capability.pytests/unit/pydantic_ai_lightspeed/llamastack/test_model.pytests/unit/pydantic_ai_lightspeed/llamastack/test_provider.pytests/unit/utils/test_pydantic_ai.py
📜 Review details
🔇 Additional comments (8)
src/pydantic_ai_lightspeed/llamastack/_provider.py (1)
8-9: LGTM!Also applies to: 19-21
tests/unit/pydantic_ai_lightspeed/llamastack/test_provider.py (1)
7-8: LGTM!Also applies to: 148-238
src/pydantic_ai_lightspeed/llamastack/_model.py (1)
22-25: LGTM!tests/unit/pydantic_ai_lightspeed/llamastack/test_model.py (1)
1-711: LGTM!Also applies to: 733-760
src/pydantic_ai_lightspeed/capabilities/question_validity/_capability.py (1)
76-84: LGTM!src/utils/pydantic_ai.py (1)
5-23: LGTM!Also applies to: 107-129, 159-193
tests/unit/pydantic_ai_lightspeed/capabilities/question_validity/test_capability.py (1)
9-9: LGTM!Also applies to: 24-28, 109-137, 150-154, 212-216
tests/unit/utils/test_pydantic_ai.py (1)
5-5: LGTM!Also applies to: 14-20, 184-224
🛑 Comments failed to post (1)
src/utils/pydantic_ai.py (1)
45-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
New helper functions are missing sectioned docstrings.
_json_schema_to_parameters,_capability_tool_description, and_capability_tools_from_toolseteach have only a one-line docstring with noParameters:/Returns:sections. As per coding guidelines, "All functions must have complete type annotations for parameters and return types... and include descriptive docstrings" and must "Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes."📐 Proposed fix (representative example)
def _json_schema_to_parameters( schema: Optional[dict[str, Any]], ) -> list[dict[str, Any]]: - """Convert a JSON Schema object to the flat parameter list used by ``/tools``.""" + """Convert a JSON Schema object to the flat parameter list used by ``/tools``. + + Parameters: + schema: The JSON Schema object to convert, or None. + + Returns: + A list of flat parameter dictionaries describing each schema property. + """The same pattern (adding
Parameters:/Returns:) should be applied to_capability_tool_descriptionand_capability_tools_from_toolset.🤖 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/utils/pydantic_ai.py` around lines 45 - 104, The helper functions _json_schema_to_parameters, _capability_tool_description, and _capability_tools_from_toolset need full Google-style docstrings instead of single-line summaries. Update each docstring to include clear Parameters and Returns sections that describe the inputs and outputs, using the function names above to locate them easily, and keep the existing type annotations unchanged.Source: Coding guidelines
|
@asimurka The linter and format checks are good. The CI failures are from integration and e2e and I believe it's not caused by this patch, so this should be ready to merge. 😁 |
|
Fingers crossed this gets merged before anything else touches the related modules—I've already rebased this one twice. 😭 |
asimurka
changed the title
|
Thanks for the title fix! Now I know the drill 😁 |
2 participants
Description
Move llama_stack_provider_from_client and _model_settings_from_responses_params out of utils/pydantic_ai.py into their owning classes as static factory methods:
Update callers (QuestionValidity, build_agent) to use the new factory methods and relocate corresponding tests to test_model.py and test_provider.py.
Note that
TestFilteredResponseStream,TestPrepareConversationContinuation,TestRequest, andTestRequestStreamintests/unit/pydantic_ai_lightspeed/llamastack/test_model.pyare out-of-scope but it's good to have the unit test cover the entire module instead of just partial of it. If we want the PR to only cover the restructure related part, I can remove these test cases later. I'm fine with either ways.Type of change
Tools used to create PR
Test cases are drafted by claude-opus-4.6 and refined by me.
Related Tickets & Documents
Related discussion: here
Checklist before requesting a review
Testing
Everything related to pydantic agent should work exactly like it used to since this patch is just to restructure the utility functions under the hood which should not affect the overall behavior.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests