-
Notifications
You must be signed in to change notification settings - Fork 408
feat(samples): add real-estate-agent sample #198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @amineremache, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request delivers a comprehensive new sample showcasing an intelligent real-estate agent built on the A2A protocol. It integrates a Python agent with a Node.js tool server that performs web scraping and leverages a local LLM for natural language processing, all packaged for straightforward deployment via Docker Compose, including cloud-ready scripts.
Highlights
- New Sample Introduction: This PR introduces a new, self-contained
real-estate-agent
sample, demonstrating an Agent-to-Agent (A2A) agent capable of understanding natural language queries to search for rental properties. - Multi-Service Architecture: The sample is composed of three orchestrated services: a core Python A2A agent, a Node.js MCP tool server (
dafty-mcp
) for real-estate interactions, and a local LLM service (ollama
) for natural language understanding. - Natural Language Understanding: The
dafty-mcp
server integrates with a localollama
instance running thetinydolphin
model to parse natural language queries (e.g., 'Find a 2-bed apartment in Dublin under €2000') into structured JSON parameters. - Web Scraping for Property Search: The
dafty-mcp
tool server implements robust web scraping functionality to fetch and filter rental property data directly from Daft.ie, including handling multi-unit developments and detailed property parsing. - Self-Contained Deployment: The entire sample is designed for easy, self-contained deployment using Docker Compose, with all necessary code and configuration included. Initial GCP deployment scripts and
ngrok
integration are also provided for public access.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, self-contained sample for a real-estate agent. The architecture is well-defined. I've identified several areas for improvement, primarily focusing on enhancing security, correctness, and maintainability. Key points include:
- Security: The API key authentication in the Python agent needs to be hardened against timing attacks and server misconfigurations.
- Correctness: There are several places in the Python agent where unsafe data access and lack of error handling for JSON parsing could lead to runtime crashes. The deployment scripts also have a critical issue that would prevent the agent from receiving its public URL. The web scraping throttling logic in the Node.js service is flawed and could lead to being blocked.
- Maintainability: I've suggested improvements like using multi-stage Docker builds to reduce image size, pinning dependency versions for reproducible builds, and improving logging and exception handling practices.
Addressing the identified issues will significantly improve the robustness and security of the sample.
real-estate-agent: | ||
build: . | ||
ports: | ||
- "3001:3001" | ||
environment: | ||
- MCP_SERVER_URL=http://dafty-mcp:4000/sse | ||
depends_on: | ||
- dafty-mcp | ||
networks: | ||
- app_net |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real-estate-agent
service relies on environment variables from a .env
file, which is updated at runtime by the GCP startup script to include the dynamic AGENT_PUBLIC_URL
from ngrok. However, the service definition is missing the env_file
property. Without it, the container will not pick up the variables from the .env
file on the host.
real-estate-agent:
build: .
ports:
- "3001:3001"
env_file:
- ./.env
environment:
- MCP_SERVER_URL=http://dafty-mcp:4000/sse
depends_on:
- dafty-mcp
networks:
- app_net
api_key = request.headers.get("authorization") | ||
if not api_key or api_key != f"Bearer {os.environ.get('API_KEY')}": | ||
return Response(status_code=401, content="Unauthorized") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current API key authentication has two critical security vulnerabilities:
- Timing Attacks: The direct string comparison is vulnerable to timing attacks.
- Server Misconfiguration: If the
API_KEY
environment variable is not set,os.environ.get('API_KEY')
returnsNone
, and the check becomesapi_key != "Bearer None"
. An attacker could potentially bypass authentication by sendingAuthorization: Bearer None
.
You should use secrets.compare_digest
for a constant-time comparison to prevent timing attacks and handle the case where the API key is not configured on the server.
api_key = request.headers.get("authorization") | |
if not api_key or api_key != f"Bearer {os.environ.get('API_KEY')}": | |
return Response(status_code=401, content="Unauthorized") | |
api_key_header = request.headers.get("authorization") | |
expected_api_key = os.environ.get("API_KEY") | |
if not expected_api_key: | |
# This is a server configuration error. Log it and return an error. | |
return Response(status_code=500, content="Internal Server Error: API Key not configured.") | |
if not api_key_header or not api_key_header.startswith("Bearer "): | |
return Response(status_code=401, content="Unauthorized") | |
submitted_key = api_key_header.removeprefix("Bearer ") | |
import secrets | |
if not secrets.compare_digest(submitted_key, expected_api_key): | |
return Response(status_code=401, content="Unauthorized") |
const detailedProperties = await Promise.all( | ||
propertiesOnPage.map(async (p) => { | ||
if (!p.url) return []; | ||
await delay(500); // Delay between detail page fetches | ||
const detailHtml = await fetchPageHTML(p.url, 0); // 0 indicates a detail page | ||
const enhancedProperty = parsePropertyDetails(detailHtml, p); | ||
if (enhancedProperty.units && enhancedProperty.units.length > 0) { | ||
return enhancedProperty.units as Property[]; | ||
} | ||
return [enhancedProperty as Property]; | ||
}) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of fetching detailed properties using Promise.all
with map
is flawed. The map
function will create all promises at once, and each promise will then independently delay
for 500ms before fetching. This means all fetch requests will be sent concurrently after the initial delay, which defeats the purpose of throttling and could lead to being rate-limited or blocked by the server. Use a for...of
loop to properly throttle the requests.
const detailedProperties = await Promise.all( | |
propertiesOnPage.map(async (p) => { | |
if (!p.url) return []; | |
await delay(500); // Delay between detail page fetches | |
const detailHtml = await fetchPageHTML(p.url, 0); // 0 indicates a detail page | |
const enhancedProperty = parsePropertyDetails(detailHtml, p); | |
if (enhancedProperty.units && enhancedProperty.units.length > 0) { | |
return enhancedProperty.units as Property[]; | |
} | |
return [enhancedProperty as Property]; | |
}) | |
); | |
const detailedPropertiesPromises = propertiesOnPage.map(async (p) => { | |
if (!p.url) return []; | |
await delay(500); // Delay between detail page fetches | |
const detailHtml = await fetchPageHTML(p.url, 0); // 0 indicates a detail page | |
const enhancedProperty = parsePropertyDetails(detailHtml, p); | |
if (enhancedProperty.units && enhancedProperty.units.length > 0) { | |
return enhancedProperty.units as Property[]; | |
} | |
return [enhancedProperty as Property]; | |
}); | |
const detailedProperties = []; | |
for (const promise of detailedPropertiesPromises) { | |
detailedProperties.push(...(await promise)); | |
} |
) -> None: | ||
try: | ||
await self.mcp_client.connect() | ||
query = request.message.parts[0].root.text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code directly accesses request.message.parts[0]
without checking if the parts
list is empty. This could lead to an IndexError
if the agent receives a request with no message parts.
query = request.message.parts[0].root.text | |
if not request.message.parts or not isinstance(request.message.parts[0].root, TextContent): | |
await event_queue.enqueue_event(new_agent_text_message("Invalid request: message is empty or not text.")) | |
return | |
query = request.message.parts[0].root.text |
filters = json.loads(parsed_params_parts[0].text) | ||
|
||
result_parts = await self.mcp_client.search_rental_properties(filters) | ||
|
||
if not result_parts or not isinstance(result_parts[0], TextContent): | ||
await event_queue.enqueue_event(new_agent_text_message("No results found.")) | ||
return | ||
|
||
# The actual data is a JSON string inside the first text part | ||
properties = json.loads(result_parts[0].text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code calls json.loads()
on text content received from both the LLM (parse_query
tool) and the search_rental_properties
tool. These external services might not always return valid JSON, which would cause json.JSONDecodeError
. Wrap both json.loads()
calls in try...except json.JSONDecodeError
blocks.
filters = json.loads(parsed_params_parts[0].text) | |
result_parts = await self.mcp_client.search_rental_properties(filters) | |
if not result_parts or not isinstance(result_parts[0], TextContent): | |
await event_queue.enqueue_event(new_agent_text_message("No results found.")) | |
return | |
# The actual data is a JSON string inside the first text part | |
properties = json.loads(result_parts[0].text) | |
try: | |
filters = json.loads(parsed_params_parts[0].text) | |
except json.JSONDecodeError: | |
await event_queue.enqueue_event(new_agent_text_message("Error: Could not parse the query parameters.")) | |
return | |
result_parts = await self.mcp_client.search_rental_properties(filters) | |
if not result_parts or not isinstance(result_parts[0], TextContent): | |
await event_queue.enqueue_event(new_agent_text_message("No results found.")) | |
return | |
# The actual data is a JSON string inside the first text part | |
try: | |
properties = json.loads(result_parts[0].text) | |
except json.JSONDecodeError: | |
await event_queue.enqueue_event(new_agent_text_message("Error: Could not parse the property results.")) | |
return |
sudo chown -R app-runner:app-runner /home/app-runner/agent | ||
|
||
# Install Docker Compose v2. | ||
sudo curl -L "https://github.com/docker/compose/releases/download/v2.24.5/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script hardcodes the Docker Compose version v2.24.5
. It's better to dynamically fetch the latest version number from the GitHub API.
sudo curl -L "https://github.com/docker/compose/releases/download/v2.24.5/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose | |
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | grep 'tag_name' | cut -d\" -f4) | |
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose |
ENV_FILE="${BASE_PATH}/.env" | ||
|
||
# Update the .env file with the correct path for the dafty-mcp project on the VM. | ||
sed -i "s#^DAFTY_MCP_PATH=.*#DAFTY_MCP_PATH=${BASE_PATH}/dafty-mcp#" ${ENV_FILE} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
ollama: | ||
image: ollama/ollama | ||
entrypoint: ["/bin/sh", "-c", "ollama serve > /dev/null 2>&1"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ollama
service entrypoint redirects all standard output and standard error to /dev/null
. This will suppress all logs from the Ollama service, making it very difficult to debug any potential issues with the LLM service. Remove the redirection and let Docker's logging driver handle the output.
entrypoint: ["/bin/sh", "-c", "ollama serve"]
if result.isError: | ||
error_message = f"Tool call for '{tool_name}' failed." | ||
if result.content and isinstance(result.content[0], types.TextContent): | ||
error_message += f" Reason: {result.content[0].text}" | ||
raise Exception(error_message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a2a-sdk | ||
python-dotenv | ||
uvicorn | ||
mcp No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, self-contained sample for a real-estate agent. The sample demonstrates an agent capable of understanding natural language queries to search for rental properties. It is composed of three services orchestrated with Docker Compose: a core Python A2A agent, a Node.js MCP tool server, and a local LLM service. I've identified a few areas for improvement, primarily concerning dependency management, container security, and a code correctness issue. Addressing the feedback will enhance its quality and robustness.
await event_queue.enqueue_event(new_agent_text_message("No properties matched your criteria.")) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appears to be an indentation error here. The if not properties:
block is indented one level too deep, which will cause a Python IndentationError
at runtime. This block should be at the same level as the try...except
block above it.
await event_queue.enqueue_event(new_agent_text_message("No properties matched your criteria.")) | |
return | |
if not properties: | |
await event_queue.enqueue_event(new_agent_text_message("No properties matched your criteria.")) | |
return |
"axios": "^1.9.0", | ||
"cheerio": "^1.0.0-rc.12", | ||
"dotenv": "^16.5.0", | ||
"llamaindex": "^0.1.15", | ||
"winston": "^3.17.0", | ||
"zod": "^3.25.51" | ||
}, | ||
"devDependencies": { | ||
"@types/cheerio": "^0.22.35", | ||
"@types/express": "^4.17.13", | ||
"@types/node": "^22.5.2", | ||
"@typescript-eslint/eslint-plugin": "^8.34.1", | ||
"@typescript-eslint/parser": "^8.34.1", | ||
"@vitest/coverage-v8": "^3.2.4", | ||
"eslint": "^9.29.0", | ||
"eslint-config-prettier": "^10.1.5", | ||
"eslint-plugin-prettier": "^5.5.0", | ||
"prettier": "^3.5.3", | ||
"typescript": "^5.8.3", | ||
"typescript-eslint": "^8.34.1", | ||
"vitest": "^3.2.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several of the dependency versions listed here appear to be incorrect or do not exist in the public npm registry. For example:
axios
:^1.9.0
(latest is^1.7.2
)zod
:^3.25.51
(latest is^3.23.8
)@typescript-eslint/eslint-plugin
:^8.34.1
(latest is^7.13.0
)vitest
:^3.2.3
(latest is^1.6.0
, there is no v3)
These incorrect versions will cause npm install
to fail. Please verify and correct the versions for all dependencies to ensure the project can be set up correctly.
authors = ["Roo"] | ||
|
||
[tool.poetry.dependencies] | ||
python = "^3.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a version mismatch between the Python version specified here (^3.10
) and the one used in the Dockerfile
(python:3.13-slim
). This can lead to dependency resolution issues or runtime errors if code relies on features from a newer Python version.
Please align the versions to ensure consistency. Given the Dockerfile, ^3.13
would be appropriate here.
python = "^3.13"
COPY . . | ||
|
||
# Your app binds to port 3001, so expose that | ||
EXPOSE 3001 | ||
|
||
# Define the command to run your app | ||
CMD ["uvicorn", "main:app", "--host", "0.0.0.0", "--port", "3001"] No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For security best practices, it's recommended to run the container as a non-root user. The base python:3.13-slim
image runs as root by default. You should create a non-root user and switch to it before running the application.
COPY . .
# Create a non-root user and switch to it
RUN useradd --create-home appuser
USER appuser
# Your app binds to port 3001, so expose that
EXPOSE 3001
# Define the command to run your app
CMD ["uvicorn", "main:app", "--host", "0.0.0.0", "--port", "3001"]
build/ | ||
.env | ||
.session | ||
package-lock.json No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a best practice to commit the package-lock.json
file. This ensures that all developers and CI/CD environments use the exact same dependency versions, leading to more reproducible and stable builds. Ignoring it can lead to situations where different environments install slightly different package versions, potentially causing hard-to-debug issues.
COPY --from=builder /usr/src/app/build ./build | ||
|
||
# Your app binds to port 4000, so expose that | ||
EXPOSE 4000 | ||
|
||
# Define the command to run your app | ||
CMD [ "npm", "start" ] No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For enhanced security, it's a best practice to run containers as a non-root user. The node:18-alpine
image runs as root
by default, but it includes a non-root node
user. You should switch to this user before starting the application.
COPY --from=builder /usr/src/app/build ./build
# Switch to the non-root user
USER node
# Your app binds to port 4000, so expose that
EXPOSE 4000
# Define the command to run your app
CMD [ "npm", "start" ]
sudo -u app-runner /usr/local/bin/ngrok config add-authtoken $1 | ||
|
||
# Create systemd service for ngrok. | ||
sudo bash -c 'cat > /etc/systemd/system/ngrok.service <<EOF | ||
[Unit] | ||
Description=ngrok tunnel | ||
After=network.target | ||
|
||
[Service] | ||
User=app-runner | ||
ExecStart=/usr/local/bin/ngrok http 3001 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script installs ngrok
via apt-get
, which typically places the binary in /usr/bin
. However, the script later refers to it at /usr/local/bin/ngrok
. This path mismatch will likely cause the script to fail when trying to configure the authtoken and when the systemd service tries to start.
It's more robust to rely on the system's PATH
by calling ngrok
directly, or by using $(which ngrok)
if you need the absolute path.
sudo -u app-runner /usr/local/bin/ngrok config add-authtoken $1 | |
# Create systemd service for ngrok. | |
sudo bash -c 'cat > /etc/systemd/system/ngrok.service <<EOF | |
[Unit] | |
Description=ngrok tunnel | |
After=network.target | |
[Service] | |
User=app-runner | |
ExecStart=/usr/local/bin/ngrok http 3001 | |
sudo -u app-runner ngrok config add-authtoken $1 | |
# Create systemd service for ngrok. | |
sudo bash -c 'cat > /etc/systemd/system/ngrok.service <<EOF | |
[Unit] | |
Description=ngrok tunnel | |
After=network.target | |
[Service] | |
User=app-runner | |
ExecStart=ngrok http 3001 |
async def search_rental_properties(self, args: dict) -> list[types.ContentBlock]: | ||
if not self.session: | ||
raise RuntimeError("Client not connected") | ||
|
||
result = await self.session.call_tool( | ||
"search_rental_properties", | ||
arguments=args, | ||
) | ||
|
||
if result.isError: | ||
error_message = "Tool call failed." | ||
if result.content and isinstance(result.content[0], types.TextContent): | ||
error_message += f" Reason: {result.content[0].text}" | ||
raise RuntimeError(error_message) | ||
|
||
return result.content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The search_rental_properties
method contains logic that is very similar to the generic call_tool
method. To reduce code duplication and improve maintainability, you can refactor this method to simply call call_tool
with the specific tool name.
async def search_rental_properties(self, args: dict) -> list[types.ContentBlock]:
return await self.call_tool("search_rental_properties", args)
class ApiKeyAuthMiddleware(BaseHTTPMiddleware): | ||
async def dispatch(self, request: Request, call_next): | ||
if request.url.path.startswith("/a2a/") and request.url.path != "/.well-known/agent.json": | ||
import secrets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to PEP 8, imports should be at the top of the file. Importing secrets
inside the dispatch
method is unconventional and incurs a small performance overhead on each call. Please move the import to the top of the file.
import secrets | |
import os | |
import secrets | |
from starlette.middleware.base import BaseHTTPMiddleware | |
from starlette.requests import Request | |
from starlette.responses import Response | |
class ApiKeyAuthMiddleware(BaseHTTPMiddleware): | |
async def dispatch(self, request: Request, call_next): | |
if request.url.path.startswith("/a2a/") and request.url.path != "/.well-known/agent.json": |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request introduces a new real-estate agent sample with a well-structured architecture and good coding practices. The review focuses on enhancing robustness and maintainability by suggesting improvements to shell scripts, Docker image optimization, code style, and logging strategy.
const timestampStr = new Date().toISOString().replace(/:/g, '-').replace(/\..+/, ''); | ||
const logFileName = `${timestampStr}_daft_mcp.log`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current logging configuration creates a new log file with a timestamp in its name every time the server starts. For a long-running service, this can lead to a large number of log files accumulating and consuming significant disk space over time.
Consider using a fixed log file name (e.g., daft_mcp.log
) and implementing a log rotation strategy. Winston's winston-daily-rotate-file
transport is an excellent option for managing log files by size, date, and retention period.
sudo chown -R app-runner:app-runner /home/app-runner/agent | ||
|
||
# Install Docker Compose v2. | ||
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | grep 'tag_name' | cut -d\\\" -f4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing JSON with grep
and cut
is fragile and can break if the API response formatting changes. A more robust method should be used to extract the version tag.
To avoid adding new dependencies, you could use awk
for more reliable parsing.
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | grep 'tag_name' | cut -d\\\" -f4) | |
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | grep '"tag_name"' | awk -F '"' '{print $4}') |
# before this script starts, so we can proceed directly. | ||
|
||
# Get the public URL from the ngrok API. | ||
NGROK_URL=$(curl -s http://localhost:4040/api/tunnels | grep -o 'https://[a-zA-Z0-9.-]*\.ngrok-free.app') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using grep
with a regex to parse the ngrok URL is brittle and might fail if ngrok changes its domain or the API response structure. The ngrok agent API returns JSON, so a more robust parsing method is recommended.
If jq
is available on the VM, it would be a much more reliable way to extract the public URL.
NGROK_URL=$(curl -s http://localhost:4040/api/tunnels | grep -o 'https://[a-zA-Z0-9.-]*\.ngrok-free.app') | |
NGROK_URL=$(curl -s http://localhost:4040/api/tunnels | jq -r '.tunnels[] | select(.proto=="https") | .public_url') |
code.diff
Outdated
if request.url.path.startswith("/a2a/") and request.url.path != "/.well-known/agent.json": | ||
- api_key = request.headers.get("authorization") | ||
- if not api_key or api_key != f"Bearer {os.environ.get('API_KEY')}": | ||
+ import secrets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to PEP 8, imports should be at the top of the file, just after any module comments and docstrings, and before module globals and constants. Placing import secrets
inside the dispatch
method is not standard practice.
Move this import to the top of the file with the other imports.
import secrets
from starlette.middleware.base import BaseHTTPMiddleware
from starlette.requests import Request
from starlette.responses import Response
class ApiKeyAuthMiddleware(BaseHTTPMiddleware):
async def dispatch(self, request: Request, call_next):
if request.url.path.startswith("/a2a/") and request.url.path != "/.well-known/agent.json":
api_key_header = request.headers.get("authorization")
expected_api_key = os.environ.get("API_KEY")
if not expected_api_key:
# This is a server configuration error. Log it and return an error.
return Response(status_code=500, content="Internal Server Error: API Key not configured.")
if not api_key_header or not api_key_header.startswith("Bearer "):
return Response(status_code=401, content="Unauthorized")
submitted_key = api_key_header.removeprefix("Bearer ")
if not secrets.compare_digest(submitted_key, expected_api_key):
return Response(status_code=401, content="Unauthorized")
response = await call_next(request)
return response
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a comprehensive and well-structured real-estate agent sample. The use of Docker Compose to orchestrate the three services (Python agent, Node.js MCP server, and Ollama) makes it a self-contained and easy-to-run example. The code quality is generally high, with excellent practices demonstrated throughout, such as multi-stage Docker builds, security hardening (non-root users, secrets.compare_digest
), robust error handling with custom error types, extensive testing, and clear project structure.
My review has identified a critical issue in the Node.js server's handling of SSE connections that would prevent concurrent client usage. I have also noted a couple of medium-severity issues: one concerning the robustness of a deployment script and another related to Python import conventions.
Overall, this is a fantastic contribution that showcases a complex agent interaction. The suggested changes will help improve its robustness and adherence to best practices.
app.get('/sse', (req: Request, res: Response) => { | ||
transport = new SSEServerTransport('/message', res); | ||
server.connect(transport); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation for handling SSE transport appears to support only a single client connection at a time. The transport
variable is declared in the startServer
scope and is overwritten each time a new client connects to the /sse
endpoint. This will lead to unpredictable behavior and dropped connections if multiple clients attempt to use the agent concurrently.
To handle multiple clients correctly, you should manage a collection of transport instances, for example, using a Map
keyed by a unique session or client identifier. Each incoming request to /sse
should create a new transport instance and add it to the collection, and it should be removed when the connection is closed.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a well-structured and comprehensive sample for a real-estate agent. The use of Docker Compose to orchestrate the different services is excellent, and the code is generally clean and well-documented. I've provided some feedback focused on improving robustness, consistency, and type safety, particularly within the dafty-mcp
service and the deployment scripts. Overall, this is a high-quality contribution.
EXPOSE 3001 | ||
|
||
# Define the command to run your app | ||
CMD ["uvicorn", "main:app", "--host", "0.0.0.0", "--port", "3001"] No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a slight inconsistency in the host binding configuration. The main.py
file and CHANGELOG.md
mention binding to ::
(all IPv6 and usually IPv4 addresses), but this Dockerfile's CMD
specifies 0.0.0.0
(all IPv4 addresses).
To maintain consistency and ensure the service behaves as documented (especially since the docker-compose.yml
enables IPv6 for the network), it would be better to use ::
here as well.
CMD ["uvicorn", "main:app", "--host", "::", "--port", "3001"]
// This is a simplified approach. In a real-world scenario, you'd | ||
// need a way to route the message to the correct client's transport. | ||
// For this example, we'll just use the first available one. | ||
const transport = this.clients.values().next().value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation for handling POST messages in SseManager
retrieves the first available transport, which could lead to incorrect routing in a scenario with multiple clients. While this works for the current single-client sample, it's not robust.
A more scalable approach would be to identify the correct client transport to route the message to, perhaps using a client ID or another unique identifier from the request if the protocol supports it. This would prevent potential race conditions or misrouted messages if the service is ever used with more than one simultaneous client.
try { | ||
// This endpoint is a placeholder from the original code. | ||
// Consult Daft.ie API docs for the actual details endpoint if using the API. | ||
const response = await daftApi.get(`/listings/${property_id}`); | ||
logger.info(`[daftApi.service.ts] API call for property ID ${property_id} successful. Status: ${response.status}`); | ||
return { | ||
content: [ | ||
{ | ||
type: 'text', | ||
text: JSON.stringify(response.data, null, 2), | ||
}, | ||
], | ||
}; | ||
} catch (error) { | ||
let errorMessage = `Error fetching details for property ID "${property_id}" from Daft.ie API`; | ||
if (axios.isAxiosError(error)) { | ||
errorMessage = `Daft.ie API error for property ID "${property_id}": ${error.response?.data?.message || error.message}. Status: ${error.response?.status}`; | ||
} else if (error instanceof Error) { | ||
errorMessage = `Generic error for property ID "${property_id}" using Daft.ie API: ${error.message}`; | ||
} | ||
logger.error(`[daftApi.service.ts] ${errorMessage}`); | ||
return { | ||
content: [ | ||
{ | ||
type: 'text', | ||
text: errorMessage, | ||
}, | ||
], | ||
isError: true, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling in this function is inconsistent with the pattern used elsewhere in the application. Here, errors are caught and formatted into a response object, which is then returned. However, other services (like daftScraper.service.ts
) throw custom errors that are caught and handled centrally by createErrorResponse
in server.ts
.
To improve consistency and centralize error handling, this function should throw a custom ApiError
instead of catching it. This will allow the catch
block in server.ts
to handle it uniformly with other tool errors.
try {
// This endpoint is a placeholder from the original code.
// Consult Daft.ie API docs for the actual details endpoint if using the API.
const response = await daftApi.get(`/listings/${property_id}`);
logger.info(`[daftApi.service.ts] API call for property ID ${property_id} successful. Status: ${response.status}`);
return {
content: [
{
type: 'text',
text: JSON.stringify(response.data, null, 2),
},
],
};
} catch (error) {
let errorMessage = `Error fetching details for property ID "${property_id}" from Daft.ie API`;
if (axios.isAxiosError(error)) {
errorMessage = `Daft.ie API error for property ID "${property_id}": ${error.response?.data?.message || error.message}. Status: ${error.response?.status}`;
} else if (error instanceof Error) {
errorMessage = `Generic error for property ID "${property_id}" using Daft.ie API: ${error.message}`;
}
logger.error(`[daftApi.service.ts] ${errorMessage}`);
// Throw an error to be caught by the centralized handler in server.ts
throw new ApiError(errorMessage);
}
// "erasableSyntaxOnly": true, /* Do not allow runtime constructs that are not part of ECMAScript. */ | ||
// "allowSyntheticDefaultImports": true, /* Allow 'import x from y' when a module doesn't have a default export. */ | ||
"esModuleInterop": true, /* Emit additional JavaScript to ease support for importing CommonJS modules. This enables 'allowSyntheticDefaultImports' for type compatibility. */ | ||
"allowSyntheticDefaultImports": true, /* Allow 'import x from y' when a module doesn't have a default export. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | grep '\"tag_name\"' | awk -F '\"' '{print $4}') | ||
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing JSON from the GitHub API using grep
and awk
is fragile and can break if the JSON structure changes. Since jq
is installed later in the script, a more robust approach would be to install jq
first and then use it to reliably parse the tag_name
.
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | grep '\"tag_name\"' | awk -F '\"' '{print $4}') | |
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose | |
sudo apt-get update | |
sudo apt-get install -y ngrok jq | |
# Install Docker Compose v2. | |
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | jq -r '.tag_name') | |
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose |
samples/python/agents/real_estate_agent/deployment/gcp/gcp_startup_service.sh
Outdated
Show resolved
Hide resolved
class DaftyMcpClient: | ||
def __init__(self): | ||
self.session: ClientSession | None = None | ||
self._transport_cm = None | ||
self._session_cm = None | ||
|
||
async def connect(self): | ||
if self.session: | ||
return | ||
|
||
url = os.environ.get("MCP_SERVER_URL") | ||
if not url: | ||
raise ValueError("MCP_SERVER_URL environment variable not set") | ||
|
||
headers = {} | ||
api_key = os.environ.get("DAFT_API_KEY") | ||
if api_key: | ||
headers["X-API-Key"] = api_key | ||
|
||
self._transport_cm = sse_client(url, headers=headers) | ||
read_stream, write_stream = await self._transport_cm.__aenter__() | ||
|
||
self._session_cm = ClientSession(read_stream, write_stream) | ||
self.session = await self._session_cm.__aenter__() | ||
await self.session.initialize() | ||
|
||
async def search_rental_properties(self, args: dict) -> list[types.ContentBlock]: | ||
return await self.call_tool("search_rental_properties", args) | ||
|
||
async def call_tool(self, tool_name: str, args: dict) -> list[types.ContentBlock]: | ||
if not self.session: | ||
raise RuntimeError("Client not connected") | ||
|
||
result = await self.session.call_tool( | ||
tool_name, | ||
arguments=args, | ||
) | ||
|
||
if result.isError: | ||
error_message = f"Tool call for '{tool_name}' failed." | ||
if result.content and isinstance(result.content[0], types.TextContent): | ||
error_message += f" Reason: {result.content[0].text}" | ||
raise RuntimeError(error_message) | ||
|
||
return result.content | ||
|
||
async def disconnect(self): | ||
if self._session_cm: | ||
await self._session_cm.__aexit__(None, None, None) | ||
self.session = None | ||
self._session_cm = None | ||
if self._transport_cm: | ||
await self._transport_cm.__aexit__(None, None, None) | ||
self._transport_cm = None No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manual management of the async context managers (__aenter__
and __aexit__
) for the session and transport is a bit verbose and less idiomatic than using an async with
block.
Consider refactoring DaftyMcpClient
to be an async context manager itself. This would simplify the resource management in the RealEstateAgentExecutor
and make the code more aligned with Python best practices.
import os
from mcp.client.session import ClientSession
from mcp.client.sse import sse_client
import mcp.types as types
class DaftyMcpClient:
def __init__(self):
self.session: ClientSession | None = None
self._transport_cm = None
self._session_cm = None
async def __aenter__(self):
await self.connect()
return self
async def __aexit__(self, exc_type, exc_val, exc_tb):
await self.disconnect()
async def connect(self):
if self.session:
return
url = os.environ.get("MCP_SERVER_URL")
if not url:
raise ValueError("MCP_SERVER_URL environment variable not set")
headers = {}
api_key = os.environ.get("DAFT_API_KEY")
if api_key:
headers["X-API-Key"] = api_key
self._transport_cm = sse_client(url, headers=headers)
read_stream, write_stream = await self._transport_cm.__aenter__()
self._session_cm = ClientSession(read_stream, write_stream)
self.session = await self._session_cm.__aenter__()
await self.session.initialize()
async def search_rental_properties(self, args: dict) -> list[types.ContentBlock]:
return await self.call_tool("search_rental_properties", args)
async def call_tool(self, tool_name: str, args: dict) -> list[types.ContentBlock]:
if not self.session:
raise RuntimeError("Client not connected")
result = await self.session.call_tool(
tool_name,
arguments=args,
)
if result.isError:
error_message = f"Tool call for '{tool_name}' failed."
if result.content and isinstance(result.content[0], types.TextContent):
error_message += f" Reason: {result.content[0].text}"
raise RuntimeError(error_message)
return result.content
async def disconnect(self):
if self._session_cm:
await self._session_cm.__aexit__(None, None, None)
self.session = None
self._session_cm = None
if self._transport_cm:
await self._transport_cm.__aexit__(None, None, None)
self._transport_cm = None
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, well-architected real-estate agent sample, demonstrating strong engineering practices and modern tooling. My review provides feedback to further enhance its robustness, reproducibility, and efficiency, including improvements to Docker builds, SSE handling, and deployment scripts. Overall, this is a high-quality contribution.
handlePost(req: Request, res: Response) { | ||
// This is a simplified approach. In a real-world scenario, you'd | ||
// need a way to route the message to the correct client's transport. | ||
// For this example, we'll just use the first available one. | ||
const transport = this.clients.values().next().value; | ||
if (transport) { | ||
transport.handlePostMessage(req, res, req.body); | ||
} else { | ||
res.status(400).send('No active SSE transport to handle message'); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handlePost
method in SseManager
is not robust for handling multiple concurrent clients. It currently retrieves the first available transport using this.clients.values().next().value
. In a scenario with multiple connected agents, this will cause messages to be routed to an arbitrary client, likely not the one that sent the request, leading to incorrect behavior or silent failures.
While the comment acknowledges this is a simplified approach, it's a significant correctness issue. The implementation should route the incoming message to the specific SSEServerTransport
instance associated with the client that sent the message. This typically requires a client identifier to be part of the request, which can then be used to look up the correct transport in the clients
map.
COPY package*.json ./ | ||
|
||
# Install all dependencies and build the TypeScript source | ||
RUN npm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more reproducible builds, it's recommended to use npm ci
instead of npm install
. npm ci
installs dependencies directly from the package-lock.json
file, ensuring that the exact same dependency versions are used every time. This requires that package-lock.json
is committed to the repository.
RUN npm ci
samples/python/agents/real_estate_agent/dafty-mcp/src/services/queryParser.service.ts
Outdated
Show resolved
Hide resolved
# Start all docker-compose services using absolute paths for all files. | ||
/usr/local/bin/docker-compose --project-directory ${BASE_PATH} --env-file ${ENV_FILE} -f ${COMPOSE_FILE} up --build -d | ||
|
||
# The ngrok service is managed by systemd and is guaranteed to be running | ||
# before this script starts, so we can proceed directly. | ||
|
||
# Get the public URL from the ngrok API. | ||
NGROK_URL=$(curl -s http://localhost:4040/api/tunnels | jq -r '.tunnels[] | select(.proto=="https") | .public_url') | ||
|
||
if [ -z "$NGROK_URL" ]; then | ||
echo "Failed to retrieve ngrok URL. Startup aborted." >&2 | ||
exit 1 | ||
fi | ||
|
||
# Update the .env file with the new dynamic URL using an absolute path. | ||
sed -i "s#^AGENT_PUBLIC_URL=.*#AGENT_PUBLIC_URL=${NGROK_URL}/#" ${ENV_FILE} | ||
|
||
# Force the real-estate-agent container to be recreated, using absolute paths. | ||
/usr/local/bin/docker-compose --project-directory ${BASE_PATH} --env-file ${ENV_FILE} -f ${COMPOSE_FILE} up -d --force-recreate real-estate-agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current startup sequence starts all services with docker-compose up
, then updates the .env
file with the ngrok URL, and finally forces a recreation of the real-estate-agent
container. This is inefficient as it starts the agent container twice.
A more efficient approach would be to get the ngrok URL before starting the services. This would allow the .env
file to be correctly configured from the beginning, so docker-compose up
only needs to be run once.
Consider restructuring the script to:
- Wait for Docker.
- Wait for ngrok and get the public URL.
- Update the
.env
file with the URL. - Run
docker-compose up --build -d
once for all services.
This will improve startup time and reduce unnecessary container restarts.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, comprehensive sample for a real-estate agent. The sample is well-structured, using Docker Compose to orchestrate a Python agent, a Node.js tool server, and an Ollama LLM service.
The code quality is very high across the board. It demonstrates excellent practices such as:
- Using multi-stage Docker builds and non-root users.
- Robust error handling with custom error types and
try...catch
blocks. - Secure authentication using
secrets.compare_digest
. - Clear documentation and helpful setup scripts.
- Well-structured and modular code in both the Python and TypeScript services.
I have one suggestion to improve the robustness of the GCP deployment script by using jq
to parse the Docker Compose version, which makes it less prone to breaking from API changes.
Overall, this is an excellent addition and a great example for other developers.
# Install Docker Compose v2. | ||
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | grep '\"tag_name\"' | awk -F '\"' '{print $4}') | ||
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose | ||
sudo chmod +x /usr/local/bin/docker-compose | ||
|
||
# Install ngrok and jq. | ||
curl -s https://ngrok-agent.s3.amazonaws.com/ngrok.asc | sudo tee /etc/apt/trusted.gpg.d/ngrok.asc >/dev/null | ||
echo "deb https://ngrok-agent.s3.amazonaws.com buster main" | sudo tee /etc/apt/sources.list.d/ngrok.list | ||
sudo apt-get update | ||
sudo apt-get install -y ngrok jq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current method of fetching the latest Docker Compose version is a bit fragile as it relies on grep
and awk
to parse JSON from the GitHub API. This can easily break if the API response format changes.
Since jq
is already being installed, you could use it for a more robust and readable way to parse the version. This would require installing jq
before you install Docker Compose.
Here's a suggested reordering and improvement:
# Install Docker Compose v2. | |
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | grep '\"tag_name\"' | awk -F '\"' '{print $4}') | |
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose | |
sudo chmod +x /usr/local/bin/docker-compose | |
# Install ngrok and jq. | |
curl -s https://ngrok-agent.s3.amazonaws.com/ngrok.asc | sudo tee /etc/apt/trusted.gpg.d/ngrok.asc >/dev/null | |
echo "deb https://ngrok-agent.s3.amazonaws.com buster main" | sudo tee /etc/apt/sources.list.d/ngrok.list | |
sudo apt-get update | |
sudo apt-get install -y ngrok jq | |
# Install ngrok and jq. | |
curl -s https://ngrok-agent.s3.amazonaws.com/ngrok.asc | sudo tee /etc/apt/trusted.gpg.d/ngrok.asc >/dev/null | |
echo "deb https://ngrok-agent.s3.amazonaws.com buster main" | sudo tee /etc/apt/sources.list.d/ngrok.list | |
sudo apt-get update | |
sudo apt-get install -y ngrok jq | |
# Install Docker Compose v2. | |
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | jq -r .tag_name) | |
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose | |
sudo chmod +x /usr/local/bin/docker-compose |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a well-structured real-estate agent sample with a clean architecture and high-quality code. I've identified a couple of medium-severity maintainability improvements related to dependency management and Docker volume usage. Overall, this is an excellent addition.
dafty-mcp: | ||
build: ./dafty-mcp | ||
volumes: | ||
- ./dafty-mcp/.session:/usr/src/app/.session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[tool.poetry] | ||
name = "real-estate-agent" | ||
version = "0.1.0" | ||
description = "A2A Agent for finding real estate properties" | ||
authors = ["Roo"] | ||
|
||
[tool.poetry.dependencies] | ||
python = "^3.13" | ||
a2a-sdk = "^0.2.0" | ||
python-dotenv = "^1.0.0" | ||
uvicorn = "^0.22.0" | ||
|
||
[tool.poetry.dev-dependencies] | ||
pytest = "^7.4" | ||
httpx = "^0.24.1" | ||
|
||
[build-system] | ||
requires = ["poetry-core>=1.0.0"] | ||
build-backend = "poetry.core.masonry.api" No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new real-estate-agent sample, including a core Python A2A agent, a Node.js MCP tool server, and a local LLM service. The changes include Dockerfile configurations, service implementations, and necessary dependencies. The code appears well-structured and includes error handling, but there are opportunities to improve error message context and URL construction.
const message = error instanceof Error ? error.message : 'An unknown error occurred during scraping.'; | ||
throw new ScraperError(message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current error handling only creates a ScraperError
with a generic message. It would be more helpful to propagate the original error message to provide more context for debugging. Consider including the original error message in the ScraperError
.
const message = error instanceof Error ? error.message : 'An unknown error occurred during scraping.'; | |
throw new ScraperError(message); | |
throw new ScraperError(`Scraping failed: ${message}`); |
samples/python/agents/real_estate_agent/dafty-mcp/src/services/daftScraper.service.ts
Show resolved
Hide resolved
samples/python/agents/real_estate_agent/dafty-mcp/src/services/daftApi.service.ts
Show resolved
Hide resolved
samples/python/agents/real_estate_agent/dafty-mcp/src/services/queryParser.service.ts
Outdated
Show resolved
Hide resolved
const message = error instanceof Error ? error.message : 'An unknown error occurred during query parsing.'; | ||
throw new ScraperError(message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current error handling only creates a ScraperError
with a generic message. It would be more helpful to propagate the original error message to provide more context for debugging. Consider including the original error message in the ScraperError
.
const message = error instanceof Error ? error.message : 'An unknown error occurred during query parsing.'; | |
throw new ScraperError(message); | |
throw new ScraperError(`LLM query parsing failed: ${message}`); |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request introduces a real-estate agent sample with a Python agent, Node.js tool server, and LLM service orchestrated using Docker. The architecture exhibits a clear separation of concerns and employs modern technologies. Critical issues identified include duplicate user creation in the Dockerfile, potential rate-limiting in the TypeScript scraper, missing environment variable configuration in the GCP deployment script, and logical errors in the Python agent's executor. Addressing these issues will significantly enhance the sample's functionality and robustness.
# Create a non-root user and switch to it | ||
RUN useradd --create-home appuser | ||
USER appuser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Force the real-estate-agent container to be recreated, using absolute paths. | ||
/usr/local/bin/docker-compose --project-directory ${BASE_PATH} --env-file ${ENV_FILE} -f ${COMPOSE_FILE} up -d --force-recreate real-estate-agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script correctly retrieves the public NGROK_URL
, but it doesn't pass this URL to the real-estate-agent
container. As a result, the agent's card will report its default localhost
URL, which is incorrect for a publicly deployed agent.
To fix this, you should append the AGENT_PUBLIC_URL
to the .env
file before restarting the container. This ensures the agent uses the correct public URL in its agent card.
# Force the real-estate-agent container to be recreated, using absolute paths. | |
/usr/local/bin/docker-compose --project-directory ${BASE_PATH} --env-file ${ENV_FILE} -f ${COMPOSE_FILE} up -d --force-recreate real-estate-agent | |
# Add the public URL to the .env file so the agent can use it. | |
echo "AGENT_PUBLIC_URL=${NGROK_URL}" >> "${ENV_FILE}" | |
# Force the real-estate-agent container to be recreated to pick up the new environment variable. | |
/usr/local/bin/docker-compose --project-directory ${BASE_PATH} --env-file ${ENV_FILE} -f ${COMPOSE_FILE} up -d --force-recreate real-estate-agent |
if not properties: | ||
await event_queue.enqueue_event(new_agent_text_message("No properties matched your criteria.")) | ||
return | ||
|
||
# Process and format the result before sending | ||
formatted_result = json.dumps(properties, indent=2) | ||
|
||
await event_queue.enqueue_event(new_agent_text_message(formatted_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of logical errors in this block that will prevent the agent from returning results correctly:
- Scope Issue: The
if not properties:
check is outside thetry
block where theproperties
variable is defined. If an exception occurs beforeproperties
is assigned (e.g., inself.mcp_client.connect()
), this will raise anUnboundLocalError
. - Unreachable Code: The code to format and send the results (lines 56-59) is unreachable because of the
return
statement on line 54.
All result handling logic should be moved inside the main try
block to ensure correct execution flow and variable scope.
if not properties: | |
await event_queue.enqueue_event(new_agent_text_message("No properties matched your criteria.")) | |
return | |
# Process and format the result before sending | |
formatted_result = json.dumps(properties, indent=2) | |
await event_queue.enqueue_event(new_agent_text_message(formatted_result)) | |
if not properties: | |
await event_queue.enqueue_event(new_agent_text_message("No properties matched your criteria.")) | |
return | |
# Process and format the result before sending | |
formatted_result = json.dumps(properties, indent=2) | |
await event_queue.enqueue_event(new_agent_text_message(formatted_result)) |
const detailedPropertiesPromises = propertiesOnPage.map(async (p) => { | ||
if (!p.url) return []; | ||
await delay(500); // Delay between detail page fetches | ||
const detailHtml = await fetchPageHTML(p.url, 0); // 0 indicates a detail page | ||
const enhancedProperty = parsePropertyDetails(detailHtml, p); | ||
if (enhancedProperty.units && enhancedProperty.units.length > 0) { | ||
return enhancedProperty.units as Property[]; | ||
} | ||
return [enhancedProperty as Property]; | ||
}); | ||
|
||
const detailedProperties = []; | ||
for (const promise of detailedPropertiesPromises) { | ||
detailedProperties.push(...(await promise)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation for fetching detailed property information may inadvertently send a burst of requests to the server, increasing the risk of being rate-limited or blocked.
The propertiesOnPage.map(async ...)
call initiates all asynchronous operations concurrently. The await delay(500)
inside the map callback only delays the start of each operation by 500ms from the time the loop starts, but it doesn't space out the requests relative to each other.
To ensure requests are properly throttled, I recommend refactoring this to a sequential loop that awaits a delay between each network request. This will make the scraping process more robust and respectful to the target server.
const detailedProperties: Property[] = [];
for (const p of propertiesOnPage) {
if (!p.url) {
continue;
}
await delay(500); // Delay *between* each detail page fetch
const detailHtml = await fetchPageHTML(p.url, 0); // 0 indicates a detail page
const enhancedProperty = parsePropertyDetails(detailHtml, p);
if (enhancedProperty.units && enhancedProperty.units.length > 0) {
detailedProperties.push(...(enhancedProperty.units as Property[]));
} else {
detailedProperties.push(enhancedProperty as Property);
}
}
async def search_rental_properties(self, args: dict) -> list[types.ContentBlock]: | ||
if not self.session: | ||
raise RuntimeError("Client not connected") | ||
|
||
result = await self.session.call_tool( | ||
"search_rental_properties", | ||
arguments=args, | ||
) | ||
|
||
if result.isError: | ||
error_message = "Tool call failed." | ||
if result.content and isinstance(result.content[0], types.TextContent): | ||
error_message += f" Reason: {result.content[0].text}" | ||
raise RuntimeError(error_message) | ||
|
||
return result.content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The search_rental_properties
method duplicates the implementation of the call_tool
method. To improve maintainability and reduce code duplication, this method can be simplified to directly call call_tool
with the specific tool name.
async def search_rental_properties(self, args: dict) -> list[types.ContentBlock]: | |
if not self.session: | |
raise RuntimeError("Client not connected") | |
result = await self.session.call_tool( | |
"search_rental_properties", | |
arguments=args, | |
) | |
if result.isError: | |
error_message = "Tool call failed." | |
if result.content and isinstance(result.content[0], types.TextContent): | |
error_message += f" Reason: {result.content[0].text}" | |
raise RuntimeError(error_message) | |
return result.content | |
async def search_rental_properties(self, args: dict) -> list[types.ContentBlock]: | |
"""A convenience method to call the 'search_rental_properties' tool.""" | |
return await self.call_tool("search_rental_properties", args) |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, self-contained real-estate agent sample. The architecture, involving a Python agent, a Node.js MCP server, and a local LLM, is well-structured and demonstrates a sophisticated use case. The code is generally high-quality.
My review has identified a few critical issues in the GCP deployment scripts that could prevent the sample from running correctly. I've also provided suggestions to improve the robustness of the web scraping logic. Addressing these points will make this an even more solid and reliable sample.
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | jq -r .tag_name) | ||
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose | ||
sudo chmod +x /usr/local/bin/docker-compose | ||
|
||
# Install ngrok and jq. | ||
curl -s https://ngrok-agent.s3.amazonaws.com/ngrok.asc | sudo tee /etc/apt/trusted.gpg.d/ngrok.asc >/dev/null | ||
echo "deb https://ngrok-agent.s3.amazonaws.com buster main" | sudo tee /etc/apt/sources.list.d/ngrok.list | ||
sudo apt-get update | ||
sudo apt-get install -y ngrok jq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script attempts to use jq
on line 17 to parse the latest Docker Compose version from the GitHub API, but jq
is not installed until line 25. This will cause the script to fail during execution.
To fix this, the jq
installation should be moved before it is first used.
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | jq -r .tag_name) | |
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose | |
sudo chmod +x /usr/local/bin/docker-compose | |
# Install ngrok and jq. | |
curl -s https://ngrok-agent.s3.amazonaws.com/ngrok.asc | sudo tee /etc/apt/trusted.gpg.d/ngrok.asc >/dev/null | |
echo "deb https://ngrok-agent.s3.amazonaws.com buster main" | sudo tee /etc/apt/sources.list.d/ngrok.list | |
sudo apt-get update | |
sudo apt-get install -y ngrok jq | |
# Install ngrok and jq. | |
sudo apt-get update | |
sudo apt-get install -y ngrok jq | |
# Install Docker Compose v2. | |
LATEST_COMPOSE_VERSION=$(curl -s https://api.github.com/repos/docker/compose/releases/latest | jq -r .tag_name) | |
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose | |
sudo chmod +x /usr/local/bin/docker-compose |
# Force the real-estate-agent container to be recreated, using absolute paths. | ||
/usr/local/bin/docker-compose --project-directory ${BASE_PATH} --env-file ${ENV_FILE} -f ${COMPOSE_FILE} up -d --force-recreate real-estate-agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script correctly retrieves the public URL from ngrok but doesn't pass this URL to the real-estate-agent
container. The agent's public URL is configured via the AGENT_PUBLIC_URL
environment variable, which is loaded from the .env
file. The script should update the .env
file with the retrieved ngrok URL before restarting the agent container.
# Force the real-estate-agent container to be recreated, using absolute paths. | |
/usr/local/bin/docker-compose --project-directory ${BASE_PATH} --env-file ${ENV_FILE} -f ${COMPOSE_FILE} up -d --force-recreate real-estate-agent | |
# Update the .env file with the public URL. | |
echo "AGENT_PUBLIC_URL=${NGROK_URL}" >> "${ENV_FILE}" | |
# Force the real-estate-agent container to be recreated, using absolute paths. | |
/usr/local/bin/docker-compose --project-directory ${BASE_PATH} --env-file ${ENV_FILE} -f ${COMPOSE_FILE} up -d --force-recreate real-estate-agent |
const detailedPropertiesPromises = propertiesOnPage.map(async (p) => { | ||
if (!p.url) return []; | ||
await delay(500); // Delay between detail page fetches | ||
const detailHtml = await fetchPageHTML(p.url, 0); // 0 indicates a detail page | ||
const enhancedProperty = parsePropertyDetails(detailHtml, p); | ||
if (enhancedProperty.units && enhancedProperty.units.length > 0) { | ||
return enhancedProperty.units as Property[]; | ||
} | ||
return [enhancedProperty as Property]; | ||
}); | ||
|
||
const detailedProperties = []; | ||
for (const promise of detailedPropertiesPromises) { | ||
detailedProperties.push(...(await promise)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scrapeAllPages
function intends to fetch property detail pages with a delay to avoid being rate-limited. However, the current implementation using propertiesOnPage.map(async ...)
initiates all network requests for detail pages in parallel. The for...of
loop that follows only awaits the already-in-flight promises one by one. This can lead to a burst of requests to the server, increasing the risk of being blocked.
To ensure requests are sent sequentially with a delay between them, you should use a for...of
loop to iterate and await
each fetch.
const detailedPropertiesPromises = propertiesOnPage.map(async (p) => { | |
if (!p.url) return []; | |
await delay(500); // Delay between detail page fetches | |
const detailHtml = await fetchPageHTML(p.url, 0); // 0 indicates a detail page | |
const enhancedProperty = parsePropertyDetails(detailHtml, p); | |
if (enhancedProperty.units && enhancedProperty.units.length > 0) { | |
return enhancedProperty.units as Property[]; | |
} | |
return [enhancedProperty as Property]; | |
}); | |
const detailedProperties = []; | |
for (const promise of detailedPropertiesPromises) { | |
detailedProperties.push(...(await promise)); | |
} | |
const detailedProperties: Property[] = []; | |
for (const p of propertiesOnPage) { | |
if (!p.url) continue; | |
await delay(500); // Delay between detail page fetches | |
const detailHtml = await fetchPageHTML(p.url, 0); | |
const enhancedProperty = parsePropertyDetails(detailHtml, p); | |
if (enhancedProperty.units && enhancedProperty.units.length > 0) { | |
detailedProperties.push(...(enhancedProperty.units as Property[])); | |
} else { | |
detailedProperties.push(enhancedProperty as Property); | |
} | |
} |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a well-structured and comprehensive real-estate agent sample. My feedback focuses on improving robustness, efficiency, and maintainability. I've suggested adding more robust error handling in the Python agent, optimizing the asynchronous scraping logic in the Node.js service for better performance, and making the deployment scripts idempotent. These changes will help make the sample even more reliable and easier to maintain.
try: | ||
await self.mcp_client.connect() | ||
if not request.message.parts or not isinstance(request.message.parts[0].root, TextContent): | ||
await event_queue.enqueue_event(new_agent_text_message("Invalid request: message is empty or not text.")) | ||
return | ||
query = request.message.parts[0].root.text | ||
|
||
# Call the new parse_query tool | ||
parsed_params_parts = await self.mcp_client.call_tool("parse_query", {"query": query}) | ||
if not parsed_params_parts or not isinstance(parsed_params_parts[0], TextContent): | ||
await event_queue.enqueue_event(new_agent_text_message("Could not understand your request.")) | ||
return | ||
|
||
try: | ||
filters = json.loads(parsed_params_parts[0].text) | ||
except json.JSONDecodeError: | ||
await event_queue.enqueue_event(new_agent_text_message("Error: Could not parse the query parameters.")) | ||
return | ||
|
||
result_parts = await self.mcp_client.search_rental_properties(filters) | ||
|
||
if not result_parts or not isinstance(result_parts[0], TextContent): | ||
await event_queue.enqueue_event(new_agent_text_message("No results found.")) | ||
return | ||
|
||
# The actual data is a JSON string inside the first text part | ||
try: | ||
properties = json.loads(result_parts[0].text) | ||
except json.JSONDecodeError: | ||
await event_queue.enqueue_event(new_agent_text_message("Error: Could not parse the property results.")) | ||
return | ||
|
||
if not properties: | ||
await event_queue.enqueue_event(new_agent_text_message("No properties matched your criteria.")) | ||
return | ||
|
||
# Process and format the result before sending | ||
formatted_result = json.dumps(properties, indent=2) | ||
|
||
await event_queue.enqueue_event(new_agent_text_message(formatted_result)) | ||
finally: | ||
await self.mcp_client.disconnect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation lacks a try...except
block to handle potential RuntimeError
exceptions raised by the mcp_client
when a tool call fails. If an exception occurs (e.g., the MCP server is down or returns an error), it will be unhandled.
Wrapping the core logic in a try...except
block will allow you to catch these errors and provide a more graceful and informative response to the user.
try:
await self.mcp_client.connect()
if not request.message.parts or not isinstance(request.message.parts[0].root, TextContent):
await event_queue.enqueue_event(new_agent_text_message("Invalid request: message is empty or not text."))
return
query = request.message.parts[0].root.text
# Call the new parse_query tool
parsed_params_parts = await self.mcp_client.call_tool("parse_query", {"query": query})
if not parsed_params_parts or not isinstance(parsed_params_parts[0], TextContent):
await event_queue.enqueue_event(new_agent_text_message("Could not understand your request."))
return
try:
filters = json.loads(parsed_params_parts[0].text)
except json.JSONDecodeError:
await event_queue.enqueue_event(new_agent_text_message("Error: Could not parse the query parameters."))
return
result_parts = await self.mcp_client.search_rental_properties(filters)
if not result_parts or not isinstance(result_parts[0], TextContent):
await event_queue.enqueue_event(new_agent_text_message("No results found."))
return
# The actual data is a JSON string inside the first text part
try:
properties = json.loads(result_parts[0].text)
except json.JSONDecodeError:
await event_queue.enqueue_event(new_agent_text_message("Error: Could not parse the property results."))
return
if not properties:
await event_queue.enqueue_event(new_agent_text_message("No properties matched your criteria."))
return
# Process and format the result before sending
formatted_result = json.dumps(properties, indent=2)
await event_queue.enqueue_event(new_agent_text_message(formatted_result))
except Exception as e:
# In a production system, you would log the full error `e`
await event_queue.enqueue_event(new_agent_text_message(f"An error occurred while processing your request: {e}"))
finally:
await self.mcp_client.disconnect()
const detailedProperties = []; | ||
for (const promise of detailedPropertiesPromises) { | ||
detailedProperties.push(...(await promise)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation processes the detailedPropertiesPromises
sequentially inside a for...of
loop. This negates the benefit of creating the promises in parallel, as each iteration waits for the previous promise to resolve. Use Promise.all()
to resolve all detail page fetch promises concurrently.
const detailedPropertiesArrays = await Promise.all(detailedPropertiesPromises);
const detailedProperties = detailedPropertiesArrays.flat();
# Update the .env file with the public URL. | ||
echo "AGENT_PUBLIC_URL=${NGROK_URL}" >> "${ENV_FILE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using echo ... >> .env
to set the AGENT_PUBLIC_URL
is not idempotent. If this script runs more than once, it will append multiple AGENT_PUBLIC_URL
lines to the .env
file, which can lead to incorrect behavior. Make the script more robust by checking if the variable already exists and update it, or add it if it doesn't.
# Update the .env file with the public URL. | |
echo "AGENT_PUBLIC_URL=${NGROK_URL}" >> "${ENV_FILE}" | |
# Update the .env file with the public URL idempotently. | |
if grep -q "^AGENT_PUBLIC_URL=" "${ENV_FILE}"; then | |
sed -i "s|^AGENT_PUBLIC_URL=.*|AGENT_PUBLIC_URL=${NGROK_URL}|" "${ENV_FILE}" | |
else | |
echo "AGENT_PUBLIC_URL=${NGROK_URL}" >> "${ENV_FILE}" | |
fi |
async def search_rental_properties(self, args: dict) -> list[types.ContentBlock]: | ||
if not self.session: | ||
raise RuntimeError("Client not connected") | ||
|
||
result = await self.session.call_tool( | ||
"search_rental_properties", | ||
arguments=args, | ||
) | ||
|
||
if result.isError: | ||
error_message = "Tool call failed." | ||
if result.content and isinstance(result.content[0], types.TextContent): | ||
error_message += f" Reason: {result.content[0].text}" | ||
raise RuntimeError(error_message) | ||
|
||
return result.content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The search_rental_properties
method duplicates the logic for calling a tool and handling errors that is already present in the generic call_tool
method. Simplify this method to be a direct wrapper around call_tool
.
async def search_rental_properties(self, args: dict) -> list[types.ContentBlock]: | |
if not self.session: | |
raise RuntimeError("Client not connected") | |
result = await self.session.call_tool( | |
"search_rental_properties", | |
arguments=args, | |
) | |
if result.isError: | |
error_message = "Tool call failed." | |
if result.content and isinstance(result.content[0], types.TextContent): | |
error_message += f" Reason: {result.content[0].text}" | |
raise RuntimeError(error_message) | |
return result.content | |
async def search_rental_properties(self, args: dict) -> list[types.ContentBlock]: | |
"""A specific method for calling the 'search_rental_properties' tool.""" | |
return await self.call_tool("search_rental_properties", args) |
const ollama = new Ollama({ | ||
model: 'tinydolphin', | ||
temperature: 0.1, | ||
baseURL: 'http://ollama:11434', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Ollama baseURL
is currently hardcoded. For better maintainability and flexibility, it's best to manage configurable values like this in a central location. Moving this URL to src/config.ts
will make it easier to modify for different environments.
baseURL: 'http://ollama:11434', | |
baseURL: 'http://ollama:11434', // TODO: Move to config.ts |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, self-contained sample for a real-estate agent. My review focuses on improving robustness, security, and configuration clarity. I suggest preventing the leakage of internal error details in the Python agent, improving resource management in the Python MCP client, and tidying up the TypeScript configuration to remove redundancy.
await event_queue.enqueue_event(new_agent_text_message(formatted_result)) | ||
except Exception as e: | ||
# In a production system, you would log the full error `e` | ||
await event_queue.enqueue_event(new_agent_text_message(f"An error occurred while processing your request: {e}")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current exception handling leaks internal error details to the client by including the raw exception e
in the response message. This is a security risk (information disclosure) as it can reveal implementation details of your agent. It's best practice to log the detailed error for debugging and return a generic, user-friendly error message.
await event_queue.enqueue_event(new_agent_text_message(f"An error occurred while processing your request: {e}")) | |
await event_queue.enqueue_event(new_agent_text_message("An unexpected error occurred while processing your request.")) |
self._session_cm = ClientSession(read_stream, write_stream) | ||
self.session = await self._session_cm.__aenter__() | ||
await self.session.initialize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually calling __aenter__
on async context managers can be risky. If an exception occurs while setting up the ClientSession
(lines 28-30), the __aexit__
method for the sse_client
transport will not be called, potentially leading to a resource leak.
Using a try...except
block here will make the connection logic more robust by ensuring resources are cleaned up on failure. For an even more idiomatic solution in the future, you could consider using contextlib.AsyncExitStack
to manage the lifecycle of nested async contexts.
self._session_cm = ClientSession(read_stream, write_stream) | |
self.session = await self._session_cm.__aenter__() | |
await self.session.initialize() | |
try: | |
self._session_cm = ClientSession(read_stream, write_stream) | |
self.session = await self._session_cm.__aenter__() | |
await self.session.initialize() | |
except Exception: | |
# If session setup fails, ensure transport is closed. | |
await self._transport_cm.__aexit__(None, None, None) | |
self._transport_cm = None | |
raise |
// "erasableSyntaxOnly": true, /* Do not allow runtime constructs that are not part of ECMAScript. */ | ||
// "allowSyntheticDefaultImports": true, /* Allow 'import x from y' when a module doesn't have a default export. */ | ||
"esModuleInterop": true, /* Emit additional JavaScript to ease support for importing CommonJS modules. This enables 'allowSyntheticDefaultImports' for type compatibility. */ | ||
"allowSyntheticDefaultImports": true, /* Allow 'import x from y' when a module doesn't have a default export. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a well-structured and sophisticated real-estate agent sample. My review focuses on a few critical and medium-severity issues related to dependency management that will prevent the sample from building correctly and could cause maintenance challenges. Once these are addressed, this will be an excellent addition to the samples repository.
"dependencies": { | ||
"@modelcontextprotocol/sdk": "1.15.0", | ||
"axios": "1.7.2", | ||
"cheerio": "1.0.0-rc.12", | ||
"dotenv": "16.5.0", | ||
"llamaindex": "0.1.15", | ||
"winston": "3.17.0", | ||
"zod": "3.23.8" | ||
}, | ||
"devDependencies": { | ||
"@types/cheerio": "0.22.35", | ||
"@types/express": "4.17.13", | ||
"@types/node": "22.5.2", | ||
"@typescript-eslint/eslint-plugin": "6.0.0", | ||
"@typescript-eslint/parser": "6.0.0", | ||
"@vitest/coverage-v8": "1.6.0", | ||
"eslint": "8.56.0", | ||
"eslint-config-prettier": "10.1.5", | ||
"eslint-plugin-prettier": "5.5.0", | ||
"prettier": "3.5.3", | ||
"typescript": "5.8.3", | ||
"typescript-eslint": "7.0.0", | ||
"vitest": "1.6.0" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several dependencies in this package.json
file have versions that likely do not exist on the npm registry. This will cause the npm install
command to fail, making the dafty-mcp
service unbuildable.
Please review all dependencies and update them to valid, existing versions. I recommend using the latest stable versions or running npm install <package>@latest
to find the correct versions.
@@ -0,0 +1 @@ | |||
declare module 'llamaindex'; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This declaration file declare module 'llamaindex';
effectively types the llamaindex
module as any
, which suppresses TypeScript's type checking for it.
The llamaindex
package (version 0.1.15
as specified in package.json
) already includes its own TypeScript definitions. This file is therefore unnecessary and can hide potential type-related bugs.
Please remove this file to allow TypeScript to use the built-in types from the llamaindex
package.
[tool.poetry.dependencies] | ||
python = "3.13" | ||
a2a-sdk = "0.2.0" | ||
python-dotenv = "1.0.0" | ||
uvicorn = "0.22.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dependencies defined in pyproject.toml
are inconsistent with those in requirements.txt
. This can lead to different behavior between local development (using Poetry) and the Docker container (using requirements.txt
), which is a significant maintainability issue.
The best practice is to maintain direct dependencies in pyproject.toml
and generate requirements.txt
from it. This ensures consistency across all environments. You can generate the file using Poetry:
poetry export -f requirements.txt --output requirements.txt --without-hashes
I recommend updating the versions in pyproject.toml
to the desired ones and then regenerating requirements.txt
.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, self-contained sample for a real-estate agent. My review identified one high-severity issue in the dafty-mcp
service. The query parsing logic correctly prompts the LLM to extract the propertyType
from a user's query, but this information is subsequently ignored. This prevents the agent from filtering search results by property type. Apart from this, the implementation is excellent, demonstrating robust error handling, good security practices, and a clean architecture.
function sanitizeLLMOutput(data: RawLLMOutput): Partial<SearchRentalPropertiesParams> { | ||
const sanitized: Partial<SearchRentalPropertiesParams> = {}; | ||
|
||
if (data.location && typeof data.location === 'string') { | ||
sanitized.location = data.location; | ||
} | ||
|
||
if (data.maxPrice) { | ||
const priceString = String(data.maxPrice).replace(/[€,]/g, ''); | ||
const price = parseInt(priceString, 10); | ||
if (!isNaN(price)) { | ||
sanitized.maxPrice = price; | ||
} | ||
} | ||
|
||
if (data.numBeds) { | ||
const beds = parseInt(String(data.numBeds), 10); | ||
if (!isNaN(beds)) { | ||
sanitized.numBeds = beds; | ||
} | ||
} | ||
|
||
return sanitized; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RawLLMOutput
interface and sanitizeLLMOutput
function do not handle the propertyType
field. The LLM prompt (lines 17-45) instructs the model to extract this information, and the examples show it being extracted. However, the current implementation ignores this field, meaning that if a user specifies a property type in their query (e.g., "Find an apartment..."), it will not be used in the subsequent property search.
To fix this, you should update the interface and the sanitization function to handle propertyType
.
interface RawLLMOutput {
location?: string;
maxPrice?: string | number;
numBeds?: string | number;
propertyType?: string;
}
function sanitizeLLMOutput(data: RawLLMOutput): Partial<SearchRentalPropertiesParams> {
const sanitized: Partial<SearchRentalPropertiesParams> = {};
if (data.location && typeof data.location === 'string') {
sanitized.location = data.location;
}
if (data.maxPrice) {
const priceString = String(data.maxPrice).replace(/[€,]/g, '');
const price = parseInt(priceString, 10);
if (!isNaN(price)) {
sanitized.maxPrice = price;
}
}
if (data.numBeds) {
const beds = parseInt(String(data.numBeds), 10);
if (!isNaN(beds)) {
sanitized.numBeds = beds;
}
}
if (data.propertyType && typeof data.propertyType === 'string') {
sanitized.propertyType = data.propertyType;
}
return sanitized;
}
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, self-contained sample for a real-estate agent. The code quality is high, and the sample demonstrates a sophisticated use of multiple services. The review focuses on suggestions to enhance security and error handling.
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose | ||
sudo chmod +x /usr/local/bin/docker-compose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a checksum validation step to ensure the integrity of the downloaded docker-compose
binary. This mitigates the risk of using a compromised binary if the download source is tampered with.
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose | |
sudo chmod +x /usr/local/bin/docker-compose | |
LATEST_COMPOSE_VERSION="v2.27.0" # Or another specific, tested version | |
COMPOSE_PLATFORM="$(uname -s | tr '[:upper:]' '[:lower:]')-$(uname -m)" | |
sudo curl -L "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-${COMPOSE_PLATFORM}" -o /usr/local/bin/docker-compose | |
# Verify the binary's checksum for security | |
sudo curl -sL "https://github.com/docker/compose/releases/download/${LATEST_COMPOSE_VERSION}/docker-compose-${COMPOSE_PLATFORM}.sha256" -o /tmp/docker-compose.sha256 | |
(cd /tmp && echo "$(cat docker-compose.sha256) /usr/local/bin/docker-compose" | sha256sum --check --strict) | |
if [ $? -ne 0 ]; then | |
echo "Docker Compose checksum validation failed. Aborting." >&2 | |
exit 1 | |
fi | |
rm /tmp/docker-compose.sha256 | |
sudo chmod +x /usr/local/bin/docker-compose |
error_message = f"Tool call for '{tool_name}' failed." | ||
if result.content and isinstance(result.content[0], types.TextContent): | ||
error_message += f" Reason: {result.content[0].text}" | ||
raise RuntimeError(error_message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a tool call fails, consider parsing the JSON error payload and extracting the more user-friendly message
field, raising an error with that instead. This provides better encapsulation and a cleaner user experience.
error_message = f"Tool call for '{tool_name}' failed." | |
if result.content and isinstance(result.content[0], types.TextContent): | |
error_message += f" Reason: {result.content[0].text}" | |
raise RuntimeError(error_message) | |
if result.isError: | |
error_message = f"Tool call for '{tool_name}' failed." | |
if result.content and isinstance(result.content[0], types.TextContent): | |
try: | |
payload = json.loads(result.content[0].text) | |
# The payload's message is intended to be more user-friendly | |
error_message = payload.get("message", error_message) | |
except json.JSONDecodeError: | |
# Fallback if the content is not JSON, append the raw text | |
error_message += f" Reason: {result.content[0].text}" | |
raise RuntimeError(error_message) |
@gemini-code-assist I've pushed numerous commits addressing the bot's feedback. Could a human reviewer please take a look? I want to ensure there aren't any underlying issues that I'm missing before this PR gets too noisy. |
Hi @amineremache, Thank you for the update and for diligently addressing the feedback! It's great to see the progress. As an automated code review assistant, I'm designed to help identify potential issues and suggest improvements based on the code changes. While I can continue to provide detailed feedback on your commits, I'm unable to directly 'summon' a human reviewer or escalate the pull request for a manual review. If you'd like a human reviewer to take a look, please use your team's standard process for requesting a manual review (e.g., by tagging a specific team member or using the platform's review request features). I'm here to assist further with automated reviews if you continue to push changes and would like my input. Just let me know! |
@gemini-code-assist, who would be the most appropriate contributor to tag based on this repo's commit history? |
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This PR introduces a new, comprehensive real-estate agent sample. The architecture is well-designed, using Docker Compose to orchestrate a Python agent, a Node.js tool server, and an LLM service. The code quality is high, with excellent use of modern features, structured logging, custom error handling, and thorough testing. My review focuses on a few configuration issues in package.json
that would prevent the Node.js service from building, and a medium-severity issue in the Dockerfile to ensure correct logging permissions. Once these are addressed, the sample should be in great shape.
"dependencies": { | ||
"@modelcontextprotocol/sdk": "1.15.0", | ||
"axios": "^1.7.2", | ||
"cheerio": "1.0.0-rc.12", | ||
"dotenv": "^16.4.5", | ||
"llamaindex": "0.11.13", | ||
"winston": "3.17.0", | ||
"zod": "^3.23.8" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several dependencies have versions that do not exist on the npm registry, which will cause npm ci
to fail. For example, llamaindex
is at version 0.5.x
, not 0.11.x
, and winston
is at 3.13.x
, not 3.17.x
. Please correct these versions to use existing, stable releases.
"dependencies": {
"@modelcontextprotocol/sdk": "1.15.0",
"axios": "^1.7.2",
"cheerio": "1.0.0-rc.12",
"dotenv": "^16.4.5",
"llamaindex": "^0.5.11",
"winston": "^3.13.1",
"zod": "^3.23.8"
}
"devDependencies": { | ||
"@types/cheerio": "1.0.0", | ||
"@types/express": "5.0.3", | ||
"@types/node": "^18.19.0", | ||
"@typescript-eslint/eslint-plugin": "8.36.0", | ||
"@typescript-eslint/parser": "8.36.0", | ||
"@vitest/coverage-v8": "3.2.4", | ||
"eslint": "9.30.1", | ||
"eslint-config-prettier": "10.1.5", | ||
"eslint-plugin-prettier": "5.5.1", | ||
"prettier": "3.6.2", | ||
"typescript": "5.8.3", | ||
"typescript-eslint": "8.36.0", | ||
"vitest": "3.2.4" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of the devDependencies
have versions that do not exist on the npm registry. This will cause the build to fail. For example, @types/express
does not have a major version 5, and many of the eslint/prettier packages have inflated version numbers. Please update these to valid, recent versions from npm.
"devDependencies": {
"@types/cheerio": "1.0.0",
"@types/express": "^4.17.21",
"@types/node": "^18.19.0",
"@typescript-eslint/eslint-plugin": "^6.0.0",
"@typescript-eslint/parser": "^6.0.0",
"@vitest/coverage-v8": "^0.34.0",
"eslint": "^8.0.0",
"eslint-config-prettier": "^8.0.0",
"eslint-plugin-prettier": "^5.0.0",
"prettier": "^3.0.0",
"typescript": "^5.0.0",
"typescript-eslint": "^6.0.0",
"vitest": "^0.34.0"
}
# Switch to the non-root user | ||
USER node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The application logger is configured to write to a logs
directory. However, the non-root node
user may not have permission to create this directory inside /usr/src/app
at runtime, which could cause the application to fail on startup. It's a best practice to create this directory and set its ownership in the Dockerfile before switching to the non-root user.
RUN mkdir -p logs && chown node:node logs
USER node
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new, self-contained sample for a real-estate agent. The sample is well-structured, using Docker Compose to orchestrate a Python agent, a Node.js tool server, and an Ollama LLM service. The code quality is high, with good practices like multi-stage Docker builds, non-root users, healthchecks, structured logging, and custom error handling. I've provided a few suggestions to improve robustness and configurability, particularly around the deployment scripts and the MCP tool server.
samples/python/agents/real_estate_agent/deployment/gcp/gcp_startup_service.sh
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,10 @@ | |||
node_modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
samples/python/agents/real_estate_agent/dafty-mcp/src/services/daftScraper.service.ts
Outdated
Show resolved
Hide resolved
samples/python/agents/real_estate_agent/dafty-mcp/src/services/daftScraper.service.ts
Outdated
Show resolved
Hide resolved
samples/python/agents/real_estate_agent/dafty-mcp/src/services/queryParser.service.ts
Outdated
Show resolved
Hide resolved
01840e2
to
25e7e48
Compare
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new real-estate-agent sample, demonstrating an agent capable of understanding natural language queries to search for rental properties. The sample is well-structured and uses Docker for orchestration. The review identified a missing dependency (express
) in dafty-mcp/package.json
, a logging issue in docker-compose.yml
for the ollama
service, and a typing issue in dafty-mcp/src/types.ts
.
"dotenv": "17.1.0", | ||
"llamaindex": "0.1.21", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
ollama: | ||
image: ollama/ollama | ||
entrypoint: ["/bin/sh", "-c", "ollama serve > /dev/null 2>&1"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entrypoint for the ollama
service redirects all standard output and standard error to /dev/null
. This will suppress all logs from the Ollama server, including critical startup errors, making it very difficult to debug if the service fails to start or misbehaves. It's better to let it log to the console so the logs can be inspected with docker-compose logs
.
entrypoint: ["/bin/sh", "-c", "ollama serve"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logs are noisy and not needed.
ber?: string; | ||
latitude?: number; | ||
longitude?: number; | ||
units?: Unit[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The units
property on the Property
interface is typed as Unit[]
, but the logic in parser.service.ts
populates it with objects that are effectively Property
instances (including inherited location data). This leads to incorrect type casting back and forth between Property[]
and Unit[]
in the services.
units?: Unit[]; | |
units?: Property[]; |
98f7199
to
7746558
Compare
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a comprehensive, multi-service sample for a real-estate agent. The overall architecture is well-designed, leveraging Docker Compose for orchestration, a Python agent for the core logic, and a Node.js server for tool implementation. The code quality is high, with good use of modern language features, clear documentation in READMEs, and the inclusion of tests.
My review focuses on improving dependency management for reproducibility, enhancing maintainability by removing code duplication, and correcting a type-safety issue. These changes will help ensure the sample is robust and easy for others to run and maintain.
samples/python/agents/real_estate_agent/dafty-mcp/src/services/parser.service.ts
Outdated
Show resolved
Hide resolved
samples/python/agents/real_estate_agent/dafty-mcp/src/services/fetch.service.ts
Outdated
Show resolved
Hide resolved
92ea5b8
to
e773af1
Compare
This commit introduces a new, self-contained sample for a real-estate agent. The sample demonstrates an agent capable of understanding natural language queries to search for rental properties. It is composed of three services orchestrated with Docker Compose: - A core Python A2A agent (`real-estate-agent`). - A Node.js MCP tool server (`dafty-mcp`) for interacting with a real-estate service. - A local LLM service (`ollama`) for natural language understanding. The sample is fully self-contained, with all necessary code and configuration included. The README provides detailed instructions for setup, usage, and testing, and directs contributions back to the original source repositories.
e773af1
to
b16c564
Compare
@gemini-code-assist, how can I get a human to get merge approval? @holtskinner, apologies if you're not the one, but you are one of the top contributors in the repo. If it's not you, please feel free to reassign the CR. |
Hi @amineremache, I understand you're looking to get human merge approval for this pull request. As an automated code review assistant, my capabilities are limited to providing feedback on the code itself. I don't have access to your team's internal processes for merge approval, nor can I identify specific human reviewers or reassign pull requests. To get human merge approval, please continue to follow your team's established procedures for requesting manual reviews and approvals. This typically involves tagging specific team members or using your platform's built-in review request features, as you've done by tagging @holtskinner. I'm here to continue assisting with automated code reviews if you have further code changes you'd like me to examine. |
@mikeas1 maybe? |
One of our goals with our samples repository is to help A2A users understand how various frameworks and styles of agents can be built with A2A. Can you help me understand what distinguishes this agent from others that already exist in our samples? Or, stated another way, what are you looking to show with this sample? What are the unique characteristics of this sample agent that you believe will help users better understand how to use A2A? |
This commit introduces a new, self-contained sample for a real-estate agent.
The sample demonstrates an agent capable of understanding natural language queries to search for rental properties. It is composed of three services orchestrated with Docker Compose:
real-estate-agent
).dafty-mcp
) for interacting with a real-estate service.ollama
) for natural language understanding.The sample is fully self-contained, with all necessary code and configuration included. The README provides detailed instructions for setup, usage, and testing, and directs contributions back to the original source repositories.