Conversation
…cle, and project management
…d improve microservice endpoint path resolution.
… parsing in the microservice client.
…ror handling and support for varying JSON structures
…eader Fix registervehicle authheader
WalkthroughThis PR refactors token handling from module-level injection to a thread-safe ContextVar pattern, expands the microservice client with new async APIs for appointments, vehicles, projects, and profile management, adds JWT-based user context propagation with token extraction, enhances agent invocation with async/sync fallback resilience, and introduces comprehensive tests for invocation and tool concurrency patterns. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Agent as Agent Core
participant TokenCtx as Token Context
participant Tools as Agent Tools
participant MicroClient as Microservice Client
participant API as Backend API
Client->>Agent: invoke_agent(payload, user_token)
Agent->>TokenCtx: set(user_token)
alt ainvoke available
Agent->>Agent: ainvoke(payload)
else fallback to sync
Agent->>Agent: to_thread(run, payload)
end
Agent->>Tools: execute tool calls
Tools->>TokenCtx: get()
Tools->>MicroClient: tool_operation(token_context.get())
MicroClient->>MicroClient: _extract_user_from_token(token)
MicroClient->>MicroClient: build X-User-Subject, X-User-Roles headers
MicroClient->>API: POST/GET/PUT/DELETE with auth headers
API-->>MicroClient: response
MicroClient-->>Tools: result
Tools-->>Agent: tool output
Agent->>Agent: normalize_output(result)
Agent->>Agent: extract tool_executed status
Agent-->>Client: formatted response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/microservice_client.py (1)
249-260: Fix potential crash when logs API returns a list
get_time_logs_for_serviceassumesdatais a dict and callsdata.get("error"), but_make_get_requestcan return a list on success. If the service returns a bare list of logs, this will raiseAttributeError.- data = await self._make_get_request(url, token) - - if data.get("error"): - logger.warning(f"Error fetching logs for {service_id}: {data['error']}") - return [] - - return self._parse_logs_response(data) + data = await self._make_get_request(url, token) + + # If the helper returned an error dict, handle it explicitly + if isinstance(data, dict) and data.get("error"): + logger.warning(f"Error fetching logs for {service_id}: {data['error']}") + return [] + + # Otherwise, normalize to a list via the parser + return self._parse_logs_response(data)This keeps existing behavior for error responses while handling list responses safely.
🧹 Nitpick comments (9)
services/token_context.py (1)
1-4: ContextVar-based token storage is appropriate for async/concurrent flowsUsing
ContextVarfor the per-request token is a solid improvement over module-level globals. Consider usingOptional[str]withdefault=Noneinstead of an empty string so downstream code can distinguish “no token set” from “empty token” more explicitly.test_agent_core_invocation.py (2)
11-46: Module stubs are effective; can be simplified and lint-friendlyThe
sys.modulesstubs nicely decouple these tests from reallangchain/Google installs. To avoid Ruff’s B010 warnings and make this a bit clearer, you can assign attributes directly on the stub modules and use underscore-prefixed args in helpers, e.g.:-langchain_agents = sys.modules.get('langchain.agents') -setattr(langchain_agents, 'AgentExecutor', type('AgentExecutor', (), {})) -setattr(langchain_agents, 'initialize_agent', lambda *a, **k: MagicMock()) -setattr(langchain_agents, 'AgentType', type('AgentType', (), {})) +langchain_agents = sys.modules.get('langchain.agents') +langchain_agents.AgentExecutor = type("AgentExecutor", (), {}) +def _initialize_agent(*_args, **_kwargs): + return MagicMock() +langchain_agents.initialize_agent = _initialize_agent +langchain_agents.AgentType = type("AgentType", (), {}) @@ -langchain_tools = sys.modules.get('langchain.tools') -class _StructuredTool: - @classmethod - def from_function(cls, *args, **kwargs): - return None -setattr(langchain_tools, 'StructuredTool', _StructuredTool) +langchain_tools = sys.modules.get('langchain.tools') +class _StructuredTool: + @classmethod + def from_function(cls, *_args, **_kwargs): + return None +langchain_tools.StructuredTool = _StructuredTool @@ -langchain_prompts = sys.modules.get('langchain_core.prompts') -setattr(langchain_prompts, 'ChatPromptTemplate', type('ChatPromptTemplate', (), {'from_messages': classmethod(lambda cls, x: None)})) -setattr(langchain_prompts, 'MessagesPlaceholder', type('MessagesPlaceholder', (), {})) - -setattr(sys.modules.get('langchain_google_genai'), 'ChatGoogleGenerativeAI', type('ChatGoogleGenerativeAI', (), {})) +langchain_prompts = sys.modules.get('langchain_core.prompts') +class _ChatPromptTemplate: + @classmethod + def from_messages(cls, _messages): + return None +langchain_prompts.ChatPromptTemplate = _ChatPromptTemplate +langchain_prompts.MessagesPlaceholder = type("MessagesPlaceholder", (), {}) + +sys.modules.get('langchain_google_genai').ChatGoogleGenerativeAI = type( + "ChatGoogleGenerativeAI", (), {} +)This keeps behavior the same but avoids the lint warnings and makes the stubs a bit easier to read.
57-59: Quiet unused-argument warnings in small executor stubs
payloadisn’t used in these test-only executors, which is fine. If you want Ruff completely clean, you can signal intentional unused args:class SyncExecutor: - def run(self, payload): + def run(self, _payload): return ("sync output", [("action1", "tool-output")]) @@ class AsyncExecutor: - async def ainvoke(self, payload): + async def ainvoke(self, _payload): return {"output": "async output", "intermediate_steps": []}Purely cosmetic, but keeps static analysis noise down.
Also applies to: 83-84
services/agent_core.py (2)
116-144: Consider reducing exposure of full user queries in logsThe off-topic branch logs the entire
user_queryat info level:logger.info(f"Query appears off-topic ...: {user_query}")If chat messages may contain PII or sensitive content, consider redacting/summarizing the query, or logging a hash/short excerpt instead, especially in production.
146-189: ContextVar injection and async/sync fallback look good; clean up unused exception variableUsing
token_context.set(user_token)before invoking the agent, combined withasyncio.to_threadfor the syncrunfallback, is a solid pattern for async + threaded interoperability and per-request token isolation.One small cleanup: the
exvariable is never used in theexceptblock:except Exception as ex: logger.exception("AgentExecutor invocation failed") raiseYou can drop the binding to avoid the Ruff warning:
- except Exception as ex: + except Exception: logger.exception("AgentExecutor invocation failed") raiseOtherwise the control flow and error handling here look correct.
test_new_tools.py (1)
76-101: Strengthen concurrency test by asserting client token usage
test_concurrency_contextcurrently only checks that each coroutine sees its owntoken_contextvalue at the end. To fully validate per-request token propagation into the tool, also assert the tokens used inmock_client.get_appointment_slots:@@ async def test_concurrency_context(): - # Run two "users" concurrently - results = await asyncio.gather( - user_action("token_A"), - user_action("token_B") - ) - - assert results[0] == "token_A" - assert results[1] == "token_B" + # Run two "users" concurrently + results = await asyncio.gather( + user_action("token_A"), + user_action("token_B"), + ) + + # Each coroutine should preserve its own token_context value + assert results[0] == "token_A" + assert results[1] == "token_B" + + # And both tokens should have been passed through to the client + called_tokens = [call.args[2] for call in mock_client.get_appointment_slots.call_args_list] + assert set(called_tokens) == {"token_A", "token_B"}This keeps the concurrency check while also verifying that the tool actually uses the per-task token when calling the microservice client.
services/agent_tools.py (2)
7-15: Makeclient is Nonefailures explicit instead ofAttributeErrorIf
get_microservice_client()fails,clientis set toNoneand all tools will later fail with'NoneType' object has no attribute ...', which is hard to diagnose.Consider either:
- Restricting the
exceptto the specific failure you expect in tests (e.g., import/config errors), or- Adding a simple guard in each public tool (or a small helper) to raise/return a clear error when
client is None, e.g.:def _ensure_client() -> None: if client is None: raise RuntimeError("Microservice client not available; cannot call tools.")and call
_ensure_client()at the start of each tool.
191-210: Improve typing and readability for profile update payload
update_my_profile_toolcurrently:
- Uses
str = Nonedefaults without marking parameters as optional in type hints.- Packs multiple statements on a single line, hurting readability.
A small cleanup:
-async def update_my_profile_tool(full_name: str = None, phone: str = None, address: str = None) -> str: +async def update_my_profile_tool( + full_name: str | None = None, + phone: str | None = None, + address: str | None = None, +) -> str: @@ - payload = {} - if full_name: payload['fullName'] = full_name - if phone: payload['phone'] = phone - if address: payload['address'] = address + payload: dict[str, str] = {} + if full_name: + payload["fullName"] = full_name + if phone: + payload["phone"] = phone + if address: + payload["address"] = addressThis aligns with type-checkers and resolves the Ruff style warnings without changing behavior.
services/microservice_client.py (1)
59-101: Loosen type annotations and consider usinglogging.exceptionin generic error handlers
_make_get_requestis annotated to returnDict[str, Any], but it can return a list (e.g., when the service returns[]). The signature should reflect this, e.g.:from collections.abc import Mapping from typing import Union async def _make_get_request( self, url: str, token: str, params: dict[str, Any] | None = None, ) -> dict[str, Any] | list[Any]: ...
- In the generic
except Exceptionbranch, usinglogger.exception(...)instead oflogger.error(...)gives you a stack trace, which is very helpful for debugging unexpected failures, while still returning the same error shape.These are internal changes only and won’t affect callers, but they will improve diagnostics and type-checking fidelity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
QUICK_REFERENCE.md(1 hunks)requirements.txt(1 hunks)services/agent_core.py(3 hunks)services/agent_tools.py(1 hunks)services/microservice_client.py(5 hunks)services/token_context.py(1 hunks)test_agent_core_invocation.py(1 hunks)test_new_tools.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
services/agent_core.py (1)
test_agent_core_invocation.py (2)
ainvoke(83-84)run(58-59)
test_agent_core_invocation.py (3)
services/agent_core.py (2)
AIAgentService(18-237)invoke_agent(99-237)services/microservice_client.py (1)
get_user_context(171-173)services/rag.py (1)
retrieve_and_format(139-186)
services/microservice_client.py (1)
models/chat.py (1)
UserContext(23-27)
🪛 Ruff (0.14.5)
services/agent_tools.py
10-10: Do not catch blind exception: Exception
(BLE001)
191-191: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
191-191: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
191-191: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
198-198: Multiple statements on one line (colon)
(E701)
199-199: Multiple statements on one line (colon)
(E701)
200-200: Multiple statements on one line (colon)
(E701)
services/agent_core.py
186-186: Local variable ex is assigned to but never used
Remove assignment to unused variable ex
(F841)
test_agent_core_invocation.py
30-30: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
31-31: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
31-31: Unused lambda argument: a
(ARG005)
31-31: Unused lambda argument: k
(ARG005)
32-32: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
37-37: Unused class method argument: args
(ARG003)
37-37: Unused class method argument: kwargs
(ARG003)
39-39: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
42-42: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
42-42: Unused lambda argument: cls
(ARG005)
42-42: Unused lambda argument: x
(ARG005)
43-43: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
45-45: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
58-58: Unused method argument: payload
(ARG002)
83-83: Unused method argument: payload
(ARG002)
services/microservice_client.py
54-54: Consider moving this statement to an else block
(TRY300)
55-55: Do not catch blind exception: Exception
(BLE001)
59-59: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
82-82: Do not catch blind exception: Exception
(BLE001)
85-85: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
97-97: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
99-99: Do not catch blind exception: Exception
(BLE001)
100-100: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
103-103: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
119-119: Do not use bare except
(E722)
121-121: Do not catch blind exception: Exception
(BLE001)
122-122: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
125-125: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
141-141: Do not use bare except
(E722)
143-143: Do not catch blind exception: Exception
(BLE001)
144-144: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
163-163: Do not use bare except
(E722)
165-165: Do not catch blind exception: Exception
(BLE001)
166-166: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
test_new_tools.py
21-21: Possible hardcoded password assigned to: "token"
(S105)
⏰ 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: Build and Test
🔇 Additional comments (4)
requirements.txt (1)
80-80: PyJWT pin looks consistent with new JWT-based behaviorAdding
PyJWT==2.8.0matches the new JWT handling inmicroservice_client. Just ensure this pinned version aligns with your runtime environment and any security guidance you follow (e.g., same version used when generatingrequirements.txt).QUICK_REFERENCE.md (1)
42-45: k3s section clarification is clear and low-riskRenaming the section to explicitly call out k3s and noting “tested on k3s” makes the deployment context clearer without changing commands.
test_agent_core_invocation.py (1)
51-96: Tests cover both async and sync invocation paths wellNice pattern using
object.__new__to bypass heavy__init__, and then injectingagent_executor,ms_client, andrag_service. The two tests clearly validate theainvoke-preferred path and the syncrunfallback with normalization of both tuple and dict outputs.services/agent_core.py (1)
36-46: Prompt updates align with expanded tool surfaceThe additional capabilities and step-by-step guidance for appointments/projects match the richer toolset described elsewhere (vehicles, projects, profile, etc.), and should help steer the agent towards correct tool usage.
| async def get_my_projects_tool() -> str: | ||
| """ | ||
| Lists all custom projects for the user. | ||
| """ | ||
| token = token_context.get() | ||
| projects = await client.get_customer_projects(token) | ||
|
|
||
| if not projects: | ||
| return "You have no active projects." | ||
|
|
||
| summary = "Your Projects:\n" | ||
| for p in projects: | ||
| summary += f"- Project ID: {p.get('id')} - Status: {p.get('status')} - {p.get('description')[:50]}...\n" | ||
| return summary |
There was a problem hiding this comment.
Guard against missing or null project descriptions
get_my_projects_tool slices p.get('description')[:50] directly. If description is missing or None, this will raise a TypeError at runtime.
- summary = "Your Projects:\n"
- for p in projects:
- summary += f"- Project ID: {p.get('id')} - Status: {p.get('status')} - {p.get('description')[:50]}...\n"
+ summary = "Your Projects:\n"
+ for p in projects:
+ desc = p.get("description") or ""
+ summary += (
+ f"- Project ID: {p.get('id')} - Status: {p.get('status')} - {desc[:50]}...\n"
+ )This keeps behavior the same when descriptions are present, but avoids crashes when they’re absent or null.
📝 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.
| async def get_my_projects_tool() -> str: | |
| """ | |
| Lists all custom projects for the user. | |
| """ | |
| token = token_context.get() | |
| projects = await client.get_customer_projects(token) | |
| if not projects: | |
| return "You have no active projects." | |
| summary = "Your Projects:\n" | |
| for p in projects: | |
| summary += f"- Project ID: {p.get('id')} - Status: {p.get('status')} - {p.get('description')[:50]}...\n" | |
| return summary | |
| async def get_my_projects_tool() -> str: | |
| """ | |
| Lists all custom projects for the user. | |
| """ | |
| token = token_context.get() | |
| projects = await client.get_customer_projects(token) | |
| if not projects: | |
| return "You have no active projects." | |
| summary = "Your Projects:\n" | |
| for p in projects: | |
| desc = p.get("description") or "" | |
| summary += ( | |
| f"- Project ID: {p.get('id')} - Status: {p.get('status')} - {desc[:50]}...\n" | |
| ) | |
| return summary |
🤖 Prompt for AI Agents
In services/agent_tools.py around lines 150 to 163, the code slices
p.get('description')[:50] which will raise a TypeError if description is missing
or None; update the code to guard against null/missing descriptions by
defaulting to an empty string (e.g. use (p.get('description') or '')[:50] or
similar) before slicing so behavior is unchanged when present but safe when
absent, and ensure the displayed snippet still appends the ellipsis only when
appropriate.
| def _extract_user_from_token(self, token: str) -> Tuple[str, str]: | ||
| """ | ||
| Extract username and roles from JWT token. | ||
| Returns (username, roles_csv_string) | ||
| """ | ||
| try: | ||
| # Decode without verification (we trust our own tokens) | ||
| payload = jwt.decode(token, options={"verify_signature": False}) | ||
| username = payload.get("sub", "") | ||
|
|
||
| # Extract roles - they might be in different formats | ||
| roles = payload.get("roles", []) | ||
| if isinstance(roles, list): | ||
| # Remove ROLE_ prefix if present | ||
| cleaned_roles = [r.replace("ROLE_", "") for r in roles] | ||
| roles_str = ",".join(cleaned_roles) | ||
| elif isinstance(roles, str): | ||
| roles_str = roles.replace("ROLE_", "") | ||
| else: | ||
| roles_str = "" | ||
|
|
||
| logger.debug(f"Extracted from JWT - username: {username}, roles: {roles_str}") | ||
| return username, roles_str | ||
| except Exception as e: | ||
| logger.warning(f"Failed to extract user from token: {e}") | ||
| return "", "" |
There was a problem hiding this comment.
Be explicit about JWT trust assumptions and narrow exception handling
_extract_user_from_token decodes the JWT with verify_signature=False and catches a broad Exception. That’s fine only if:
- The token has already been fully verified elsewhere, and
- These extracted claims are used purely for convenience (e.g., X-User-* headers, logging), not as an additional trust boundary.
Two improvements:
- If tokens are not guaranteed to be pre-verified on all code paths, decode with verification enabled and a known key/algorithms.
- Narrow the except to JWT-specific errors:
from jwt import InvalidTokenError
try:
payload = jwt.decode(token, options={"verify_signature": False})
...
except InvalidTokenError as e:
logger.warning("Failed to extract user from token: %s", e)
return "", ""This avoids masking unrelated bugs while keeping failure handling predictable.
🧰 Tools
🪛 Ruff (0.14.5)
54-54: Consider moving this statement to an else block
(TRY300)
55-55: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In services/microservice_client.py around lines 32 to 57, the JWT decode is done
with verify_signature=False and a broad Exception is caught; update this to
either decode with verification (provide the expected key and algorithms) if
tokens might not be pre-verified, or clearly document that tokens are
pre-verified on all call paths, and replace the broad except with a JWT-specific
exception handler (e.g. import and catch InvalidTokenError from the jwt
library), logging the error and returning the empty tuple as before; ensure the
logging call uses structured logging (message with exception parameter) and keep
the existing claim extraction logic otherwise.
| async def _make_post_request(self, url: str, token: str, data: Dict[str, Any] = None) -> Dict[str, Any]: | ||
| """Internal helper for making async authenticated POST requests.""" | ||
| headers = {"Authorization": f"Bearer {token}"} | ||
|
|
||
| # Add X-User headers for direct service calls | ||
| username, roles = self._extract_user_from_token(token) | ||
| if username: | ||
| headers["X-User-Subject"] = username | ||
| headers["X-User-Roles"] = roles | ||
|
|
||
| try: | ||
| response = await self._async_client.post(url, json=data, headers=headers) | ||
| if response.is_success: | ||
| return response.json() | ||
| try: | ||
| return response.json() | ||
| except: | ||
| return {"error": f"HTTP Error {response.status_code}", "status_code": response.status_code} | ||
| except Exception as e: | ||
| logger.error(f"POST Error to {url}: {e}") | ||
| return {"error": str(e), "status_code": 500} | ||
|
|
||
| async def _make_put_request(self, url: str, token: str, data: Dict[str, Any] = None) -> Dict[str, Any]: | ||
| """Internal helper for making async authenticated PUT requests.""" | ||
| headers = {"Authorization": f"Bearer {token}"} | ||
|
|
||
| # Add X-User headers for direct service calls | ||
| username, roles = self._extract_user_from_token(token) | ||
| if username: | ||
| headers["X-User-Subject"] = username | ||
| headers["X-User-Roles"] = roles | ||
|
|
||
| try: | ||
| response = await self._async_client.put(url, json=data, headers=headers) | ||
| if response.is_success: | ||
| return response.json() | ||
| try: | ||
| return response.json() | ||
| except: | ||
| return {"error": f"HTTP Error {response.status_code}", "status_code": response.status_code} | ||
| except Exception as e: | ||
| logger.error(f"PUT Error to {url}: {e}") | ||
| return {"error": str(e), "status_code": 500} | ||
|
|
||
| async def _make_delete_request(self, url: str, token: str) -> Dict[str, Any]: | ||
| """Internal helper for making async authenticated DELETE requests.""" | ||
| headers = {"Authorization": f"Bearer {token}"} | ||
|
|
||
| # Add X-User headers for direct service calls | ||
| username, roles = self._extract_user_from_token(token) | ||
| if username: | ||
| headers["X-User-Subject"] = username | ||
| headers["X-User-Roles"] = roles | ||
|
|
||
| try: | ||
| response = await self._async_client.delete(url, headers=headers) | ||
| if response.is_success: | ||
| return {"success": True, "status_code": response.status_code} | ||
| try: | ||
| return response.json() | ||
| except: | ||
| return {"error": f"HTTP Error {response.status_code}", "status_code": response.status_code} | ||
| except Exception as e: | ||
| logger.error(f"DELETE Error to {url}: {e}") | ||
| return {"error": str(e), "status_code": 500} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Normalize error handling for POST/PUT/DELETE to always expose an error on non-2xx
_make_post_request, _make_put_request, and _make_delete_request return raw response.json() bodies for non-2xx statuses. Callers like the agent tools rely on result.get("error") to detect failures, so APIs that return e.g. {"message": "..."}
on errors will be misinterpreted as success.
Consider aligning these with _make_get_request so that any non-2xx response yields a consistent error shape, e.g.:
- try:
- response = await self._async_client.post(url, json=data, headers=headers)
- if response.is_success:
- return response.json()
- try:
- return response.json()
- except:
- return {"error": f"HTTP Error {response.status_code}", "status_code": response.status_code}
- except Exception as e:
- logger.error(f"POST Error to {url}: {e}")
- return {"error": str(e), "status_code": 500}
+ try:
+ response = await self._async_client.post(url, json=data, headers=headers)
+ if response.is_success:
+ return response.json()
+ try:
+ body = response.json()
+ except Exception:
+ body = None
+ result: dict[str, Any] = {"status_code": response.status_code}
+ if isinstance(body, dict):
+ result.update(body)
+ else:
+ result["error"] = body or f"HTTP Error {response.status_code}"
+ return result
+ except Exception as e:
+ logger.exception(f"POST Error to {url}: {e}")
+ return {"error": str(e), "status_code": 500}and similarly for _make_put_request and _make_delete_request.
This guarantees callers always see an "error" (or at least a status_code) on failure and reduces the chance of tools reporting success when the microservice returned an error payload.
📝 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.
| async def _make_post_request(self, url: str, token: str, data: Dict[str, Any] = None) -> Dict[str, Any]: | |
| """Internal helper for making async authenticated POST requests.""" | |
| headers = {"Authorization": f"Bearer {token}"} | |
| # Add X-User headers for direct service calls | |
| username, roles = self._extract_user_from_token(token) | |
| if username: | |
| headers["X-User-Subject"] = username | |
| headers["X-User-Roles"] = roles | |
| try: | |
| response = await self._async_client.post(url, json=data, headers=headers) | |
| if response.is_success: | |
| return response.json() | |
| try: | |
| return response.json() | |
| except: | |
| return {"error": f"HTTP Error {response.status_code}", "status_code": response.status_code} | |
| except Exception as e: | |
| logger.error(f"POST Error to {url}: {e}") | |
| return {"error": str(e), "status_code": 500} | |
| async def _make_put_request(self, url: str, token: str, data: Dict[str, Any] = None) -> Dict[str, Any]: | |
| """Internal helper for making async authenticated PUT requests.""" | |
| headers = {"Authorization": f"Bearer {token}"} | |
| # Add X-User headers for direct service calls | |
| username, roles = self._extract_user_from_token(token) | |
| if username: | |
| headers["X-User-Subject"] = username | |
| headers["X-User-Roles"] = roles | |
| try: | |
| response = await self._async_client.put(url, json=data, headers=headers) | |
| if response.is_success: | |
| return response.json() | |
| try: | |
| return response.json() | |
| except: | |
| return {"error": f"HTTP Error {response.status_code}", "status_code": response.status_code} | |
| except Exception as e: | |
| logger.error(f"PUT Error to {url}: {e}") | |
| return {"error": str(e), "status_code": 500} | |
| async def _make_delete_request(self, url: str, token: str) -> Dict[str, Any]: | |
| """Internal helper for making async authenticated DELETE requests.""" | |
| headers = {"Authorization": f"Bearer {token}"} | |
| # Add X-User headers for direct service calls | |
| username, roles = self._extract_user_from_token(token) | |
| if username: | |
| headers["X-User-Subject"] = username | |
| headers["X-User-Roles"] = roles | |
| try: | |
| response = await self._async_client.delete(url, headers=headers) | |
| if response.is_success: | |
| return {"success": True, "status_code": response.status_code} | |
| try: | |
| return response.json() | |
| except: | |
| return {"error": f"HTTP Error {response.status_code}", "status_code": response.status_code} | |
| except Exception as e: | |
| logger.error(f"DELETE Error to {url}: {e}") | |
| return {"error": str(e), "status_code": 500} | |
| async def _make_post_request(self, url: str, token: str, data: Dict[str, Any] = None) -> Dict[str, Any]: | |
| """Internal helper for making async authenticated POST requests.""" | |
| headers = {"Authorization": f"Bearer {token}"} | |
| # Add X-User headers for direct service calls | |
| username, roles = self._extract_user_from_token(token) | |
| if username: | |
| headers["X-User-Subject"] = username | |
| headers["X-User-Roles"] = roles | |
| try: | |
| response = await self._async_client.post(url, json=data, headers=headers) | |
| if response.is_success: | |
| return response.json() | |
| try: | |
| body = response.json() | |
| except Exception: | |
| body = None | |
| result: dict[str, Any] = {"status_code": response.status_code} | |
| if isinstance(body, dict): | |
| result.update(body) | |
| else: | |
| result["error"] = body or f"HTTP Error {response.status_code}" | |
| return result | |
| except Exception as e: | |
| logger.exception(f"POST Error to {url}: {e}") | |
| return {"error": str(e), "status_code": 500} | |
| async def _make_put_request(self, url: str, token: str, data: Dict[str, Any] = None) -> Dict[str, Any]: | |
| """Internal helper for making async authenticated PUT requests.""" | |
| headers = {"Authorization": f"Bearer {token}"} | |
| # Add X-User headers for direct service calls | |
| username, roles = self._extract_user_from_token(token) | |
| if username: | |
| headers["X-User-Subject"] = username | |
| headers["X-User-Roles"] = roles | |
| try: | |
| response = await self._async_client.put(url, json=data, headers=headers) | |
| if response.is_success: | |
| return response.json() | |
| try: | |
| body = response.json() | |
| except Exception: | |
| body = None | |
| result: dict[str, Any] = {"status_code": response.status_code} | |
| if isinstance(body, dict): | |
| result.update(body) | |
| else: | |
| result["error"] = body or f"HTTP Error {response.status_code}" | |
| return result | |
| except Exception as e: | |
| logger.exception(f"PUT Error to {url}: {e}") | |
| return {"error": str(e), "status_code": 500} | |
| async def _make_delete_request(self, url: str, token: str) -> Dict[str, Any]: | |
| """Internal helper for making async authenticated DELETE requests.""" | |
| headers = {"Authorization": f"Bearer {token}"} | |
| # Add X-User headers for direct service calls | |
| username, roles = self._extract_user_from_token(token) | |
| if username: | |
| headers["X-User-Subject"] = username | |
| headers["X-User-Roles"] = roles | |
| try: | |
| response = await self._async_client.delete(url, headers=headers) | |
| if response.is_success: | |
| return {"success": True, "status_code": response.status_code} | |
| try: | |
| body = response.json() | |
| except Exception: | |
| body = None | |
| result: dict[str, Any] = {"status_code": response.status_code} | |
| if isinstance(body, dict): | |
| result.update(body) | |
| else: | |
| result["error"] = body or f"HTTP Error {response.status_code}" | |
| return result | |
| except Exception as e: | |
| logger.exception(f"DELETE Error to {url}: {e}") | |
| return {"error": str(e), "status_code": 500} |
🧰 Tools
🪛 Ruff (0.14.5)
103-103: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
119-119: Do not use bare except
(E722)
121-121: Do not catch blind exception: Exception
(BLE001)
122-122: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
125-125: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
141-141: Do not use bare except
(E722)
143-143: Do not catch blind exception: Exception
(BLE001)
144-144: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
163-163: Do not use bare except
(E722)
165-165: Do not catch blind exception: Exception
(BLE001)
166-166: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In services/microservice_client.py around lines 103 to 168, the POST/PUT/DELETE
helpers currently return raw response.json() for non-2xx responses which can
hide errors (callers expect an "error" key); change them to match
_make_get_request by: after awaiting response, if response.is_success return the
parsed JSON (for DELETE keep {"success": True, "status_code": ...}), otherwise
attempt to parse response.json() and map any message/body into a normalized
{"error": <message or body>, "status_code": response.status_code} shape (falling
back to {"error": f"HTTP Error {status_code}", "status_code": status_code} if
parsing fails), and keep the existing exception logging/500 return for network
errors.
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.