Skip to content

Conversation

amineremache
Copy link

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.

@amineremache amineremache requested a review from a team as a code owner July 6, 2025 10:14
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 local ollama instance running the tinydolphin 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

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 3 to 15
real-estate-agent:
build: .
ports:
- "3001:3001"
environment:
- MCP_SERVER_URL=http://dafty-mcp:4000/sse
depends_on:
- dafty-mcp
networks:
- app_net
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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

Comment on lines 9 to 22
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The current API key authentication has two critical security vulnerabilities:

  1. Timing Attacks: The direct string comparison is vulnerable to timing attacks.
  2. Server Misconfiguration: If the API_KEY environment variable is not set, os.environ.get('API_KEY') returns None, and the check becomes api_key != "Bearer None". An attacker could potentially bypass authentication by sending Authorization: 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.

Suggested change
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")

Comment on lines 107 to 118
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];
})
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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

Comment on lines 30 to 39
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The script hardcodes the Docker Compose version v2.24.5. It's better to dynamically fetch the latest version number from the GitHub API.

Suggested change
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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This line attempts to update a DAFTY_MCP_PATH variable in the .env file. However, this variable is not present in the .env.example file and is not used by the provided docker-compose.yml. If it's a leftover from a previous implementation, it should be removed.


ollama:
image: ollama/ollama
entrypoint: ["/bin/sh", "-c", "ollama serve > /dev/null 2>&1"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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"]

Comment on lines 58 to 62
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Raising a generic Exception is not ideal for error handling. It's better to define and use a custom exception class (e.g., McpToolError) for tool-related failures. This applies to both search_rental_properties and call_tool methods.

            raise RuntimeError(error_message)

Comment on lines 1 to 4
a2a-sdk
python-dotenv
uvicorn
mcp No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The requirements.txt file lists dependencies without pinning their versions. This can lead to non-reproducible builds. Pin dependency versions to ensure that the application is built and run with the exact same set of packages every time.

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 53 to 54
await event_queue.enqueue_event(new_agent_text_message("No properties matched your criteria."))
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

Suggested change
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

Comment on lines 21 to 41
"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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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"

Comment on lines 14 to 20
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Comment on lines 26 to 41
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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" ]

Comment on lines 32 to 42
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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

Comment on lines 32 to 47
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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":

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 5 to 6
const timestampStr = new Date().toISOString().replace(/:/g, '-').replace(/\..+/, '');
const logFileName = `${timestampStr}_daft_mcp.log`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 64 to 109
app.get('/sse', (req: Request, res: Response) => {
transport = new SSEServerTransport('/message', res);
server.connect(transport);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Comment on lines 44 to 66
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,
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The compiler option allowSyntheticDefaultImports is duplicated. You can safely remove this line as esModuleInterop: true (on line 84) already implies it.

Comment on lines 17 to 18
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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

Comment on lines 6 to 75
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 82 to 102
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');
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

Comment on lines 15 to 33
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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:

  1. Wait for Docker.
  2. Wait for ngrok and get the public URL.
  3. Update the .env file with the URL.
  4. Run docker-compose up --build -d once for all services.
    This will improve startup time and reduce unnecessary container restarts.

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 16 to 20
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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:

Suggested change
# 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

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider using a Docker named volume instead of a bind mount for the .session directory. Bind mounts can lead to permission issues and pollute the source tree with runtime artifacts. Using a named volume ensures better isolation and portability.

      - dafty-session:/usr/src/app/.session

Comment on lines 1 to 23
[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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure dependency consistency between local development and the Docker container, generate requirements.txt from pyproject.toml before building the Docker image. This establishes a single source of truth for dependencies, enhancing dependency management robustness and predictability.

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 37 to 38
const message = error instanceof Error ? error.message : 'An unknown error occurred during scraping.';
throw new ScraperError(message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
const message = error instanceof Error ? error.message : 'An unknown error occurred during scraping.';
throw new ScraperError(message);
throw new ScraperError(`Scraping failed: ${message}`);

Comment on lines 85 to 86
const message = error instanceof Error ? error.message : 'An unknown error occurred during query parsing.';
throw new ScraperError(message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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}`);

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 23 to 25
# Create a non-root user and switch to it
RUN useradd --create-home appuser
USER appuser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The appuser user and group are created twice in this Dockerfile (lines 17-18 and 23-25). The second RUN useradd command will fail because the user already exists, which will cause the Docker build to fail. This duplicate block should be removed.

Comment on lines 30 to 31
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

Suggested change
# 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

Comment on lines 52 to 59
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

There are a couple of logical errors in this block that will prevent the agent from returning results correctly:

  1. Scope Issue: The if not properties: check is outside the try block where the properties variable is defined. If an exception occurs before properties is assigned (e.g., in self.mcp_client.connect()), this will raise an UnboundLocalError.
  2. 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.

Suggested change
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))

Comment on lines 107 to 118
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));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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);
      }
    }

Comment on lines 32 to 47
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 17 to 20
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

Suggested change
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

Comment on lines 30 to 31
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.

Suggested change
# 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

Comment on lines 107 to 118
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));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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);
}
}

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 20 to 61
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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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();

Comment on lines 30 to 31
# Update the .env file with the public URL.
echo "AGENT_PUBLIC_URL=${NGROK_URL}" >> "${ENV_FILE}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
# 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

Comment on lines 32 to 47
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
baseURL: 'http://ollama:11434',
baseURL: 'http://ollama:11434', // TODO: Move to config.ts

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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}"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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."))

Comment on lines 28 to 30
self._session_cm = ClientSession(read_stream, write_stream)
self.session = await self._session_cm.__aenter__()
await self.session.initialize()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The allowSyntheticDefaultImports option is redundant here. The esModuleInterop: true option on line 84 already implies allowSyntheticDefaultImports: true. You can safely remove this line to keep the configuration cleaner and avoid potential confusion.

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 19 to 42
"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"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Comment on lines 7 to 11
[tool.poetry.dependencies]
python = "3.13"
a2a-sdk = "0.2.0"
python-dotenv = "1.0.0"
uvicorn = "0.22.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 73 to 86
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;
}

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 24 to 36
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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

Comment on lines 54 to 64
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)

@amineremache
Copy link
Author

@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.

Copy link
Contributor

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!

@amineremache
Copy link
Author

@gemini-code-assist, who would be the most appropriate contributor to tag based on this repo's commit history?

Copy link
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 19 to 27
"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"
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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"
  }

Comment on lines 28 to 42
"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"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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"
  }

Comment on lines +28 to +35
# Switch to the non-root user
USER node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

@@ -0,0 +1,10 @@
node_modules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider adding .session to this file to prevent it from being copied into the Docker image during the build process. This directory is created at runtime, so there is no need to include it in the image.

@amineremache amineremache force-pushed the feat-real-estate-agent branch 2 times, most recently from 01840e2 to 25e7e48 Compare July 8, 2025 23:24
@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines +23 to +24
"dotenv": "17.1.0",
"llamaindex": "0.1.21",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The express package is used in src/server.ts to create the web server, but it's missing from the dependencies list. This will cause the application to crash at runtime with a "Cannot find module 'express'" error.

    "dotenv": "17.1.0",
    "express": "4.19.2",
    "llamaindex": "0.1.21",


ollama:
image: ollama/ollama
entrypoint: ["/bin/sh", "-c", "ollama serve > /dev/null 2>&1"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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"]

Copy link
Author

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[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
units?: Unit[];
units?: Property[];

@amineremache amineremache force-pushed the feat-real-estate-agent branch 2 times, most recently from 98f7199 to 7746558 Compare July 8, 2025 23:36
@amineremache
Copy link
Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

@amineremache amineremache force-pushed the feat-real-estate-agent branch 2 times, most recently from 92ea5b8 to e773af1 Compare July 8, 2025 23:58
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.
@amineremache amineremache force-pushed the feat-real-estate-agent branch from e773af1 to b16c564 Compare July 9, 2025 00:03
@amineremache
Copy link
Author

@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.

Copy link
Contributor

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.

@amineremache
Copy link
Author

@mikeas1 maybe?

@mikeas1
Copy link
Contributor

mikeas1 commented Jul 16, 2025

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants