Skip to content

LCORE-1931: Use HttpUrl instead of plain string#1552

Merged
tisnik merged 1 commit into
lightspeed-core:mainfrom
tisnik:lcore-1931-use-http-url-instead-of-plain-string
Apr 20, 2026
Merged

LCORE-1931: Use HttpUrl instead of plain string#1552
tisnik merged 1 commit into
lightspeed-core:mainfrom
tisnik:lcore-1931-use-http-url-instead-of-plain-string

Conversation

@tisnik
Copy link
Copy Markdown
Contributor

@tisnik tisnik commented Apr 20, 2026

Description

LCORE-1931: Use HttpUrl instead of plain string

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

  • Assisted-by: N/A
  • Generated by: N/A

Related Tickets & Documents

  • Related Issue #LCORE-1931

Summary by CodeRabbit

  • Tests
    • Improved test coverage and type safety validation for document URL handling in conversation and response models.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 20, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d9172207-ec00-4734-85f5-cc47922b5cda

📥 Commits

Reviewing files that changed from the base of the PR and between 2b947a9 and 3ddfb0a.

📒 Files selected for processing (2)
  • tests/unit/app/endpoints/test_conversations_v2.py
  • tests/unit/models/responses/test_rag_chunk.py
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
  • GitHub Check: Pyright
  • GitHub Check: integration_tests (3.12)
  • GitHub Check: build-pr
  • GitHub Check: Pylinter
  • GitHub Check: unit_tests (3.12)
  • GitHub Check: unit_tests (3.13)
  • GitHub Check: E2E: library mode / ci / group 2
  • GitHub Check: E2E: server mode / ci / group 3
  • GitHub Check: E2E: server mode / ci / group 2
  • GitHub Check: E2E: library mode / ci / group 3
  • GitHub Check: E2E: library mode / ci / group 1
  • GitHub Check: E2E: server mode / ci / group 1
  • GitHub Check: E2E Tests for Lightspeed Evaluation job
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Use absolute imports for internal modules: from authentication import get_auth_dependency
Import FastAPI dependencies with: from fastapi import APIRouter, HTTPException, Request, status, Depends
Import Llama Stack client with: from llama_stack_client import AsyncLlamaStackClient
Check constants.py for shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Use logger = get_logger(__name__) from log.py for module logging
Type aliases defined at module level for clarity
Use Final[type] as type hint for all constants
All functions require docstrings with brief descriptions
Complete type annotations for parameters and return types in functions
Use typing_extensions.Self for model validators in Pydantic models
Use modern union type syntax str | int instead of Union[str, int]
Use Optional[Type] for optional type hints
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Use async def for I/O operations and external API calls
Handle APIConnectionError from Llama Stack in error handling
Use standard log levels with clear purposes: debug, info, warning, error
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes: Configuration, Error/Exception, Resolver, Interface
Use ABC for abstract base classes with @abstractmethod decorators
Use @model_validator and @field_validator for Pydantic model validation
Complete type annotations for all class attributes; use specific types, not Any
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections

Files:

  • tests/unit/models/responses/test_rag_chunk.py
  • tests/unit/app/endpoints/test_conversations_v2.py
tests/{unit,integration}/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest; pytest is the standard for this project
Use pytest-mock for AsyncMock objects in tests
Use marker pytest.mark.asyncio for async tests
Unit tests require 60% coverage, integration tests 10%

Files:

  • tests/unit/models/responses/test_rag_chunk.py
  • tests/unit/app/endpoints/test_conversations_v2.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: are-ces
Repo: lightspeed-core/lightspeed-stack PR: 1073
File: src/app/endpoints/query_v2.py:575-586
Timestamp: 2026-01-30T13:33:40.479Z
Learning: In `src/app/endpoints/query_v2.py`, the `parse_referenced_documents_from_responses_api` function filters referenced documents to include only those with `doc_title` or `doc_url` because these documents are displayed in the frontend. Documents with only `doc_id` are intentionally excluded as they wouldn't provide useful information to end users.
📚 Learning: 2026-01-30T13:33:40.479Z
Learnt from: are-ces
Repo: lightspeed-core/lightspeed-stack PR: 1073
File: src/app/endpoints/query_v2.py:575-586
Timestamp: 2026-01-30T13:33:40.479Z
Learning: In `src/app/endpoints/query_v2.py`, the `parse_referenced_documents_from_responses_api` function filters referenced documents to include only those with `doc_title` or `doc_url` because these documents are displayed in the frontend. Documents with only `doc_id` are intentionally excluded as they wouldn't provide useful information to end users.

Applied to files:

  • tests/unit/models/responses/test_rag_chunk.py
  • tests/unit/app/endpoints/test_conversations_v2.py
📚 Learning: 2026-04-19T15:40:25.624Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-19T15:40:25.624Z
Learning: Applies to src/app/endpoints/**/*.py : Use FastAPI `HTTPException` with appropriate status codes for API endpoints

Applied to files:

  • tests/unit/app/endpoints/test_conversations_v2.py
🔇 Additional comments (3)
tests/unit/models/responses/test_rag_chunk.py (2)

3-3: LGTM — URL fixtures are now explicitly typed.

The HttpUrl import and fixture updates keep referenced document test data aligned with URL validation while preserving useful doc_title/doc_url coverage. Based on learnings, parse_referenced_documents_from_responses_api filters referenced documents to include only those with doc_title or doc_url because these documents are displayed in the frontend.

Also applies to: 168-176, 188-190


124-129: LGTM — explicit optional narrowing improves the test.

The added non-None assertion makes the intent clear before indexing chunk.attributes.

tests/unit/app/endpoints/test_conversations_v2.py (1)

10-10: LGTM — conversation fixtures now use typed URLs consistently.

The HttpUrl updates are consistent across the referenced-document test cases, and the surrounding assertions still validate the response-facing values.

Also applies to: 100-108, 200-201, 567-573


Walkthrough

Tests updated to construct ReferencedDocument objects using pydantic.HttpUrl for doc_url instead of plain strings. Changes affect test cases verifying referenced documents in conversation responses and serialization output. An explicit non-None assertion for RAGChunk.attributes was added.

Changes

Cohort / File(s) Summary
Conversation endpoint tests
tests/unit/app/endpoints/test_conversations_v2.py
Updated multiple test cases to wrap doc_url values in pydantic.HttpUrl(...) when constructing ReferencedDocument objects, while maintaining existing str(doc_url) assertions.
RAG chunk model tests
tests/unit/models/responses/test_rag_chunk.py
Updated ReferencedDocument construction to use pydantic.HttpUrl for doc_url instead of raw strings. Added import of HttpUrl from pydantic and introduced explicit non-None assertion for RAGChunk.attributes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change—refactoring to use HttpUrl instead of plain strings in URL handling, which aligns with the file-level changes in both test files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tisnik tisnik merged commit ac428f3 into lightspeed-core:main Apr 20, 2026
27 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant