vet-clinic-ai-agent-with-secure-mcp #22
Conversation
|
|
|
Warning Rate limit exceeded@Bin4yi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a secured Pet Care AI system: Asgardeo PKCE authentication, JWKS-backed JWT validation, an authenticated FastMCP service with pet-care tools (including OpenAI name suggestions), a LangGraph-based agent integrating MCP tools and multi-pet context, plus Streamlit and standalone HTML chat frontends and comprehensive README. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / Frontend
participant Agent as Agent<br/>(agent.py)
participant MCP as MCP Server<br/>(main.py)
participant Asgardeo as Asgardeo<br/>OAuth2
participant LLM as OpenAI<br/>LLM
User->>Agent: Launch interactive chat
Agent->>Agent: Generate PKCE challenge
Agent->>Asgardeo: Initiate OAuth2 (PKCE)
Asgardeo->>Agent: Authorization code
Agent->>Asgardeo: Exchange code for tokens
Asgardeo->>Agent: Access token + ID token
Agent->>Agent: Extract user_email (ID token / SCIM2 fallback)
Agent->>MCP: Establish MCP session
MCP->>MCP: Load JWT validator with JWKS
Agent->>MCP: Load available tools
MCP->>Agent: Return tool list
Agent->>Agent: Compile LangGraph with nodes
rect rgb(220, 235, 255)
Note over User,LLM: Chat Flow - Per User Message
User->>Agent: Send chat message
Agent->>Agent: Build initial state (user_id, pets, history)
Agent->>Agent: Route to load_context_node
Agent->>MCP: Call get_user_id_by_email(user_email)
MCP->>MCP: Validate JWT, lookup user
MCP->>Agent: Return user_id
Agent->>MCP: Call get_pets_by_user_id(user_id)
MCP->>Agent: Return pets list
Agent->>Agent: Route to classify_node
Agent->>LLM: Classify user intent
LLM->>Agent: Return intent category
alt Intent requires MCP action
Agent->>Agent: Build context-aware prompt with pet info
Agent->>LLM: Invoke agent with tools
LLM->>Agent: Decide tool calls
Agent->>MCP: Call tool(s) with JWT
MCP->>Agent: Return tool results
Agent->>LLM: Provide results to LLM
LLM->>Agent: Return final response
else Simple response
Agent->>LLM: Generate predefined response
LLM->>Agent: Return response
end
Agent->>Agent: Update session cache
Agent->>User: Return bot response
end
sequenceDiagram
participant Client as HTTP Client
participant Frontend as Frontend<br/>(frontend_app.py / index.html)
participant Agent as Agent<br/>HTTP Server
participant MCP as MCP Server<br/>(JWT validated)
Client->>Frontend: Open chat UI
Frontend->>Frontend: Check session auth
alt Not authenticated
Frontend->>Frontend: Show Asgardeo login (PKCE)
Frontend->>Frontend: Store access_token
end
Client->>Frontend: Send message
Frontend->>Agent: POST /chat { message }
Agent->>Agent: Extract user state (cache or rebuild)
Agent->>MCP: Validate JWT from agent session
Agent->>MCP: Call tool endpoints as needed
MCP->>Agent: Tool results
Agent->>LLM: Compose final response
Agent->>Frontend: Return JSON response
Frontend->>Client: Render message + response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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: 5
🧹 Nitpick comments (13)
Pet_Care_ai_agent_secured_with_asgardeo/jwt_validator.py (1)
48-58: Tighten exception handling and preserve original error contextTwo improvements would make diagnostics and behavior clearer:
Avoid broad
except Exceptionin_fetch_jwksandvalidate_token
- In
_fetch_jwks, catchingExceptionand re‑raising adds little value; or at least narrow tohttpx.HTTPError(or similar) and let unexpected errors bubble up.- In
validate_token, the finalexcept Exception as ewill also catch infrastructure issues (network, JWKS parsing) and re‑label them asValueError("Token validation failed: ..."), making it hard for callers to distinguish “bad token” vs “auth infra unavailable”.Preserve original exceptions via chaining and richer logging
- For the specific JWT error branches, consider chaining:
except jwt.ExpiredSignatureError as e: raise ValueError("Token has expired") from e- For the generic branch,
logger.exception("Token validation error")will record the stack trace, and you can either re‑raise the original error or use a custom exception type so callers can treat infra failures differently from invalid tokens.These changes keep behavior mostly the same for callers while giving you clearer observability and debuggability.
Also applies to: 124-137
Pet_Care_ai_agent_secured_with_asgardeo/agent/frontend_app.py (1)
13-17: Auth gating and chat flow look solid; consider wrapping backend errors for UXThe Streamlit state handling and auth checks (forcing login before sending, clearing state on logout, storing history in
st.session_state.messages) are consistent and easy to follow.One optional improvement: if
perform_login()orrun_chat_turn()raise (network issues, unexpected backend error), users will see a raw Streamlit exception. You could wrap these calls intry/exceptand surface a friendlyst.error("Something went wrong talking to the agent. Please try again.")while logging the traceback for debugging.Also applies to: 24-40, 55-87
Pet_Care_ai_agent_secured_with_asgardeo/requirements.txt (1)
1-10: ---Pin AI dependencies to stable LTS versions to ensure reproducible deployments
openai,langgraph,langchain,langchain-openai, andlangchain-coreare unpinned while others are version-constrained. These libraries release frequently with breaking changes and inter-package incompatibilities; leaving them unpinned risks silent breakage on deployment.Recommended production set (mutually compatible, stable 1.0 LTS line):
langchain~=1.0.7langchain-core~=1.0.5langchain-openai~=1.0.3langgraph~=1.0.3Verify the latest 1.0.* patch releases against PyPI and your integration tests before committing.
Pet_Care_ai_agent_secured_with_asgardeo/main.py (5)
98-135: JWTTokenVerifier looks good; consider tightening genericExceptioncatchThe mapping from validated JWT payload to
AccessTokenis reasonable and logs failures. The finalexcept Exceptionwill hide unexpected bugs (e.g., programming errors) by returningNone.If you want stricter behavior, consider only catching
ValueError(fromJWTValidator) and letting other exceptions surface, or at least logging withlogger.exceptionto preserve the traceback.
161-196: Email lookup logic fine; reconsider MD5 for synthetic user IDs and broadExceptionThe email normalization and lookup logic are solid, and the synthetic ID fallback is helpful for unknown users. Two minor points:
hashlib.md5is flagged as insecure; even though this is only for generating a non-secret user ID, considerhashlib.sha256for better defaults.- The broad
except Exceptionwraps all errors into aValueError, losing the original stack trace. Narrowing this to expected errors (or usinglogger.exceptionandraise ValueError(...) from error) would aid debugging.Example tweak:
- import hashlib - hash_object = hashlib.md5(email_clean.encode()) + import hashlib + hash_object = hashlib.sha256(email_clean.encode()) @@ - except Exception as error: - logger.error(f"System error looking up user ID: {error}") - raise ValueError(f"Failed to retrieve user ID: {str(error)}") + except Exception as error: + logger.exception("System error looking up user ID") + raise ValueError("Failed to retrieve user ID") from errorPlease ensure any hash change doesn’t break existing synthetic IDs you might persist elsewhere.
268-334: Consider validatingpet_id(and optionallytime) when booking appointmentsThe booking flow (date validation, synthetic
appointment_id, rich confirmation payload, and logging) is good for a sample.Two potential improvements:
- Validate that
pet_idexists inPETS_DBbefore confirming the booking, so you don’t create appointments for non-existent pets.- Optionally validate the
timestring (e.g., using a fixed format likeHH:MMor a parsing helper) if you expect a particular format.Example addition:
- try: - # Validate date format + try: + # Validate date format try: datetime.strptime(date, "%Y-%m-%d") except ValueError: raise ValueError("Invalid date format. Please use YYYY-MM-DD format.") + + # Optional: ensure pet exists + if pet_id not in PETS_DB: + raise ValueError(f"Unknown pet_id '{pet_id}'")
335-359: Typo in tool name (cancel_appinment) and internal variableFunctionality and payload look fine, but:
- The tool name is misspelled (
cancel_appinmentinstead ofcancel_appointment), which leaks into the MCP tool surface and may confuse consumers.- Local variable
cancelllation_detailshas an extral(only internal, but distracting).If you can still safely change the API, consider:
-@mcp.tool() -async def cancel_appinment(appointment_id: str, reason: str) -> dict[str, Any]: +@mcp.tool() +async def cancel_appointment(appointment_id: str, reason: str) -> dict[str, Any]: @@ - cancelllation_details = { + cancellation_details = { @@ - return cancelllation_details + return cancellation_details
361-501: Simplify OpenAI usage and make JSON parsing a bit more robustThe overall flow (validation of inputs, prompt construction, JSON-only response contract, cleanup of code-block fences, detailed error handling) is solid. A few targeted refinements:
- You’re maintaining both a global
openai_clientandopenai.api_key. Since all calls go throughopenai_client.chat.completions.create, you can drop theopenai.api_keylogic and just requireopenai_clientto be non-None.- When JSON parsing fails you already log the full raw response; consider
logger.exceptionto preserve tracebacks.- In the
RateLimitErrorandAuthenticationErrorbranches the caught exception variableeis unused; you can drop theas e.Sketch:
- try: - # Ensure OpenAI API key is set from environment if available - if OPENAI_API_KEY and not openai.api_key: - openai.api_key = OPENAI_API_KEY - - if not openai.api_key: - raise ValueError("OpenAI API key is not configured. Please set OPENAI_API_KEY in environment variables.") + try: + if not openai_client: + raise ValueError("OpenAI API key not configured. Please set OPENAI_API_KEY environment variable.") @@ - if not openai_client: - raise ValueError("OpenAI API key not configured. Please set OPENAI_API_KEY environment variable.") + # openai_client guaranteed non-None here @@ - except openai.RateLimitError as e: + except openai.RateLimitError: @@ - except openai.AuthenticationError as e: + except openai.AuthenticationError:And for JSON parsing:
- except json.JSONDecodeError: - logger.error(f"Failed to parse OpenAI response as JSON: {ai_response}") + except json.JSONDecodeError: + logger.exception("Failed to parse OpenAI response as JSON") raise ValueError("Failed to parse AI response. Please try again.")Please confirm this matches the
openaiSDK version you have pinned (client construction & exception classes).Pet_Care_ai_agent_secured_with_asgardeo/agent/agent.py (5)
74-135: PKCE flow and callback server are reasonable; tighten timeout error type if needed
generate_pkceandstart_callback_serverare implemented correctly, and the async auth flow withasyncio.wait_foris clear.The timeout path currently raises a generic
Exception("Authentication timed out."). If you plan to surface this to callers/UI distinctly, consider defining a small custom exception (e.g.,AuthTimeoutError) so upstream code can distinguish timeouts from other failures.
213-287: Context loader works; consider improving JSON parsing and exception granularityThe logic to resolve
user_idandpetsvia MCP tools is good and nicely separated into two steps. Two small things:
- The JSON parsing uses
try/except: pass, which silently ignores parsing errors and leavesdata = {}. This makes debugging harder if the MCP tool output changes format.- Both steps wrap everything in a broad
except Exceptionand print a message; you might want to at least log the stacktrace.Example tweak for Step 1 (similarly for Step 2):
- if isinstance(result, str): - try: - clean = result.replace('```json', '').replace('```', '').strip() - data = json.loads(clean) - except: pass + if isinstance(result, str): + try: + clean = result.replace('```json', '').replace('```', '').strip() + data = json.loads(clean) + except json.JSONDecodeError as e: + print(f" ❌ Failed to parse get_user_id_by_email result as JSON: {e}")
410-426: Intent routing and simple nodes are fine for this use caseThe greeting, services, pricing, and general nodes are minimal but functional, and
route_intentcleanly dispatches based on the classifier output.Only note:
stateis unused ingreeting_node,service_node, andpricing_node, which is fine but will trigger linters; keep as-is unless you want to evolve them later.
451-510: Chat handler is coherent; consider more structured error loggingThe flow — parse request, pull graph & tools from
app, restore history/context, run the graph, update caches, and return the last AI message — is well-structured.Minor suggestions:
- In the
except Exceptionblock, you currentlylogging.exception(or at least logging alongside the JSON error response) would help observability.- If
final_state["messages"]ever diverges fromhistory + new_messages, the slicefinal_state["messages"][len(history):]could become brittle, but given how nodes are implemented today this looks safe.Example:
- except Exception as e: - print(f"Error: {e}") - import traceback - traceback.print_exc() - return web.json_response({"error": str(e)}, status=500) + except Exception as e: + import logging, traceback + logging.error("Error handling /chat request: %s", e) + traceback.print_exc() + return web.json_response({"error": str(e)}, status=500)
511-553: Auth fallback to mock token is convenient; guard it for non-demo environmentsThe orchestration of auth, MCP connection, tool loading, graph compilation, and aiohttp server startup is nicely done.
One caution: on auth failure you fall back to a hardcoded
access_token = "mock_token"and fixed email. That’s great for local demos but risky if this code is ever reused in a less controlled environment.Consider gating this behavior behind an explicit “demo mode” flag or environment variable so production deployments fail fast instead of silently using a mock token.
- try: - access_token, user_email = await authenticate_with_asgardeo() - except Exception as e: - print(f"Auth failed (using mock): {e}") - access_token = "mock_token" - user_email = "[email protected]" + try: + access_token, user_email = await authenticate_with_asgardeo() + except Exception as e: + print(f"Auth failed: {e}") + if os.getenv("DEMO_MODE", "false").lower() == "true": + print("⚠️ DEMO_MODE enabled: falling back to mock token.") + access_token = "mock_token" + user_email = "[email protected]" + else: + raiseIf you expect to run this against real Asgardeo tenants, please confirm whether a mock-token fallback is acceptable operationally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Pet_Care_ai_agent_secured_with_asgardeo/images/mcp-auth-configs.pngis excluded by!**/*.pngPet_Care_ai_agent_secured_with_asgardeo/images/mcp-client-app-selection.pngis excluded by!**/*.pngPet_Care_ai_agent_secured_with_asgardeo/images/mcp-client-creation.pngis excluded by!**/*.pngPet_Care_ai_agent_secured_with_asgardeo/images/mcp-server-connect.pngis excluded by!**/*.png
📒 Files selected for processing (7)
Pet_Care_ai_agent_secured_with_asgardeo/README.md(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/agent/agent.py(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/agent/frontend_app.py(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/agent/index.html(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/jwt_validator.py(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/main.py(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/requirements.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Pet_Care_ai_agent_secured_with_asgardeo/main.py (1)
Pet_Care_ai_agent_secured_with_asgardeo/jwt_validator.py (2)
JWTValidator(26-136)validate_token(78-136)
🪛 Gitleaks (8.29.0)
Pet_Care_ai_agent_secured_with_asgardeo/README.md
[high] 81-81: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
Pet_Care_ai_agent_secured_with_asgardeo/README.md
24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
227-227: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
252-252: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.5)
Pet_Care_ai_agent_secured_with_asgardeo/jwt_validator.py
56-56: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
69-69: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
122-122: Consider moving this statement to an else block
(TRY300)
125-125: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
127-127: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
127-127: Avoid specifying long messages outside the exception class
(TRY003)
129-129: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
129-129: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
131-131: Avoid specifying long messages outside the exception class
(TRY003)
133-133: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
133-133: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Do not catch blind exception: Exception
(BLE001)
135-135: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
136-136: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
136-136: Avoid specifying long messages outside the exception class
(TRY003)
Pet_Care_ai_agent_secured_with_asgardeo/agent/agent.py
121-121: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
121-121: Create your own exception
(TRY002)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
152-152: Do not catch blind exception: Exception
(BLE001)
200-200: Do not catch blind exception: Exception
(BLE001)
238-238: Multiple statements on one line (colon)
(E701)
243-243: Do not use bare except
(E722)
243-243: try-except-pass detected, consider logging the exception
(S110)
243-243: Multiple statements on one line (colon)
(E701)
249-249: Do not catch blind exception: Exception
(BLE001)
265-265: Multiple statements on one line (colon)
(E701)
270-270: Do not use bare except
(E722)
270-270: try-except-pass detected, consider logging the exception
(S110)
270-270: Multiple statements on one line (colon)
(E701)
283-283: Do not catch blind exception: Exception
(BLE001)
399-399: Do not catch blind exception: Exception
(BLE001)
402-402: Consider [*messages_with_context, response, *tool_outputs] instead of concatenation
Replace with [*messages_with_context, response, *tool_outputs]
(RUF005)
405-405: Consider [response, *tool_outputs, final_response] instead of concatenation
Replace with [response, *tool_outputs, final_response]
(RUF005)
410-410: Unused function argument: state
(ARG001)
412-412: Unused function argument: state
(ARG001)
414-414: Unused function argument: state
(ARG001)
421-421: Multiple statements on one line (colon)
(E701)
422-422: Multiple statements on one line (colon)
(E701)
423-423: Multiple statements on one line (colon)
(E701)
424-424: Multiple statements on one line (colon)
(E701)
427-427: Unused function argument: mcp_tools
(ARG001)
469-469: Consider [*history, HumanMessage(content=user_input)] instead of concatenation
Replace with [*history, HumanMessage(content=user_input)]
(RUF005)
505-505: Do not catch blind exception: Exception
(BLE001)
518-518: Do not catch blind exception: Exception
(BLE001)
520-520: Possible hardcoded password assigned to: "access_token"
(S105)
538-538: Unused lambda argument: r
(ARG005)
544-544: Multiple statements on one line (colon)
(E701)
Pet_Care_ai_agent_secured_with_asgardeo/main.py
132-132: Do not catch blind exception: Exception
(BLE001)
133-133: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
166-166: Abstract raise to an inner function
(TRY301)
166-166: Avoid specifying long messages outside the exception class
(TRY003)
184-184: Probable use of insecure hash functions in hashlib: md5
(S324)
193-193: Do not catch blind exception: Exception
(BLE001)
194-194: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
195-195: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
195-195: Avoid specifying long messages outside the exception class
(TRY003)
195-195: Use explicit conversion flag
Replace with conversion flag
(RUF010)
214-214: Abstract raise to an inner function
(TRY301)
214-214: Avoid specifying long messages outside the exception class
(TRY003)
233-233: Do not catch blind exception: Exception
(BLE001)
235-235: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
237-237: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
237-237: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Use explicit conversion flag
Replace with conversion flag
(RUF010)
246-246: Abstract raise to an inner function
(TRY301)
246-246: Avoid specifying long messages outside the exception class
(TRY003)
261-261: Consider moving this statement to an else block
(TRY300)
263-263: Do not catch blind exception: Exception
(BLE001)
264-264: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
265-265: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
265-265: Avoid specifying long messages outside the exception class
(TRY003)
265-265: Use explicit conversion flag
Replace with conversion flag
(RUF010)
293-293: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
293-293: Avoid specifying long messages outside the exception class
(TRY003)
324-324: Consider moving this statement to an else block
(TRY300)
328-328: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
329-329: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
330-330: Do not catch blind exception: Exception
(BLE001)
332-332: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
333-333: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
333-333: Avoid specifying long messages outside the exception class
(TRY003)
355-355: Consider moving this statement to an else block
(TRY300)
356-356: Do not catch blind exception: Exception
(BLE001)
358-358: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
359-359: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
359-359: Avoid specifying long messages outside the exception class
(TRY003)
385-385: Abstract raise to an inner function
(TRY301)
385-385: Avoid specifying long messages outside the exception class
(TRY003)
393-393: Abstract raise to an inner function
(TRY301)
393-393: Avoid specifying long messages outside the exception class
(TRY003)
397-397: Abstract raise to an inner function
(TRY301)
397-397: Avoid specifying long messages outside the exception class
(TRY003)
427-427: Abstract raise to an inner function
(TRY301)
427-427: Avoid specifying long messages outside the exception class
(TRY003)
466-466: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
467-467: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
467-467: Avoid specifying long messages outside the exception class
(TRY003)
481-481: Consider moving this statement to an else block
(TRY300)
484-484: Use explicit conversion flag
Replace with conversion flag
(RUF010)
485-485: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
486-486: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
486-486: Avoid specifying long messages outside the exception class
(TRY003)
487-487: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
489-489: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
490-490: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
491-491: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
493-493: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
494-494: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
496-496: Use raise without specifying exception name
Remove exception name
(TRY201)
497-497: Do not catch blind exception: Exception
(BLE001)
499-499: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
500-500: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
500-500: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
Pet_Care_ai_agent_secured_with_asgardeo/main.py (3)
197-238: Pet lookup by user ID is correct and straightforwardThe join through
USERS_DBto resolveowner_emailand then filterPETS_DBis clear and defensively handles unknown user IDs.No functional issues spotted here.
240-266: Vaccination info tool is consistent with in-memory schemaLookup against
PETS_DBandVACCINATIONS_DBplus the merged response structure (history, upcoming, timestamp) all look consistent. Error wrapping intoValueErroris aligned with other tools.No changes needed from my side.
145-159: The original code is correct; disregard this review comment.The review's claim that
AnyHttpUrlis "not callable" and will raise aTypeErroris incorrect. In Pydantic v2, URL types like AnyHttpUrl are callable constructors that validate input. Furthermore, FastMCP's AuthSettings documentation examples show using AnyHttpUrl for issuer_url and resource_server_url fields, which matches your code exactly.Your current implementation is valid and consistent with FastMCP's documented patterns. No changes are needed.
Likely an incorrect or invalid review comment.
Pet_Care_ai_agent_secured_with_asgardeo/agent/agent.py (1)
427-447: Graph wiring matches the intended flowThe graph correctly sets
load_contextas the entry, runs classification, then conditionally routes to the appropriate node, all terminating inEND.mcp_toolsaren’t used directly here, which is fine since tools are consumed inside nodes.No issues from my side.
|
Let's also add the WSO2 open source code license header to all files. |
Bin4yi
changed the title
Co-authored-by: Pavindu Lakshan <[email protected]>
Co-authored-by: Pavindu Lakshan <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Pet_Care_ai_agent_secured_with_asgardeo/README.md (1)
72-81: Replace realistic API key placeholder with clearly fake placeholder.The example contains
OPENAI_API_KEY=sk-proj-abc123xyz789which matches the pattern of real OpenAI keys and triggers secret scanning tools (Gitleaks). Even as a dummy value, this should be replaced with an obvious placeholder.Additionally, as suggested in a prior review, these examples should be moved to a separate
.env.examplefile rather than included in the README.Apply this diff to replace the realistic key:
# OpenAI Configuration - OPENAI_API_KEY=sk-proj-abc123xyz789 + OPENAI_API_KEY=<your-openai-api-key>Recommended further step: Create a
.env.examplefile in the project root with all template values, update the README to instruct users to "copy.env.exampleto.envand update values," and mention that.envshould be in.gitignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Pet_Care_ai_agent_secured_with_asgardeo/README.md(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.29.0)
Pet_Care_ai_agent_secured_with_asgardeo/README.md
[high] 80-80: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
Pet_Care_ai_agent_secured_with_asgardeo/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
226-226: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
239-239: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
251-251: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Co-authored-by: Pavindu Lakshan <[email protected]>
Refactored code to remove mock data stores and integrate database functions for user, pet, and vaccination record management.
Added database helper functions and initialization script to README.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Pet_Care_ai_agent_secured_with_asgardeo/main.py (1)
1-23: Add the standard WSO2 open source license header to this module.This file currently has no license header; please add the WSO2 OSS license block consistently with the rest of the repo before the imports.
Pet_Care_ai_agent_secured_with_asgardeo/agent/agent.py (3)
1-34: Add the standard WSO2 open source license header here as well.This agent entrypoint also lacks the WSO2 OSS license header; please add it above the imports to align with project licensing.
140-209: Remove or redact full SCIM/Meresponse logging and reconsider ID token decoding without verification.
print(f"🔍 RAW SCIM DATA: {json.dumps(scim_data)}")logs the entire SCIM/Mepayload (including user PII) to stdout, which is not acceptable for anything beyond a local demo. In addition,jwt.decode(id_token, options={"verify_signature": False})skips signature and claim validation.Recommended changes:
- if id_token: - try: - decoded = jwt.decode(id_token, options={"verify_signature": False}) + if id_token: + try: + # For production, validate the ID token using your JWKS-based validator. + decoded = jwt.decode(id_token, options={"verify_signature": False}) @@ - scim_data = scim_resp.json() - - # DEBUG: Print the actual structure so we can see it - print(f"🔍 RAW SCIM DATA: {json.dumps(scim_data)}") + scim_data = scim_resp.json() + # Optional: log only minimal, non-PII metadata in debug mode + # logger.debug("SCIM /Me keys: %s", list(scim_data.keys()))Please confirm against Asgardeo’s logging/PII guidance for your deployment environment.
300-365: Handle staleactive_pet_idsafely when multiple pets exist.In Scenario C you assume
active_petis notNone; ifactive_pet_idis cached but that pet was removed,active_pet['pet_name']will raise aTypeErrorand break the request.Suggested fix (fall back to the “multiple pets, none selected” branch and clear the stale ID):
- # SCENARIO C: Multiple Pets, BUT one is Selected (Active) - elif len(pets) > 1 and active_pet_id: - # Find the name of the active pet - active_pet = next((p for p in pets if p['pet_id'] == active_pet_id), None) - pet_info = f"User has {len(pets)} pets. CURRENT CONTEXT: {active_pet['pet_name']} (ID: {active_pet_id})." - specific_instruction = f"Focus ONLY on {active_pet['pet_name']} (ID: {active_pet['pet_id']}) for this response." + # SCENARIO C: Multiple Pets, BUT one is Selected (Active) + elif len(pets) > 1 and active_pet_id: + active_pet = next((p for p in pets if p["pet_id"] == active_pet_id), None) + if active_pet: + pet_info = ( + f"User has {len(pets)} pets. CURRENT CONTEXT: " + f"{active_pet['pet_name']} (ID: {active_pet_id})." + ) + specific_instruction = ( + f"Focus ONLY on {active_pet['pet_name']} " + f"(ID: {active_pet['pet_id']}) for this response." + ) + else: + # Stale ID → treat as ambiguity + names = [f"{p['pet_name']} (ID: {p['pet_id']})" for p in pets] + pet_info = f"User has {len(pets)} pets: " + ", ".join(names) + specific_instruction = ( + "The user has multiple pets and the previously selected pet is no longer available.\n" + "Ask which pet they are referring to before calling tools." + ) + active_pet_id = None
🧹 Nitpick comments (7)
Pet_Care_ai_agent_secured_with_asgardeo/main.py (4)
86-121: Avoid MD5 for synthetic user IDs; use SHA‑256 instead.The fallback branch uses
hashlib.md5to derive a syntheticuser_idfrom the email, which triggers security tooling (S324) even though it’s only used as an opaque ID. You can get the same behavior with a stronger hash and keep scanners happy.- import hashlib - hash_object = hashlib.md5(email_clean.encode()) - user_id_suffix = hash_object.hexdigest()[:8].upper() + import hashlib + hash_object = hashlib.sha256(email_clean.encode("utf-8")) + user_id_suffix = hash_object.hexdigest()[:8].upper()
118-187: Harden exception handling and logging across MCP tool functions.Most tool handlers catch a bare
Exception, log withlogger.error, then wrap asValueError. This makes it harder to distinguish validation issues from system faults and loses stack traces.Consider a pattern like:
- except Exception as error: - logger.error(f"System error retrieving pets: {error}") - raise ValueError(f"Failed to retrieve pets: {str(error)}") + except ValueError: + # Let explicit validation errors propagate as-is + raise + except Exception as error: + logger.exception("System error retrieving pets") + raise ValueError("Failed to retrieve pets") from errorApplying this pattern (or equivalent) to
get_user_id_by_email,get_pets_by_user_id,get_pet_vaccination_info,book_vet_appointment, andcancel_appointmentwill address BLE001/TRY400/B904 and clarify error semantics.Also applies to: 255-263, 294-297
189-248: Use a collision‑resistant appointment ID generator.
appointment_id = f"APT-{pet_id}-{datetime.now().strftime('%Y%m%d%H%M%S')}"can collide if two bookings are created for the same pet within the same second.Consider incorporating a random/unique component, e.g.:
+ import uuid + unique_suffix = uuid.uuid4().hex[:8].upper() - appointment_id = f"APT-{pet_id}-{datetime.now().strftime('%Y%m%d%H%M%S')}" + appointment_id = f"APT-{pet_id}-{datetime.now().strftime('%Y%m%d%H%M%S')}-{unique_suffix}"
299-438: Simplify OpenAI client usage and clean up error handling insuggest_pet_names.This function mixes the module‑level
openai_clientwith checks/mutations ofopenai.api_key, and the exception handlers have unused variables plus a redundantraise ve. With the modern OpenAI SDK, it’s simpler and safer to rely solely on the instantiated client.Suggested adjustments:
- # Ensure OpenAI API key is set from environment if available - if OPENAI_API_KEY and not openai.api_key: - openai.api_key = OPENAI_API_KEY - - if not openai.api_key: - raise ValueError("OpenAI API key is not configured. Please set OPENAI_API_KEY in environment variables.") + if not openai_client: + raise ValueError("OpenAI API key is not configured. Please set OPENAI_API_KEY in environment variables.") @@ - response = openai_client.chat.completions.create( + response = openai_client.chat.completions.create( @@ - except openai.APIError as e: - error_message = f"OpenAI API error: {str(e)}" + except openai.APIError as e: + error_message = f"OpenAI API error: {e}" logger.error(error_message) raise ValueError(f"Failed to generate pet names: {error_message}") - except openai.RateLimitError as e: + except openai.RateLimitError: error_message = "OpenAI API rate limit exceeded. Please try again later." logger.error(error_message) raise ValueError(error_message) - except openai.AuthenticationError as e: + except openai.AuthenticationError: error_message = "OpenAI API authentication failed. Please check your API key." logger.error(error_message) raise ValueError(error_message) - except ValueError as ve: - raise ve + except ValueError: + # Re-raise validation/JSON errors unchanged + raiseAlso consider switching
logger.errortologger.exceptionin the JSON decode path so failures include stack traces.Pet_Care_ai_agent_secured_with_asgardeo/agent/agent.py (3)
213-287: Avoid bareexceptand multi‑statement lines inload_context_nodeJSON parsing.The parsing blocks use patterns like
if hasattr(result, 'content'): result = result.contentandtry: … except: pass, which trigger E701/E722 and silently drop parse errors.A minimal cleanup that preserves behavior:
- data = {} - if hasattr(result, 'content'): result = result.content - if isinstance(result, str): - try: - clean = result.replace('```json', '').replace('```', '').strip() - data = json.loads(clean) - except: pass + data = {} + if hasattr(result, "content"): + result = result.content + if isinstance(result, str): + try: + clean = result.replace("```json", "").replace("```", "").strip() + data = json.loads(clean) + except json.JSONDecodeError: + # Optionally log at debug level if needed + passApply the same structure in STEP 2 to address Ruff’s warnings and make failures more diagnosable.
451-510: Use structured logging instead ofhandle_chat_request.Catching a bare
Exceptionand printing the stack trace to stdout while returningstr(e)to the client can leak internal details and makes logs harder to consume in production.Pattern to consider:
- except Exception as e: - print(f"Error: {e}") - import traceback - traceback.print_exc() - return web.json_response({"error": str(e)}, status=500) + except web.HTTPException: + raise + except Exception as e: + # logger.exception("Unexpected error in /chat handler") + return web.json_response({"error": "Internal server error"}, status=500)You can keep
loggingand a generic 500 message is safer for real deployments.
516-522: Be aware that the mockaccess_tokenwill not pass JWT validation on the secured MCP server.The fallback path assigns
access_token = "mock_token"but the MCP server is configured with a strictJWTTokenVerifier; calls from this agent will fail once the resource server enforces real Asgardeo tokens.Either document this as “demo‑only” behavior or gate it behind a clearly named development flag/env var so it’s not accidentally used in environments where the FastMCP service expects valid JWTs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Pet_Care_ai_agent_secured_with_asgardeo/agent/agent.py(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/agent/frontend_app.py(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/agent/index.html(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/main.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Pet_Care_ai_agent_secured_with_asgardeo/agent/frontend_app.py
- Pet_Care_ai_agent_secured_with_asgardeo/agent/index.html
🧰 Additional context used
🧬 Code graph analysis (1)
Pet_Care_ai_agent_secured_with_asgardeo/agent/agent.py (1)
hotel-booking-agent-autogen-agent-iam/staff-management-agent/app/service.py (1)
invoke(183-210)
🪛 Ruff (0.14.5)
Pet_Care_ai_agent_secured_with_asgardeo/agent/agent.py
121-121: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
121-121: Create your own exception
(TRY002)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
152-152: Do not catch blind exception: Exception
(BLE001)
200-200: Do not catch blind exception: Exception
(BLE001)
238-238: Multiple statements on one line (colon)
(E701)
243-243: Do not use bare except
(E722)
243-243: try-except-pass detected, consider logging the exception
(S110)
243-243: Multiple statements on one line (colon)
(E701)
249-249: Do not catch blind exception: Exception
(BLE001)
265-265: Multiple statements on one line (colon)
(E701)
270-270: Do not use bare except
(E722)
270-270: try-except-pass detected, consider logging the exception
(S110)
270-270: Multiple statements on one line (colon)
(E701)
283-283: Do not catch blind exception: Exception
(BLE001)
399-399: Do not catch blind exception: Exception
(BLE001)
402-402: Consider [*messages_with_context, response, *tool_outputs] instead of concatenation
Replace with [*messages_with_context, response, *tool_outputs]
(RUF005)
405-405: Consider [response, *tool_outputs, final_response] instead of concatenation
Replace with [response, *tool_outputs, final_response]
(RUF005)
410-410: Unused function argument: state
(ARG001)
412-412: Unused function argument: state
(ARG001)
414-414: Unused function argument: state
(ARG001)
421-421: Multiple statements on one line (colon)
(E701)
422-422: Multiple statements on one line (colon)
(E701)
423-423: Multiple statements on one line (colon)
(E701)
424-424: Multiple statements on one line (colon)
(E701)
427-427: Unused function argument: mcp_tools
(ARG001)
469-469: Consider [*history, HumanMessage(content=user_input)] instead of concatenation
Replace with [*history, HumanMessage(content=user_input)]
(RUF005)
505-505: Do not catch blind exception: Exception
(BLE001)
518-518: Do not catch blind exception: Exception
(BLE001)
520-520: Possible hardcoded password assigned to: "access_token"
(S105)
542-542: Unused lambda argument: r
(ARG005)
548-548: Multiple statements on one line (colon)
(E701)
Pet_Care_ai_agent_secured_with_asgardeo/main.py
57-57: Do not catch blind exception: Exception
(BLE001)
58-58: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
91-91: Abstract raise to an inner function
(TRY301)
91-91: Avoid specifying long messages outside the exception class
(TRY003)
109-109: Probable use of insecure hash functions in hashlib: md5
(S324)
118-118: Do not catch blind exception: Exception
(BLE001)
119-119: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
120-120: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
120-120: Use explicit conversion flag
Replace with conversion flag
(RUF010)
132-132: Abstract raise to an inner function
(TRY301)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
155-155: Do not catch blind exception: Exception
(BLE001)
156-156: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
157-157: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
157-157: Avoid specifying long messages outside the exception class
(TRY003)
157-157: Use explicit conversion flag
Replace with conversion flag
(RUF010)
166-166: Abstract raise to an inner function
(TRY301)
166-166: Avoid specifying long messages outside the exception class
(TRY003)
182-182: Consider moving this statement to an else block
(TRY300)
184-184: Do not catch blind exception: Exception
(BLE001)
185-185: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
186-186: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
186-186: Use explicit conversion flag
Replace with conversion flag
(RUF010)
213-213: Abstract raise to an inner function
(TRY301)
213-213: Avoid specifying long messages outside the exception class
(TRY003)
219-219: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
219-219: Avoid specifying long messages outside the exception class
(TRY003)
254-254: Consider moving this statement to an else block
(TRY300)
258-258: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
259-259: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
259-259: Avoid specifying long messages outside the exception class
(TRY003)
260-260: Do not catch blind exception: Exception
(BLE001)
262-262: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
263-263: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
263-263: Avoid specifying long messages outside the exception class
(TRY003)
280-280: Abstract raise to an inner function
(TRY301)
280-280: Avoid specifying long messages outside the exception class
(TRY003)
292-292: Consider moving this statement to an else block
(TRY300)
294-294: Do not catch blind exception: Exception
(BLE001)
296-296: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
297-297: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
297-297: Avoid specifying long messages outside the exception class
(TRY003)
323-323: Abstract raise to an inner function
(TRY301)
323-323: Avoid specifying long messages outside the exception class
(TRY003)
331-331: Abstract raise to an inner function
(TRY301)
331-331: Avoid specifying long messages outside the exception class
(TRY003)
335-335: Abstract raise to an inner function
(TRY301)
335-335: Avoid specifying long messages outside the exception class
(TRY003)
365-365: Abstract raise to an inner function
(TRY301)
365-365: Avoid specifying long messages outside the exception class
(TRY003)
404-404: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
405-405: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
405-405: Avoid specifying long messages outside the exception class
(TRY003)
419-419: Consider moving this statement to an else block
(TRY300)
422-422: Use explicit conversion flag
Replace with conversion flag
(RUF010)
423-423: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
424-424: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
424-424: Avoid specifying long messages outside the exception class
(TRY003)
425-425: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
427-427: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
428-428: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
429-429: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
431-431: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
432-432: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
434-434: Use raise without specifying exception name
Remove exception name
(TRY201)
435-435: Do not catch blind exception: Exception
(BLE001)
437-437: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
438-438: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
438-438: Avoid specifying long messages outside the exception class
(TRY003)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
Pet_Care_ai_agent_secured_with_asgardeo/README.md (4)
23-33: Add language to project-structure code fence and switch to.env.exampleThe project-structure block is missing a language identifier (MD040) and still lists
.envdirectly. Consider both tightening lint and aligning with the earlier guidance to use an env template:-``` +```text ├── main.py # FastMCP server with secured endpoints @@ -├── .env # Environment variables configuration +├── .env.example # Environment variable template (copy to .env and edit) @@ -└── README.md # This file +└── README.md # This file[Suggest adjusting the surrounding text to tell users to copy
.env.exampleto.envand populate values.]
239-274: Add language identifiers to conversation example code blocksThe three sample conversation blocks are fenced with bare ``` and flagged by markdownlint (MD040). Mark them as plain text for clearer rendering:
-### Example 1: Single Pet User -``` +### Example 1: Single Pet User +```text @@ -### Example 2: Multiple Pet User -``` +### Example 2: Multiple Pet User +```text @@ -### Example 3: Booking Appointment -``` +### Example 3: Booking Appointment +```text
1-4: Prepend the WSO2 license header to the READMEThis markdown file should begin with the standard WSO2 Apache 2.0 license HTML comment block, with the year set to 2025, before the
# Pawsome Pet Caretitle. Reuse the header snippet already suggested in earlier review feedback.
72-95: Remove realistic-looking OpenAI API key from example and prefer.env.exampleThe “Example with actual values” block includes:
OPENAI_API_KEY=sk-proj-abc123xyz789This still looks like a real key and will keep triggering secret scanners. Use a clearly fake placeholder and, ideally, move all concrete examples into a committed
.env.exampleinstead of inline “actual values”:- # OpenAI Configuration - OPENAI_API_KEY=sk-proj-abc123xyz789 + # OpenAI Configuration + OPENAI_API_KEY=<your-openai-api-key>Then instruct users to copy
.env.example→.envand fill in real values locally.
🧹 Nitpick comments (2)
Pet_Care_ai_agent_secured_with_asgardeo/init_database.py (1)
10-16: Optional: clarify/descope destructive DB reset behavior
init_database()deletes any existingpet_clinic.dbunconditionally. That’s fine for a demo, but it’s worth either (a) clearly documenting “this will delete any existing data” in the README, or (b) gating it behind a prompt/flag if you expect people to run it repeatedly against non-throwaway data.Pet_Care_ai_agent_secured_with_asgardeo/database.py (1)
18-21: Uselogger.exceptionto preserve traceback on DB errorsInside the
exceptblock, usinglogger.exceptionwill log the full traceback, which is more useful for diagnosing DB issues:- except Exception as e: - conn.rollback() - logger.error(f"Database error: {e}") - raise + except Exception: + conn.rollback() + logger.exception("Database error") + raiseThis still re-raises the error but improves observability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Pet_Care_ai_agent_secured_with_asgardeo/README.md(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/database.py(1 hunks)Pet_Care_ai_agent_secured_with_asgardeo/init_database.py(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
Pet_Care_ai_agent_secured_with_asgardeo/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
253-253: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
265-265: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.5)
Pet_Care_ai_agent_secured_with_asgardeo/database.py
20-20: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
Co-authored-by: Pavindu Lakshan <[email protected]>
Co-authored-by: Pavindu Lakshan <[email protected]>
Co-authored-by: Pavindu Lakshan <[email protected]>
Co-authored-by: Pavindu Lakshan <[email protected]>
Co-authored-by: Pavindu Lakshan <[email protected]>
|
solved the reviews |
3 participants
Purpose
$subject
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.