Properly handle unknown conversation_id

[max-svistunov]

Description

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

Related Tickets & Documents

• Related Issue LCORE-474
• Closes LCORE-474

Checklist before requesting a review

• I have performed a self-review of my code.
• PR has passed all pre-merge test jobs.
• If it is a core feature, I have added thorough tests.

Testing

• Please provide detailed steps to perform tests related to this code change.
• How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

• Bug Fixes

  • Querying a conversation you don’t own now returns 404 Not Found with “Conversation not found” (replacing the previous 403 Forbidden). Logs updated to reflect the not-found outcome.

• Tests

  • Added a unit test to verify a 404 response and the “Conversation not found” message when a requested conversation is not found.

[coderabbitai]

Walkthrough

The query endpoint now returns HTTP 404 with "Conversation not found" when a conversation is missing or not owned by the user; log message text and argument order were adjusted. A unit test asserting the 404 behaviour was added (duplicated in the file).

Changes

Cohort / File(s)	Summary
Endpoint ownership handling src/app/endpoints/query.py	Change response for non-owned/missing conversation from 403 Forbidden to 404 Not Found; update error detail to "conversation not found"; adjust log message text and swap log argument order to (conversation_id, user_id).
Unit tests for 404 behavior tests/unit/app/endpoints/test_query.py	Add test_query_endpoint_handler_conversation_not_found which patches validate_conversation_ownership to return None and asserts a 404 with "Conversation not found"; the test appears twice (duplicate insertion).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant API as Query Endpoint
  participant Auth as Auth/Ownership Validator
  participant Store as Conversation Store

  User->>API: POST /query {conversation_id, ...}
  API->>Auth: validate_conversation_ownership(user_id, conversation_id)
  alt ownership missing or not found
    Auth-->>API: None
    API-->>User: 404 Not Found ("Conversation not found")
    note right of API #D9EAD3: Log updated: conversation_id first, then user_id
  else ownership valid
    Auth-->>API: ownership ok
    API->>Store: fetch/process conversation data
    Store-->>API: result
    API-->>User: 200 OK (response)
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A hop, a log flipped to four-oh-four,
I nibble at tests and tidy the floor.
If conversations hide, we mark them as gone,
I thump at duplicates then scamper on. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues Check	⚠️ Warning	The PR is linked to issue #474, which requires generating an SVG configuration diagram and updating documentation, yet the changes only modify query endpoint error handling and add related tests with no SVG generation or documentation updates. This mismatch shows the code does not address the linked issue’s objectives.	Update the PR to implement the SVG diagram generation and documentation build tooling changes described in issue #474, or correct the linked issue reference if this endpoint fix belongs to a different ticket.
Out of Scope Changes Check	⚠️ Warning	All modifications in this PR target query_endpoint_handler logic and tests, which are unrelated to generating SVG configuration diagrams or documentation updates required by issue #474. These changes therefore fall outside the scope of the linked issue’s objectives.	Extract the conversation_id error-handling fix into a separate PR against the appropriate bug issue and reserve this PR exclusively for implementing the SVG configuration diagram generation and build tooling updates specified in issue #474.

✅ 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 “Properly handle unknown conversation_id” succinctly describes the primary change to the query_endpoint_handler, which now returns a 404 for non-existent conversations. It is concise, specific, and accurately reflects the main update in the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a036c0 and 0b3ebd5.

📒 Files selected for processing (2)

• src/app/endpoints/query.py (1 hunks)
• tests/unit/app/endpoints/test_query.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/app/endpoints/query.py

🧰 Additional context used

📓 Path-based instructions (3)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed

Files:

• tests/unit/app/endpoints/test_query.py

tests/{unit,integration}/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard

Files:

• tests/unit/app/endpoints/test_query.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests

Files:

• tests/unit/app/endpoints/test_query.py

🧬 Code graph analysis (1)

tests/unit/app/endpoints/test_query.py (3)

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

• dummy_request (30-40)

src/models/requests.py (1)

• QueryRequest (72-222)

src/app/endpoints/query.py (1)

• query_endpoint_handler (212-389)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: e2e_tests (ci)

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00af363 and 10de8d1.

📒 Files selected for processing (2)

• src/app/endpoints/query.py (1 hunks)
• tests/unit/app/endpoints/test_query.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (7)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed

Files:

• tests/unit/app/endpoints/test_query.py
• src/app/endpoints/query.py

tests/{unit,integration}/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard

Files:

• tests/unit/app/endpoints/test_query.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests

Files:

• tests/unit/app/endpoints/test_query.py

src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)

Files:

• src/app/endpoints/query.py

src/app/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code

Files:

• src/app/endpoints/query.py

src/{app/**/*.py,client.py}

📄 CodeRabbit inference engine (CLAUDE.md)

Use async def for I/O-bound operations and external API calls

Files:

• src/app/endpoints/query.py

src/app/endpoints/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling

Files:

• src/app/endpoints/query.py

🧬 Code graph analysis (1)

tests/unit/app/endpoints/test_query.py (3)

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

• dummy_request (30-40)

src/models/requests.py (1)

• QueryRequest (72-222)

src/app/endpoints/query.py (1)

• query_endpoint_handler (212-389)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: e2e_tests (ci)

🔇 Additional comments (2)

src/app/endpoints/query.py (1)

259-269: LGTM: Status code change from 403 to 404 is appropriate.

The change from HTTP_403_FORBIDDEN to HTTP_404_NOT_FOUND when a conversation is not found correctly aligns the response with REST semantics. Returning 404 for both non-existent and not-owned conversations also prevents information disclosure about which conversation IDs exist in the system, which is a good security practice.

The log parameter order fix (conversation_id before user_id) now matches the format string placeholders.

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

2033-2052: No duplicate test definition found. Verified only one occurrence of test_query_endpoint_handler_conversation_not_found in tests/unit/app/endpoints/test_query.py; no removal needed.

Likely an incorrect or invalid review comment.

[tisnik]

LGTM