Add quick start guide and samples to the ai agent management platform #1
Conversation
WalkthroughThis PR introduces a comprehensive quick-start installation system for deploying an Agent Management Platform with OpenChoreo on Kubernetes/Kind, plus a sample customer-support agent application demonstrating LangGraph-based agent orchestration with multiple tools (flights, hotels, car rentals, excursions, policies) backed by SQLite. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FastAPI as FastAPI App
participant Agent as Agent Graph
participant LLM as LLM Service
participant Tools as Tool Executor
participant DB as SQLite DB
User->>FastAPI: POST /invocations<br/>(thread_id, question, passenger_id)
FastAPI->>Agent: invoke with config<br/>and initial messages
Agent->>Agent: entry node: pass to Assistant
Agent->>LLM: invoke with tools binding<br/>(system prompt + tools)
LLM-->>Agent: response with tool_calls
Agent->>Agent: check if tool_calls present
alt Tool calls detected
Agent->>Tools: execute tool_calls
Tools->>DB: query/update operations<br/>(flights, hotels, rentals, etc.)
DB-->>Tools: results
Tools-->>Agent: ToolMessage results
Agent->>Agent: append ToolMessages to state
Agent->>LLM: invoke again with<br/>conversation history
LLM-->>Agent: refined response<br/>(may have more tool_calls)
else No tool calls / Stop
Agent->>Agent: prepare final answer
end
Agent-->>FastAPI: final message content
FastAPI-->>User: JSON response<br/>(results/answer)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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: 17
♻️ Duplicate comments (1)
quick-start/README.md (1)
197-197: Verify the TROUBLESHOOTING.md file exists.This documentation references
TROUBLESHOOTING.mdin multiple locations (lines 104, 197, 276), but the file is not included in this PR. Ensure it exists or will be added.This is a duplicate concern with QUICK_START.md. The same verification script applies.
🧹 Nitpick comments (33)
samples/customer-support-agent/LICENSE (1)
1-201: Clarify the licensing strategy for sample applications.The Apache License 2.0 is appropriate and the file is correctly formatted. However, consider whether each sample application should have its own LICENSE file or if there should be guidance on inherited licensing from the repository root.
If you'd like, I can help document a licensing strategy for samples in a README or CONTRIBUTING guide to clarify whether:
- Samples inherit the root LICENSE automatically.
- Samples can have their own licensing terms (as done here).
- A specific convention should be followed across all samples.
Would you like me to suggest a documentation approach for this?
samples/customer-support-agent/tools/__init__.py (1)
1-1: Consider making the database path configurable.The database path is hardcoded, which reduces flexibility and assumes a specific working directory. For better deployment flexibility, consider using an environment variable with a fallback.
Apply this diff:
+import os + -db = "travel2.sqlite" +db = os.getenv("AGENT_DB_PATH", "travel2.sqlite")samples/customer-support-agent/main.py (1)
3-5: Document that binding to 0.0.0.0 is for development only.Binding to all network interfaces (0.0.0.0) can expose the service to external networks, which may be a security risk. While acceptable for a sample application, this should be documented.
Consider adding a comment or making the host configurable:
+import os from app import app if __name__ == '__main__': import uvicorn - uvicorn.run(app, host="0.0.0.0", port=8000) + # Note: Binding to 0.0.0.0 is for development/demo purposes only + # In production, bind to specific interfaces or use a reverse proxy + host = os.getenv("AGENT_HOST", "0.0.0.0") + port = int(os.getenv("AGENT_PORT", "8000")) + uvicorn.run(app, host=host, port=port)samples/customer-support-agent/README.md (2)
2-2: Consider using a proper heading instead of emphasis.The "Generated by AI" text uses emphasis instead of a heading, which is flagged by markdown linters.
Apply this diff:
# Agent Service -_Generated by AI_ + +> Generated by AI
24-29: Add language identifier to code fence.The code block on line 26 is missing a language identifier, which reduces syntax highlighting and accessibility.
Apply this diff:
Example `.env` file: -``` +```env OPENAI_API_KEY=sk-... TAVILY_API_KEY=tvly-...</blockquote></details> <details> <summary>quick-start/kind-config.yaml (1)</summary><blockquote> `15-17`: **Consider potential issues with /tmp/kind-shared mount.** Mounting `/tmp/kind-shared` may encounter issues: - `/tmp` directories are commonly cleaned on system reboot - Permissions may vary across different host systems - Not all environments allow mounting from `/tmp` Consider using a more persistent location or documenting the requirements: ```diff extraMounts: - - hostPath: /tmp/kind-shared + # Note: Ensure this directory exists and has appropriate permissions + # Consider using a persistent location outside /tmp for production + - hostPath: /tmp/kind-shared containerPath: /mnt/sharedAlternatively, use a location like
/var/lib/kind-sharedor document that users should create the directory before running the Quick Start.quick-start/stop-port-forward.sh (1)
17-24: Use of eval is safe here but could be refactored.Line 24 uses
evalto execute the dynamically constructed lsof command. While the input is controlled (hardcoded ports array), using eval is generally discouraged. Consider refactoring for clarity.Apply this diff to avoid eval:
-# Build lsof command with all ports -LSOF_CMD="lsof" -for port in "${PORTS[@]}"; do - LSOF_CMD="$LSOF_CMD -i :$port" -done - -# Get kubectl processes using these ports -KUBECTL_PIDS=$(eval "$LSOF_CMD" 2>/dev/null | grep kubectl | awk '{print $2}' | sort -u || true) +# Get kubectl processes using these ports +# Build lsof arguments array +LSOF_ARGS=() +for port in "${PORTS[@]}"; do + LSOF_ARGS+=(-i ":$port") +done + +KUBECTL_PIDS=$(lsof "${LSOF_ARGS[@]}" 2>/dev/null | grep kubectl | awk '{print $2}' | sort -u || true)quick-start/QUICK_START.md (2)
5-5: Fix heading level increment.Markdown best practices require heading levels to increment by one at a time. The jump from h1 (line 1) to h3 (line 5) should be corrected.
Apply this diff:
-### Prerequisites +## Prerequisites
59-62: Convert bare URLs to proper markdown links.The bare URLs should be formatted as proper markdown links for better rendering and accessibility.
Apply this diff:
-- **Console**: http://localhost:3000 -- **API**: http://localhost:8080 -- **Traces Observer**: http://localhost:9098 -- **Data Prepper**: http://localhost:21893 +- **Console**: <http://localhost:3000> +- **API**: <http://localhost:8080> +- **Traces Observer**: <http://localhost:9098> +- **Data Prepper**: <http://localhost:21893>samples/customer-support-agent/openapi.yaml (1)
1-30: Consider adding security definitions for production use.The OpenAPI specification lacks security definitions (global
securityfield and operation-level security). While this may be acceptable for a sample/demo application, production APIs should define authentication and authorization schemes.Consider adding security definitions if this will be used beyond local development:
components: securitySchemes: bearerAuth: type: http scheme: bearer bearerFormat: JWT security: - bearerAuth: []quick-start/README.md (1)
43-46: Convert bare URLs to proper markdown links.Similar to QUICK_START.md, these bare URLs should be formatted as proper markdown links for consistency and better rendering.
Apply this diff:
-- Console: http://localhost:3000 -- API: http://localhost:8080 -- Traces Observer: http://localhost:9098 -- Data Prepper: http://localhost:21893 +- Console: <http://localhost:3000> +- API: <http://localhost:8080> +- Traces Observer: <http://localhost:9098> +- Data Prepper: <http://localhost:21893>quick-start/port-forward.sh (1)
23-71: Consider explicitly using PID variables for clarity.Shellcheck flags the PID variables as unused, but they are implicitly used by the
waitcommand (line 71) which waits for all background processes. For better clarity and explicit process management, consider passing the PIDs towaitor storing them in an array.For example:
# Collect PIDs in an array PIDS=() kubectl port-forward -n "$AMP_NS" svc/agent-management-platform-console 3000:3000 & PIDS+=($!) # ... other port-forwards ... # Wait for all processes explicitly for pid in "${PIDS[@]}"; do wait "$pid" 2>/dev/null || true doneAlternatively, the current implementation works correctly, as
waitwithout arguments waits for all background jobs.quick-start/example-values.yaml (2)
8-8: Strengthen the example password.While this is an example file, using a weak password like "changeme-secure-password" might be copied as-is. Consider using a stronger example or a placeholder that's obviously invalid.
- password: "changeme-secure-password" + password: "REPLACE_WITH_SECURE_PASSWORD_MIN_16_CHARS"
46-46: CORS wildcard is a security risk.The CORS configuration allows all origins (
*), which is explicitly noted to change for production. However, even in examples, this could be copied without modification.Consider providing a more specific example:
- corsAllowedOrigin: "*" # Change to specific domain in production + corsAllowedOrigin: "https://your-domain.com" # Specify your domain(s)quick-start/bootstrap.sh (1)
418-441: Consider adding cleanup trap for port-forward process.The port-forward process is started in the background (line 421) and its PID is saved (line 425), but if the bootstrap script is interrupted or fails after this point, the port-forward process may be left running. Consider adding a trap handler to clean up background processes on script exit.
Add a cleanup function and trap at the beginning of the script:
# After line 23 (after sourcing install-helpers.sh) cleanup() { if [[ -f "${SCRIPT_DIR}/.port-forward.pid" ]]; then PORT_FORWARD_PID=$(cat "${SCRIPT_DIR}/.port-forward.pid") kill "$PORT_FORWARD_PID" 2>/dev/null || true rm -f "${SCRIPT_DIR}/.port-forward.pid" fi } trap cleanup EXIT INT TERMsamples/customer-support-agent/app.py (2)
45-45: Blocking synchronous call in async endpoint.
run_agentperforms synchronous streaming which will block the async event loop. For a sample application this is acceptable, but in production consider running in a thread pool:import asyncio result = await asyncio.to_thread(run_agent, payload.thread_id, payload.question, payload.passenger_id)
31-39: Consider using logging instead of print for observability.The
+import logging + +logger = logging.getLogger(__name__) + final_answer = None for event in events: # Each event is a dict representing a streamed update if "messages" in event: - print(f"Received answer: {event}") + logger.debug(f"Received answer: {event}") # Keep updating until we get the latest assistant message final_answer = event["messages"][-1].contentsamples/customer-support-agent/setup/db.py (3)
26-27: Remove unusedcursorvariable.The
cursorvariable is assigned but never used since all queries usepd.read_sql.conn = sqlite3.connect(file) - cursor = conn.cursor()
33-34: Table name interpolation in SQL query.While the table names originate from
sqlite_master(trusted source), using f-string interpolation for SQL queries is generally discouraged. In this specific case, the risk is low since the input is controlled.For added safety, you could validate table names against an allowlist:
EXPECTED_TABLES = {"flights", "bookings", "car_rentals", "hotels", "excursions"} for t in tables: if t in EXPECTED_TABLES: tdf[t] = pd.read_sql(f"SELECT * from {t}", conn)
58-65: Unnecessary explicit deletion of local variables.The
del dfanddel tdfstatements on lines 60-61 are unnecessary as these variables will be garbage collected when the function returns.for table_name, df in tdf.items(): df.to_sql(table_name, conn, if_exists="replace", index=False) - del df - del tdf conn.commit() conn.close()samples/customer-support-agent/agent/agent.py (1)
82-85: Move imports to top of file.Imports inside the function body are unconventional and can cause subtle issues with import order. Move these to the top of the file.
+from langgraph.checkpoint.memory import InMemorySaver +from langgraph.graph import StateGraph, START +from langgraph.prebuilt import tools_condition + # ... at top of file with other importsThen remove lines 82-85 from inside
create_agent().samples/customer-support-agent/tools/car_rentals.py (2)
66-77: Use context manager for connection handling to ensure cleanup.The connection is closed in both branches of the if/else, which is error-prone. Using a context manager or try/finally ensures the connection is always closed.
- conn = sqlite3.connect(db) - cursor = conn.cursor() - - cursor.execute("UPDATE car_rentals SET booked = 1 WHERE id = ?", (rental_id,)) - conn.commit() - - if cursor.rowcount > 0: - conn.close() - return f"Car rental {rental_id} successfully booked." - else: - conn.close() - return f"No car rental found with ID {rental_id}." + with sqlite3.connect(db) as conn: + cursor = conn.cursor() + cursor.execute("UPDATE car_rentals SET booked = 1 WHERE id = ?", (rental_id,)) + conn.commit() + if cursor.rowcount > 0: + return f"Car rental {rental_id} successfully booked." + return f"No car rental found with ID {rental_id}."This pattern should be applied to all functions in this file.
43-52: Unused function parameters are intentional per comment.The
price_tier,start_date, andend_dateparameters are documented but intentionally unused (lines 43-44 explain this is for tutorial simplicity). Consider documenting this in the docstring or removing the parameters if they won't be implemented.Either update the docstring to note these are placeholders, or remove them to avoid confusion:
# For our tutorial, we will let you match on any dates and price tier. # (since our toy dataset doesn't have much data) + # Note: price_tier, start_date, end_date parameters are accepted but not used in filteringsamples/customer-support-agent/tools/excursions.py (2)
26-51: Tighten row mapping and consider small robustness tweaks insearch_trip_recommendations.The logic is sound, but a couple of small improvements would help:
- Capture
column_namesbefore closing the connection and usezip(..., strict=True)to address the Ruff B905 warning and fail fast if column/row lengths ever diverge.- Optionally, trim and drop empty keywords to avoid
LIKE '%%'clauses whenkeywordshas stray commas.For example:
- cursor.execute(query, params) - results = cursor.fetchall() - - conn.close() - - return [ - dict(zip([column[0] for column in cursor.description], row)) for row in results - ] + cursor.execute(query, params) + rows = cursor.fetchall() + column_names = [column[0] for column in cursor.description] + + conn.close() + + return [ + dict(zip(column_names, row, strict=True)) for row in rows + ]And, if you want to harden keyword handling:
- if keywords: - keyword_list = keywords.split(",") + if keywords: + keyword_list = [kw.strip() for kw in keywords.split(",") if kw.strip()] + if not keyword_list: + cursor.execute(query, params) + ...
55-134: Use context managers ortry/finallyfor SQLite connections in the write helpers.
book_excursion,update_excursion, andcancel_excursionall open a connection and then close it only on the happy/“no row” branches. Any exception betweenconnectand the finalconn.close()(e.g., DB locked, schema mismatch) will leak the connection.To make these helpers more robust and DRY, consider:
- Wrapping DB access in
with sqlite3.connect(db) as conn:and usingconn.cursor()inside, or- Using
try: ... finally: conn.close()around each operation.For example (pattern applies to all three):
- conn = sqlite3.connect(db) - cursor = conn.cursor() - ... - conn.commit() - - if cursor.rowcount > 0: - conn.close() - return ... - else: - conn.close() - return ... + conn = sqlite3.connect(db) + try: + cursor = conn.cursor() + ... + conn.commit() + + if cursor.rowcount > 0: + return ... + return ... + finally: + conn.close()samples/customer-support-agent/tools/policies.py (2)
12-18: Add a timeout to therequests.getcall to avoid hanging at import time.Because this HTTP fetch runs at module import, a slow or unreachable endpoint will hang the entire process indefinitely.
You can keep behavior the same while bounding the wait:
-response = requests.get( - "https://storage.googleapis.com/benchmarks-artifacts/travel-db/swiss_faq.md" -) +response = requests.get( + "https://storage.googleapis.com/benchmarks-artifacts/travel-db/swiss_faq.md", + timeout=10, +)
48-56: Be aware of heavy side effects at import time (OpenAI embeddings + HTTP).
retriever = VectorStoreRetriever.from_docs(docs, openai.Client())runs at import, which means:
- A remote HTTP fetch and
- A full embeddings call for all docs
happen before any tool is used. This is fine for a small demo, but in larger or test environments it can slow startup or make import brittle.
Consider deferring retriever construction until first
lookup_policycall (or behind a small initialization function) if you need more control over startup behavior.samples/customer-support-agent/tools/hotels.py (1)
10-51: Clarify or implementprice_tier/date filters insearch_hotels.The signature and docstring advertise filtering by
price_tier,checkin_date, andcheckout_date, but the current query only useslocationandname. That can confuse tool users and also trips Ruff’s unused‑argument checks.Options:
- If this is intentionally simplified for the tutorial, call that out in the docstring and/or prefix the parameters with
_to silence unused‑arg warnings.- If you plan to support real filtering, extend the query and parameter list accordingly.
Also, to address the B905 lint and align with other modules, you can tighten the mapping:
- cursor.execute(query, params) - results = cursor.fetchall() - - conn.close() - - return [ - dict(zip([column[0] for column in cursor.description], row)) for row in results - ] + cursor.execute(query, params) + rows = cursor.fetchall() + column_names = [column[0] for column in cursor.description] + + conn.close() + + return [ + dict(zip(column_names, row, strict=True)) for row in rows + ]samples/customer-support-agent/tools/flights.py (3)
25-53: Row-to-dict mapping infetch_user_flight_informationlooks good; minor lint-only tweak available.The join and mapping logic are correct. To align with Ruff B905 and other modules, you can precompute column names and use
zip(..., strict=True):- rows = cursor.fetchall() - column_names = [column[0] for column in cursor.description] - results = [dict(zip(column_names, row)) for row in rows] + rows = cursor.fetchall() + column_names = [column[0] for column in cursor.description] + results = [dict(zip(column_names, row, strict=True)) for row in rows]
65-92: Same optionalzip(strict=True)improvement forsearch_flights.As above, the query and mapping logic are fine. If you want to satisfy B905 consistently:
- rows = cursor.fetchall() - column_names = [column[0] for column in cursor.description] - results = [dict(zip(column_names, row)) for row in rows] + rows = cursor.fetchall() + column_names = [column[0] for column in cursor.description] + results = [dict(zip(column_names, row, strict=True)) for row in rows]
121-123: Optional: apply the samezip(strict=True)pattern when buildingnew_flight_dict.Not a bug, just consistency and a bit of safety:
- column_names = [column[0] for column in cursor.description] - new_flight_dict = dict(zip(column_names, new_flight)) + column_names = [column[0] for column in cursor.description] + new_flight_dict = dict(zip(column_names, new_flight, strict=True))quick-start/install-helpers.sh (2)
105-122: Reconsider always returning success fromwait_for_deployment.
wait_for_deploymentlogs a warning whenkubectl waitfails, but still returns0:if kubectl wait ...; then ... return 0 else log_warning "Deployment '$deployment' may still be starting" ... return 0 fiCallers like
install_agent_management_platformandinstall_observability_dataprepperrely on this for readiness gating; masking failures here can let the script continue even when critical deployments never became available.Two options:
- Make failures fatal by returning non‑zero in the
elsebranch for critical components, or- Add a parameter (e.g.,
fatal=true|false) so callers can decide per‑deployment whether a miss is fatal or just a warning.
230-249: Ensure the background pod monitor is cleaned up on early failures.
install_agent_management_platformstarts a background monitor:( ... ) & local monitor_pid=$! ... install_remote_helm_chart ... ... kill $monitor_pid 2>/dev/null || trueIf
install_remote_helm_chart(or a later step) fails and exits the script (because ofset -e), thekillline is never reached, and the backgroundkubectl get podsloop can keep running in the user’s shell session.Possible fixes:
- Track
monitor_pidin a global variable and havecleanup()kill it if set, or- Use a subshell for the entire install flow so background jobs terminate with it.
For example:
MONITOR_PID="" install_agent_management_platform() { ... ( ... ) & MONITOR_PID=$! ... } cleanup() { if [[ -n "$MONITOR_PID" ]]; then kill "$MONITOR_PID" 2>/dev/null || true fi log_info "Cleanup complete" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
README.md(1 hunks)quick-start/QUICK_START.md(1 hunks)quick-start/README.md(1 hunks)quick-start/bootstrap.sh(1 hunks)quick-start/example-values.yaml(1 hunks)quick-start/install-helpers.sh(1 hunks)quick-start/install.sh(1 hunks)quick-start/kind-config.yaml(1 hunks)quick-start/port-forward.sh(1 hunks)quick-start/stop-port-forward.sh(1 hunks)quick-start/uninstall.sh(1 hunks)samples/customer-support-agent/.gitignore(1 hunks)samples/customer-support-agent/LICENSE(1 hunks)samples/customer-support-agent/README.md(1 hunks)samples/customer-support-agent/agent/agent.py(1 hunks)samples/customer-support-agent/agent/state.py(1 hunks)samples/customer-support-agent/agent/utils.py(1 hunks)samples/customer-support-agent/app.py(1 hunks)samples/customer-support-agent/main.py(1 hunks)samples/customer-support-agent/openapi.yaml(1 hunks)samples/customer-support-agent/requirements.txt(1 hunks)samples/customer-support-agent/setup/db.py(1 hunks)samples/customer-support-agent/tools/__init__.py(1 hunks)samples/customer-support-agent/tools/car_rentals.py(1 hunks)samples/customer-support-agent/tools/excursions.py(1 hunks)samples/customer-support-agent/tools/flights.py(1 hunks)samples/customer-support-agent/tools/hotels.py(1 hunks)samples/customer-support-agent/tools/policies.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
samples/customer-support-agent/app.py (1)
samples/customer-support-agent/agent/agent.py (1)
create_agent(41-104)
quick-start/uninstall.sh (1)
quick-start/install-helpers.sh (6)
log_error(45-47)log_info(33-35)log_success(37-39)helm_release_exists(61-65)namespace_exists(55-58)log_warning(41-43)
quick-start/bootstrap.sh (1)
quick-start/install-helpers.sh (9)
log_error(45-47)log_info(33-35)verify_openchoreo_prerequisites(742-779)log_success(37-39)setup_kind_cluster(487-547)install_openchoreo_core(683-739)install_agent_management_platform_silent(332-351)install_observability_dataprepper_silent(314-329)log_warning(41-43)
🪛 Checkov (3.2.334)
samples/customer-support-agent/openapi.yaml
[high] 1-32: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-32: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
🪛 LanguageTool
quick-start/QUICK_START.md
[style] ~98-~98: Consider using “inaccessible” to avoid wordiness.
Context: ... ./install.sh --verbose **Services not accessible?** Check port forwarding:bash kubec...
(NOT_ABLE_PREMIUM)
samples/customer-support-agent/README.md
[grammar] ~22-~22: Ensure spelling is correct
Context: ...penAI API key - TRIVIY_API_KEY - your Triviy API key Example .env file: ``` OPEN...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
quick-start/README.md
[style] ~22-~22: Consider replacing this word to strengthen your wording.
Context: ...te**: Observability is a core component and is always installed, not optional. ---...
(AND_THAT)
🪛 markdownlint-cli2 (0.18.1)
quick-start/QUICK_START.md
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
59-59: Bare URL used
(MD034, no-bare-urls)
60-60: Bare URL used
(MD034, no-bare-urls)
61-61: Bare URL used
(MD034, no-bare-urls)
62-62: Bare URL used
(MD034, no-bare-urls)
samples/customer-support-agent/README.md
2-2: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
6-6: Bare URL used
(MD034, no-bare-urls)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
quick-start/README.md
43-43: Bare URL used
(MD034, no-bare-urls)
44-44: Bare URL used
(MD034, no-bare-urls)
45-45: Bare URL used
(MD034, no-bare-urls)
46-46: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.6)
samples/customer-support-agent/tools/excursions.py
50-50: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
samples/customer-support-agent/tools/policies.py
12-12: Probable use of requests call without timeout
(S113)
samples/customer-support-agent/agent/utils.py
14-14: Use explicit conversion flag
Replace with conversion flag
(RUF010)
samples/customer-support-agent/tools/hotels.py
14-14: Unused function argument: price_tier
(ARG001)
15-15: Unused function argument: checkin_date
(ARG001)
16-16: Unused function argument: checkout_date
(ARG001)
50-50: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
samples/customer-support-agent/setup/db.py
15-15: Probable use of requests call without timeout
(S113)
27-27: Local variable cursor is assigned to but never used
Remove assignment to unused variable cursor
(F841)
34-34: Possible SQL injection vector through string-based query construction
(S608)
samples/customer-support-agent/main.py
5-5: Possible binding to all interfaces
(S104)
samples/customer-support-agent/agent/agent.py
9-9: from tools.car_rentals import * used; unable to detect undefined names
(F403)
10-10: from tools.excursions import * used; unable to detect undefined names
(F403)
11-11: from tools.flights import * used; unable to detect undefined names
(F403)
12-12: from tools.hotels import * used; unable to detect undefined names
(F403)
13-13: from tools.policies import * used; unable to detect undefined names
(F403)
15-15: load_dotenv may be undefined, or defined from star imports
(F405)
21-21: RunnableConfig may be undefined, or defined from star imports
(F405)
31-32: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
57-57: datetime may be undefined, or defined from star imports
(F405)
61-61: fetch_user_flight_information may be undefined, or defined from star imports
(F405)
62-62: search_flights may be undefined, or defined from star imports
(F405)
63-63: lookup_policy may be undefined, or defined from star imports
(F405)
64-64: update_ticket_to_new_flight may be undefined, or defined from star imports
(F405)
65-65: cancel_ticket may be undefined, or defined from star imports
(F405)
66-66: search_car_rentals may be undefined, or defined from star imports
(F405)
67-67: book_car_rental may be undefined, or defined from star imports
(F405)
68-68: update_car_rental may be undefined, or defined from star imports
(F405)
69-69: cancel_car_rental may be undefined, or defined from star imports
(F405)
70-70: search_hotels may be undefined, or defined from star imports
(F405)
71-71: book_hotel may be undefined, or defined from star imports
(F405)
72-72: update_hotel may be undefined, or defined from star imports
(F405)
73-73: cancel_hotel may be undefined, or defined from star imports
(F405)
74-74: search_trip_recommendations may be undefined, or defined from star imports
(F405)
75-75: book_excursion may be undefined, or defined from star imports
(F405)
76-76: update_excursion may be undefined, or defined from star imports
(F405)
77-77: cancel_excursion may be undefined, or defined from star imports
(F405)
samples/customer-support-agent/tools/flights.py
23-23: Avoid specifying long messages outside the exception class
(TRY003)
48-48: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
91-91: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
122-122: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
174-174: Avoid specifying long messages outside the exception class
(TRY003)
samples/customer-support-agent/tools/car_rentals.py
14-14: Unused function argument: price_tier
(ARG001)
15-15: Unused function argument: start_date
(ARG001)
16-16: Unused function argument: end_date
(ARG001)
51-51: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🪛 Shellcheck (0.11.0)
quick-start/port-forward.sh
[warning] 23-23: CONSOLE_PID appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 28-28: AGENT_MGR_PID appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 34-34: TRACES_PID appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 43-43: DATAPREPPER_PID appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 52-52: EXTERNAL_GATEWAY_PID appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (15)
samples/customer-support-agent/.gitignore (1)
1-206: Comprehensive and well-organized Python.gitignorefile.The file follows GitHub's standard Python
.gitignoretemplate with helpful comments and well-organized sections covering Python artifacts, distributions, test outputs, virtual environments, and tool-specific patterns (Abstra, VS Code, Ruff, Cursor, Marimo). The structure is excellent for a Python sample project.samples/customer-support-agent/agent/utils.py (2)
22-25: LGTM!The fallback pattern correctly wraps the ToolNode with error handling via RunnableLambda.
28-41: LGTM!The debug utility correctly tracks printed message IDs to avoid duplication and truncates long representations for readability.
README.md (1)
4-6: LGTM!Clear and concise Quick Start section that guides users to the appropriate documentation.
quick-start/stop-port-forward.sh (1)
1-71: LGTM!The script has robust error handling, provides clear user feedback, and uses multiple cleanup methods to ensure port-forward processes are terminated. The three-method approach (lsof, PID file, pgrep/pkill) ensures thorough cleanup.
quick-start/kind-config.yaml (1)
8-8: File does not exist in repository.The file
quick-start/kind-config.yamlis not present in this repository. The review comment cannot be applied as there is no file to review at the specified location.Likely an incorrect or invalid review comment.
samples/customer-support-agent/agent/state.py (1)
1-9: LGTM!The State TypedDict is correctly defined following LangGraph conventions. The
messagesfield withadd_messagesannotation is the standard pattern for managing conversation state.quick-start/uninstall.sh (1)
1-132: LGTM with one concern addressed above.The uninstall script is well-structured with proper confirmation prompts, safety guards, and clear user feedback. The only issue is the overly broad pkill command flagged above.
quick-start/QUICK_START.md (1)
104-104: The file being reviewed does not exist in the repository.The review references
quick-start/QUICK_START.mdandTROUBLESHOOTING.md, but neither file exists in this repository. A search of all tracked and untracked files confirms these files are not present. The documentation and directory structure referenced in this review comment do not match the actual repository content.Likely an incorrect or invalid review comment.
quick-start/bootstrap.sh (1)
220-220: Cannot verify path reference—files do not exist in repository.The review comment references
quick-start/bootstrap.shat line 220, but this file and the relateddeployments/directory do not exist in the current repository state. Manual verification is required when the code is available in the actual PR or branch context.quick-start/install.sh (3)
1-3: Good use of strict mode.Using
set -eo pipefailensures the script fails fast on errors and catches failures in pipelines. This is a best practice for installation scripts.
67-69: The referenced files do not exist in the repository.The review comment references
quick-start/install.sh(lines 67-69) andquick-start/install-helpers.sh, but neither file exists in the repository. The codebase currently contains only configuration files (.gitignore, LICENSE, README.md, etc.). This review comment cannot be addressed as written.Likely an incorrect or invalid review comment.
124-132: File and code referenced in this comment do not exist in the repository.The quick-start/install.sh file, the port-forwarding code at lines 124-132, and the stop-port-forward.sh script do not exist in this repository. This repository currently contains only template and configuration files with no shell scripts or quick-start directories.
Likely an incorrect or invalid review comment.
samples/customer-support-agent/tools/flights.py (1)
109-131: The review comment references a file that does not exist in this repository.The file
samples/customer-support-agent/tools/flights.pyis not present in the repository at any branch. The review comment cannot be applied to this codebase.Verify that the review is intended for this repository and branch. If this review was meant for a different repository or pull request, please provide the correct target location.
Likely an incorrect or invalid review comment.
samples/customer-support-agent/tools/policies.py (1)
21-45: The review comment referencessamples/customer-support-agent/tools/policies.py, which does not exist in this repository. The file andVectorStoreRetrieverclass cannot be located. This comment cannot be applied to the current codebase.Likely an incorrect or invalid review comment.
| rbac: | ||
| create: true | ||
| # Full permissions for agent management operations | ||
| rules: | ||
| - apiGroups: ["*"] | ||
| resources: ["*"] | ||
| verbs: ["*"] |
There was a problem hiding this comment.
Overly permissive RBAC rules pose a security risk.
The RBAC configuration grants full permissions across all API groups, resources, and verbs (apiGroups: ["*"], resources: ["*"], verbs: ["*"]). This is a cluster-admin level of access and violates the principle of least privilege. Even in example files, this can be copied by users into production environments.
Apply this diff to provide a more appropriate example scoped to agent management:
# RBAC Configuration
rbac:
create: true
- # Full permissions for agent management operations
+ # Scoped permissions for agent management operations
rules:
- - apiGroups: ["*"]
- resources: ["*"]
- verbs: ["*"]
+ - apiGroups: [""]
+ resources: ["pods", "services", "configmaps", "secrets"]
+ verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+ - apiGroups: ["apps"]
+ resources: ["deployments", "statefulsets"]
+ verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+ - apiGroups: ["batch"]
+ resources: ["jobs", "cronjobs"]
+ verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]Committable suggestion skipped: line range outside the PR's diff.
| wait_for_pods() { | ||
| local namespace="$1" | ||
| local timeout="${2:-300}" # 5 minutes default | ||
| local label_selector="${3:-}" | ||
|
|
||
| log_info "Waiting for pods in namespace '$namespace' to be ready..." | ||
|
|
||
| local selector_flag="" | ||
| if [[ -n "$label_selector" ]]; then | ||
| selector_flag="-l $label_selector" | ||
| fi | ||
|
|
||
| if ! timeout "$timeout" bash -c " | ||
| while true; do | ||
| pods=\$(kubectl get pods -n '$namespace' $selector_flag --no-headers 2>/dev/null || true) | ||
| if [[ -z \"\$pods\" ]]; then | ||
| echo 'No pods found yet, waiting...' | ||
| sleep 5 | ||
| continue | ||
| fi | ||
| if echo \"\$pods\" | grep -v 'Running\|Completed' | grep -q .; then | ||
| echo 'Waiting for pods to be ready...' | ||
| sleep 5 | ||
| else | ||
| echo 'All pods are ready!' | ||
| break | ||
| fi | ||
| done | ||
| "; then | ||
| log_error "Timeout waiting for pods in namespace '$namespace'" | ||
| kubectl get pods -n "$namespace" $selector_flag 2>/dev/null || true | ||
| return 1 | ||
| fi | ||
|
|
||
| log_success "All pods in namespace '$namespace' are ready" | ||
| } |
There was a problem hiding this comment.
Add a prereq check (or fallback) for timeout in wait_for_pods.
wait_for_pods assumes a GNU timeout binary is available:
if ! timeout "$timeout" bash -c "..."On macOS (without coreutils) or other environments without timeout, this will cause an immediate failure despite verify_prerequisites passing.
Consider either:
- Adding
timeoutto the prerequisites check and documenting it, or - Falling back to a plain loop when
timeoutis not present, e.g.:
if ! command_exists timeout; then
# simple, unbounded wait loop as a fallback
while true; do
...
done
else
if ! timeout "$timeout" bash -c '...'; then
...
fi
fi🤖 Prompt for AI Agents
In quick-start/install-helpers.sh around lines 68 to 103, the wait_for_pods
function assumes the GNU timeout binary exists which breaks on macOS/other
systems; add a prerequisite check for timeout at script startup (or include it
in verify_prerequisites) and modify wait_for_pods to fallback when timeout is
missing: detect with command -v timeout (or helper command_exists) and if absent
run the existing pod-wait loop without timeout or implement a simple manual
timeout using SECONDS to track elapsed time and exit with failure after the
configured timeout; ensure logs and exit codes remain unchanged when the
fallback path is used.
| --config) | ||
| if [[ -f "$2" ]]; then | ||
| AMP_HELM_ARGS+=("-f" "$2") | ||
| else | ||
| log_error "Config file not found: $2" | ||
| exit 1 | ||
| fi | ||
| shift 2 |
There was a problem hiding this comment.
Array AMP_HELM_ARGS used before initialization.
The AMP_HELM_ARGS array is appended to on line 27 but never initialized. This could cause issues depending on bash version and settings.
Initialize the array early in the script:
# Configuration
VERBOSE="${VERBOSE:-false}"
AUTO_PORT_FORWARD="${AUTO_PORT_FORWARD:-true}"
+AMP_HELM_ARGS=()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In quick-start/install.sh around lines 25 to 32, AMP_HELM_ARGS is appended to on
line 27 but never initialized; initialize AMP_HELM_ARGS as an empty array
(AMP_HELM_ARGS=()) near the top of the script before any argument parsing so
subsequent AMP_HELM_ARGS+=() operations are safe across bash versions and
settings.
| fi | ||
|
|
||
| # Kill all kubectl port-forward processes | ||
| pkill -f "kubectl port-forward" 2>/dev/null || true |
There was a problem hiding this comment.
Narrow the pkill scope to avoid terminating unrelated processes.
The command pkill -f "kubectl port-forward" will terminate ALL kubectl port-forward processes on the system, including those unrelated to this installation. This could disrupt other development workflows.
Consider limiting the cleanup to only the port-forwards managed by this script, or at least warn the user:
-# Kill all kubectl port-forward processes
-pkill -f "kubectl port-forward" 2>/dev/null || true
-log_success "All port forwarding processes stopped"
+# Kill port-forwards related to this installation
+log_warning "Stopping port-forward processes for agent-management-platform..."
+pkill -f "kubectl port-forward.*agent-management-platform" 2>/dev/null || true
+pkill -f "kubectl port-forward.*openchoreo-observability-plane" 2>/dev/null || true
+pkill -f "kubectl port-forward.*openchoreo-data-plane" 2>/dev/null || true
+log_success "Port forwarding processes stopped"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In quick-start/uninstall.sh around line 82, the pkill invocation is too broad
and will kill all kubectl port-forward processes system-wide; change it to only
target port-forwards started by this script by storing and later killing their
PIDs (save each kubectl port-forward PID when launched to a temp file and
read/kill those PIDs here), or at minimum narrow the match to a unique
identifier used by this script (e.g., include a specific port, namespace, label,
or unique command fragment) and warn the user before killing; implement one of
these approaches and ensure the cleanup handles missing PIDs gracefully.
| *.sage.py | ||
|
|
||
| # Environments | ||
| ../.env |
There was a problem hiding this comment.
Reconsider the relative path pattern for .env.
The pattern ../.env on Line 138 uses a relative path traversal to target the parent directory. However, .gitignore patterns are relative to the directory containing the .gitignore file, so this pattern would only ignore samples/.env, which may not be the intended behavior. If the goal is to ignore .env files at the project root, this pattern won't work as expected because .gitignore does not allow patterns to traverse above its containing directory through ...
Consider removing the ../ prefix and using only .env if the intent is to ignore .env files in this sample directory.
Apply this diff to fix the pattern:
-../.env
+.env📝 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.
| ../.env | |
| .env |
🤖 Prompt for AI Agents
In samples/customer-support-agent/.gitignore around line 138, the pattern
"../.env" incorrectly attempts to traverse to the parent directory (gitignore
patterns are relative to the file's directory); replace that entry with ".env"
(or remove the "../" prefix) so the sample's .env is ignored as intended, and
commit the updated .gitignore.
| The service requires two API keys (set these as environment variables or in a `.env` file in the project root): | ||
|
|
||
| - `OPENAI_API_KEY` - your OpenAI API key | ||
| - `TRIVIY_API_KEY` - your Triviy API key | ||
|
|
||
| Example `.env` file: | ||
|
|
||
| ``` | ||
| OPENAI_API_KEY=sk-... | ||
| TRIVIY_API_KEY=triviy-... | ||
| ``` |
There was a problem hiding this comment.
Fix typo in API key name.
The API key is misspelled as "TRIVIY_API_KEY" when it should be "TAVILY_API_KEY" (based on the tavily-python dependency). This will cause runtime failures when users follow the documentation.
Apply this diff:
- `OPENAI_API_KEY` - your OpenAI API key
-- `TRIVIY_API_KEY` - your Triviy API key
+- `TAVILY_API_KEY` - your Tavily API key
Example `.env` file:
OPENAI_API_KEY=sk-...
-TRIVIY_API_KEY=triviy-...
+TAVILY_API_KEY=tvly-...
📝 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.
| The service requires two API keys (set these as environment variables or in a `.env` file in the project root): | |
| - `OPENAI_API_KEY` - your OpenAI API key | |
| - `TRIVIY_API_KEY` - your Triviy API key | |
| Example `.env` file: | |
| ``` | |
| OPENAI_API_KEY=sk-... | |
| TRIVIY_API_KEY=triviy-... | |
| ``` | |
| The service requires two API keys (set these as environment variables or in a `.env` file in the project root): | |
| - `OPENAI_API_KEY` - your OpenAI API key | |
| - `TAVILY_API_KEY` - your Tavily API key | |
| Example `.env` file: | |
🧰 Tools
🪛 LanguageTool
[grammar] ~22-~22: Ensure spelling is correct
Context: ...penAI API key - TRIVIY_API_KEY - your Triviy API key Example .env file: ``` OPEN...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In samples/customer-support-agent/README.md around lines 19 to 29, the README
contains a typo in the environment variable name and example value — it uses
TRIVIY_API_KEY/triviy-... but the project depends on tavily-python so the
correct env var is TAVILY_API_KEY and example value prefix should be tvly-....
Update the README and example .env to replace TRIVIY_API_KEY with TAVILY_API_KEY
and change the sample key value to tvly-... so users set the correct variable
name and avoid runtime failures.
| langgraph | ||
| langchain-community | ||
| langchain_openai | ||
| tavily-python | ||
| pandas | ||
| openai | ||
| fastapi | ||
| uvicorn |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check latest versions of dependencies on PyPI
for pkg in langgraph langchain-community langchain_openai tavily-python pandas openai fastapi uvicorn; do
echo "=== $pkg ==="
curl -s "https://pypi.org/pypi/$pkg/json" | jq -r '.info.version'
doneRepository: wso2/ai-agent-management-platform
Length of output: 283
Pin dependency versions for reproducibility and security.
All dependencies are unpinned, which can lead to version drift, broken builds, and potential security vulnerabilities. This affects reproducibility and supply chain security.
Apply this diff to pin versions to current stable releases:
-langgraph
-langchain-community
-langchain_openai
-tavily-python
-pandas
-openai
-fastapi
-uvicorn
+langgraph==1.0.4
+langchain-community==0.4.1
+langchain_openai==1.1.0
+tavily-python==0.7.13
+pandas==2.3.3
+openai==2.8.1
+fastapi==0.123.0
+uvicorn[standard]==0.38.0📝 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.
| langgraph | |
| langchain-community | |
| langchain_openai | |
| tavily-python | |
| pandas | |
| openai | |
| fastapi | |
| uvicorn | |
| langgraph==1.0.4 | |
| langchain-community==0.4.1 | |
| langchain_openai==1.1.0 | |
| tavily-python==0.7.13 | |
| pandas==2.3.3 | |
| openai==2.8.1 | |
| fastapi==0.123.0 | |
| uvicorn[standard]==0.38.0 |
🤖 Prompt for AI Agents
In samples/customer-support-agent/requirements.txt lines 1–8 the dependencies
are unpinned; replace each entry with an exact, tested version pin
(package==MAJOR.MINOR.PATCH) for reproducibility and security—pin langgraph,
langchain-community, langchain_openai, tavily-python, pandas, openai, fastapi,
and uvicorn to their current stable releases, optionally add integrity hashes or
maintain a separate constraints/lock file, then run the test matrix (and
pip-compile or pip freeze) to verify compatibility and commit the updated
requirements.txt.
| if overwrite or not os.path.exists(local_file): | ||
| response = requests.get(db_url) | ||
| response.raise_for_status() # Ensure the request was successful | ||
| with open(local_file, "wb") as f: | ||
| f.write(response.content) | ||
| # Backup - we will use this to "reset" our DB in each section | ||
| shutil.copy(local_file, backup_file) |
There was a problem hiding this comment.
Add timeout to HTTP request to prevent indefinite hangs.
The requests.get call lacks a timeout, which could cause the script to hang indefinitely if the server is unresponsive.
- response = requests.get(db_url)
+ response = requests.get(db_url, timeout=30)📝 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.
| if overwrite or not os.path.exists(local_file): | |
| response = requests.get(db_url) | |
| response.raise_for_status() # Ensure the request was successful | |
| with open(local_file, "wb") as f: | |
| f.write(response.content) | |
| # Backup - we will use this to "reset" our DB in each section | |
| shutil.copy(local_file, backup_file) | |
| if overwrite or not os.path.exists(local_file): | |
| response = requests.get(db_url, timeout=30) | |
| response.raise_for_status() # Ensure the request was successful | |
| with open(local_file, "wb") as f: | |
| f.write(response.content) | |
| # Backup - we will use this to "reset" our DB in each section | |
| shutil.copy(local_file, backup_file) |
🧰 Tools
🪛 Ruff (0.14.6)
15-15: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
In samples/customer-support-agent/setup/db.py around lines 14 to 20, the
requests.get call has no timeout which can cause the script to hang; update the
call to include a sensible timeout (e.g., requests.get(db_url, timeout=10)) and
handle potential timeout/network exceptions by catching
requests.exceptions.Timeout and requests.exceptions.RequestException to log or
raise a clear error so the script fails fast instead of hanging.
| if start_date: | ||
| cursor.execute( | ||
| "UPDATE car_rentals SET start_date = ? WHERE id = ?", | ||
| (start_date, rental_id), | ||
| ) | ||
| if end_date: | ||
| cursor.execute( | ||
| "UPDATE car_rentals SET end_date = ? WHERE id = ?", (end_date, rental_id) | ||
| ) | ||
|
|
||
| conn.commit() | ||
|
|
||
| if cursor.rowcount > 0: | ||
| conn.close() | ||
| return f"Car rental {rental_id} successfully updated." | ||
| else: | ||
| conn.close() | ||
| return f"No car rental found with ID {rental_id}." |
There was a problem hiding this comment.
rowcount check may give incorrect result when both dates are updated.
When both start_date and end_date are provided, cursor.rowcount only reflects the second UPDATE statement. If the first update succeeds but the second finds no rows (shouldn't happen for same ID, but still), the message could be misleading.
Consider tracking success from either update:
+ rows_updated = 0
if start_date:
cursor.execute(
"UPDATE car_rentals SET start_date = ? WHERE id = ?",
(start_date, rental_id),
)
+ rows_updated += cursor.rowcount
if end_date:
cursor.execute(
"UPDATE car_rentals SET end_date = ? WHERE id = ?", (end_date, rental_id)
)
+ rows_updated += cursor.rowcount
conn.commit()
- if cursor.rowcount > 0:
+ if rows_updated > 0:
conn.close()
return f"Car rental {rental_id} successfully updated."📝 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.
| if start_date: | |
| cursor.execute( | |
| "UPDATE car_rentals SET start_date = ? WHERE id = ?", | |
| (start_date, rental_id), | |
| ) | |
| if end_date: | |
| cursor.execute( | |
| "UPDATE car_rentals SET end_date = ? WHERE id = ?", (end_date, rental_id) | |
| ) | |
| conn.commit() | |
| if cursor.rowcount > 0: | |
| conn.close() | |
| return f"Car rental {rental_id} successfully updated." | |
| else: | |
| conn.close() | |
| return f"No car rental found with ID {rental_id}." | |
| rows_updated = 0 | |
| if start_date: | |
| cursor.execute( | |
| "UPDATE car_rentals SET start_date = ? WHERE id = ?", | |
| (start_date, rental_id), | |
| ) | |
| rows_updated += cursor.rowcount | |
| if end_date: | |
| cursor.execute( | |
| "UPDATE car_rentals SET end_date = ? WHERE id = ?", (end_date, rental_id) | |
| ) | |
| rows_updated += cursor.rowcount | |
| conn.commit() | |
| if rows_updated > 0: | |
| conn.close() | |
| return f"Car rental {rental_id} successfully updated." | |
| else: | |
| conn.close() | |
| return f"No car rental found with ID {rental_id}." |
🤖 Prompt for AI Agents
In samples/customer-support-agent/tools/car_rentals.py around lines 100 to 117,
the logic uses cursor.rowcount after two separate UPDATEs which only reflects
the last statement; to fix this, track whether any update affected rows by
initializing a variable (e.g., updated = False) before the conditionals, after
each cursor.execute check if cursor.rowcount > 0 and set updated = True, commit
and close the connection, then use the updated flag to determine the success
message (return success if updated is True, otherwise return not found).
| @tool | ||
| def update_hotel( | ||
| hotel_id: int, | ||
| checkin_date: Optional[Union[datetime, date]] = None, | ||
| checkout_date: Optional[Union[datetime, date]] = None, | ||
| ) -> str: | ||
| """ | ||
| Update a hotel's check-in and check-out dates by its ID. | ||
|
|
||
| Args: | ||
| hotel_id (int): The ID of the hotel to update. | ||
| checkin_date (Optional[Union[datetime, date]]): The new check-in date of the hotel. Defaults to None. | ||
| checkout_date (Optional[Union[datetime, date]]): The new check-out date of the hotel. Defaults to None. | ||
|
|
||
| Returns: | ||
| str: A message indicating whether the hotel was successfully updated or not. | ||
| """ | ||
| conn = sqlite3.connect(db) | ||
| cursor = conn.cursor() | ||
|
|
||
| if checkin_date: | ||
| cursor.execute( | ||
| "UPDATE hotels SET checkin_date = ? WHERE id = ?", (checkin_date, hotel_id) | ||
| ) | ||
| if checkout_date: | ||
| cursor.execute( | ||
| "UPDATE hotels SET checkout_date = ? WHERE id = ?", | ||
| (checkout_date, hotel_id), | ||
| ) | ||
|
|
||
| conn.commit() | ||
|
|
||
| if cursor.rowcount > 0: | ||
| conn.close() | ||
| return f"Hotel {hotel_id} successfully updated." | ||
| else: | ||
| conn.close() | ||
| return f"No hotel found with ID {hotel_id}." | ||
|
|
There was a problem hiding this comment.
Handle “no fields to update” and track affected rows correctly in update_hotel.
Two edge cases here can produce misleading results:
- If both
checkin_dateandcheckout_dateareNone, noUPDATEruns, but the function still returns “No hotel found with ID …”. cursor.rowcountonly reflects the lastexecutecall; if you ever extend this to multiple updates, you’ll want to aggregate.
You can make behavior explicit and slightly more accurate:
- conn = sqlite3.connect(db)
- cursor = conn.cursor()
-
- if checkin_date:
- cursor.execute(
- "UPDATE hotels SET checkin_date = ? WHERE id = ?", (checkin_date, hotel_id)
- )
- if checkout_date:
- cursor.execute(
- "UPDATE hotels SET checkout_date = ? WHERE id = ?",
- (checkout_date, hotel_id),
- )
-
- conn.commit()
-
- if cursor.rowcount > 0:
- conn.close()
- return f"Hotel {hotel_id} successfully updated."
- else:
- conn.close()
- return f"No hotel found with ID {hotel_id}."
+ if checkin_date is None and checkout_date is None:
+ return "No fields provided to update."
+
+ conn = sqlite3.connect(db)
+ try:
+ cursor = conn.cursor()
+ rows_affected = 0
+
+ if checkin_date is not None:
+ cursor.execute(
+ "UPDATE hotels SET checkin_date = ? WHERE id = ?",
+ (checkin_date, hotel_id),
+ )
+ rows_affected += cursor.rowcount
+
+ if checkout_date is not None:
+ cursor.execute(
+ "UPDATE hotels SET checkout_date = ? WHERE id = ?",
+ (checkout_date, hotel_id),
+ )
+ rows_affected += cursor.rowcount
+
+ conn.commit()
+
+ if rows_affected > 0:
+ return f"Hotel {hotel_id} successfully updated."
+ return f"No hotel found with ID {hotel_id}."
+ finally:
+ conn.close()🤖 Prompt for AI Agents
In samples/customer-support-agent/tools/hotels.py around lines 79 to 117, the
update_hotel function can return misleading results when no fields are provided
and rowcount only reflects the last execute; fix by first checking if neither
checkin_date nor checkout_date is provided and return a clear message like "No
fields to update for hotel {hotel_id}." before opening a DB transaction, and
track affected rows across updates by accumulating a local counter (e.g.,
total_updated += cursor.rowcount) after each execute; finally commit/close and
use the accumulated total to decide whether to report success or "No hotel found
with ID ...".
1 participant
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.