feat(narratives): add agent sidecar (Gemini MCP proxy) - #426
feat(narratives): add agent sidecar (Gemini MCP proxy)#426ddebasmita-lab wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a sidecar agent for a multi-container Cloud Run service, including a Dockerfile, a build script, dependency definitions, and documentation of patches applied to the upstream agent code. Feedback on these changes suggests running the Docker container as a non-root user to improve security and prevent runtime permission errors, correcting a comment in the Dockerfile regarding the number of documented patches, and adding strict argument validation to the build script to avoid silently ignoring invalid flags.
| WORKDIR /app | ||
|
|
||
| # System deps: tzdata is needed for ZoneInfo on slim images. | ||
| RUN apt-get update \ | ||
| && apt-get install -y --no-install-recommends tzdata ca-certificates \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| COPY requirements.txt ./ | ||
| RUN pip install -r requirements.txt | ||
|
|
||
| COPY mcp_proxy_only.py ./ | ||
|
|
||
| EXPOSE 5001 | ||
|
|
||
| # Cloud Run health probes hit the agent on PROXY_PORT; the agent is the | ||
| # sidecar container, so the ingress container's nginx reverse-proxies | ||
| # /agent/* to 127.0.0.1:${PROXY_PORT}. | ||
| CMD ["python", "-u", "mcp_proxy_only.py"] |
There was a problem hiding this comment.
Running the container as root poses a security risk. Additionally, if the deployment environment enforces a non-root user, the application will crash with a PermissionError when attempting to write config.json to /app (Patch 7 bootstrap).
To resolve this, create a dedicated non-root user, change the ownership of the /app directory to this user, and run the container using the USER directive.
WORKDIR /app
# System deps: tzdata is needed for ZoneInfo on slim images.
RUN apt-get update \
&& apt-get install -y --no-install-recommends tzdata ca-certificates \
&& rm -rf /var/lib/apt/lists/*
# Create a non-root user and group
RUN groupadd -g 10001 appuser \
&& useradd -u 10001 -g appuser -m -s /sbin/nologin appuser
COPY requirements.txt ./
RUN pip install -r requirements.txt
COPY mcp_proxy_only.py ./
# Ensure the non-root user owns /app to write config.json at runtime
RUN chown -R appuser:appuser /app
USER appuser
EXPOSE 5001
# Cloud Run health probes hit the agent on PROXY_PORT; the agent is the
# sidecar container, so the ingress container's nginx reverse-proxies
# /agent/* to 127.0.0.1:${PROXY_PORT}.
CMD ["python", "-u", "mcp_proxy_only.py"]
| @@ -0,0 +1,36 @@ | |||
| # Sidecar agent for the Custom Data Commons multi-container Cloud Run service. | |||
| # Vendored from upstream additional_features/mcp_proxy_only.py (Apache-2.0) | |||
| # with six documented patches in PATCHES.md. | |||
| if [[ "${1:-}" == "--push" ]]; then | ||
| echo "Pushing ${FULL_IMAGE}" | ||
| docker push "${FULL_IMAGE}" | ||
| docker push "${AR_REGION}-docker.pkg.dev/${PROJECT}/${AR_REPO}/${IMAGE_NAME}:latest" | ||
| echo "Done. Tag for tfvars: ${TAG}" | ||
| fi |
There was a problem hiding this comment.
Currently, any argument other than --push is silently ignored, which can lead to unexpected behavior (e.g., running ./build.sh --help or ./build.sh -p will build the image but silently skip pushing without warning the user).
Enforce strict argument validation to improve usability and prevent silent failures.
| if [[ "${1:-}" == "--push" ]]; then | |
| echo "Pushing ${FULL_IMAGE}" | |
| docker push "${FULL_IMAGE}" | |
| docker push "${AR_REGION}-docker.pkg.dev/${PROJECT}/${AR_REPO}/${IMAGE_NAME}:latest" | |
| echo "Done. Tag for tfvars: ${TAG}" | |
| fi | |
| if [[ $# -gt 0 ]]; then | |
| if [[ "${1}" == "--push" ]]; then | |
| echo "Pushing ${FULL_IMAGE}" | |
| docker push "${FULL_IMAGE}" | |
| docker push "${AR_REGION}-docker.pkg.dev/${PROJECT}/${AR_REPO}/${IMAGE_NAME}:latest" | |
| echo "Done. Tag for tfvars: ${TAG}" | |
| else | |
| echo "Error: Invalid argument '${1}'" >&2 | |
| echo "Usage: $0 [--push]" >&2 | |
| exit 1 | |
| fi | |
| fi |
e7ee12b to
fe35a51
Compare
Adds the backend the narratives UI talks to at /agent/chat/stream: a Flask SSE proxy that runs the MCP tool loop, optional knowledge-base lookup, and Gemini synthesis, streaming thoughts/text/tool-calls/chart-config back to the client. Mirrors the agent/ layout from the downstream app. - agent/mcp_proxy_only.py — the proxy + streaming chat pipeline - agent/requirements.txt, Dockerfile, build.sh — deploy API keys are read from Secret Manager / config.json / env at runtime; none are committed. Comments describe current behavior (no change-history narration).
fe35a51 to
6288be2
Compare
| .gemini | ||
| .figma_pat | ||
| *.py | ||
| # ...but the agent sidecar is Python and must stay tracked. |
There was a problem hiding this comment.
I don't understand what this comment is trying to convey.
| -t "${FULL_IMAGE}" \ | ||
| -t "${AR_REGION}-docker.pkg.dev/${PROJECT}/${AR_REPO}/${IMAGE_NAME}:latest" \ | ||
| --load \ | ||
| . |
There was a problem hiding this comment.
Instead of relying on the user to be in the agent folder when running this script, it'd be better to be explicit about the folder context for docker. Something like:
DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
And then use
docker buildx build \
--platform linux/amd64 \
-t "${FULL_IMAGE}" \
-t "${AR_REGION}-docker.pkg.dev/${PROJECT}/${AR_REPO}/${IMAGE_NAME}:latest" \
--load \
${DIR}
| IMAGE_NAME="agent" | ||
| TAG="$(git rev-parse --short=12 HEAD)" | ||
|
|
||
| FULL_IMAGE="${AR_REGION}-docker.pkg.dev/${PROJECT}/${AR_REPO}/${IMAGE_NAME}:${TAG}" |
There was a problem hiding this comment.
To reduce code duplication, it'd be nice to have something like:
IMAGE_BASE="${AR_REGION}-docker.pkg.dev/${PROJECT}/${AR_REPO}/${IMAGE_NAME}"
FULL_IMAGE="${IMAGE_BASE}:${TAG}"
LATEST_IMAGE="${IMAGE_BASE}:latest"
And then use ${LATEST_IMAGE} on lines 27 and 42
| fi | ||
| echo " arch verified: ${ARCH}" | ||
|
|
||
| if [[ "${1:-}" == "--push" ]]; then |
There was a problem hiding this comment.
[Suggestion] instead of just checking the first argument, you might consider swapping to looping over $@ and checking for flags, so we could add support for other flags (like --help). It'd be good to have a --help flag that prints out usage information in general.
There was a problem hiding this comment.
This file is very large, to the point that it's a little unwieldy to maintain. Let's separate into multiple files. Here's one Gemini suggested refactoring, but you're welcome to split into something else that works better.
narratives/agent/
├── Dockerfile
├── build.sh
├── requirements.txt
├── main.py # App entrypoint (Flask / Gunicorn runner)
└── src/ # Core application package
├── __init__.py
├── config.py # Config loading, GCS fetch, Secret Manager
├── session_logger.py # Audit logger, path sanitization, token tracking
├── mcp/
│ ├── __init__.py
│ ├── client.py # JSON-RPC client & tool invocation
│ ├── schema.py # OpenAPI schema transform for Gemini
│ └── data_utils.py # Provenance extraction & data availability
├── gemini/
│ ├── __init__.py
│ ├── client.py # Gemini REST client, key rotation, streaming
│ └── schemas.py # JSON schemas (charts, validation, follow-ups)
├── workflows/
│ ├── __init__.py
│ ├── mcp_loop.py # Iterative MCP tool-calling loop
│ ├── kb_search.py # Knowledge Base (file search / RAG) query
│ ├── chart_config.py # Parallel chart config extraction & validation
│ └── follow_up.py # Self-contained follow-up question generator
├── analytics/
│ ├── __init__.py
│ └── log_parser.py # Log parsing & analytics dashboard metrics
└── server/
├── __init__.py
├── app.py # Flask app initialization & CORS setup
├── sse.py # SSE event helper utilities
└── routes.py # API endpoints & SSE streaming pipeline
Generally, I'd like to keep @app.route()s vs client handling vs util functions separate. Relevant constants can go at the top of each file or in their own dedicated file.
Overview
Adds the agent sidecar the narratives UI depends on. Until now
narratives/shipped only the front end, which talks to
/agent/chat/stream; this PR addsthat backend under
narratives/agent/, mirroring the layout used in thedownstream app.
What it is
A Flask server (
mcp_proxy_only.py) that proxies the chat pipeline and streamsServer-Sent Events back to the UI:
follow-up questions.
It emits the
session_id,tool_call,thought,text,chart_config,*_sources,usage, anddoneevents the UI already consumes.Files
agent/mcp_proxy_only.py— proxy + streaming chat pipelineagent/requirements.txt— Python depsagent/Dockerfile,agent/build.sh— container build / deploynarratives/.gitignore— negation so the agent's Python stays tracked(the project-level
*.pyignore otherwise excluded it)Security
API keys are read at runtime from Secret Manager (
GEMINI_API_KEYS_SECRET),config.json, or environment variables — none are committed.config.jsonis not included (kept out of version control, as in the source app).
Testing done
python3 -m py_compile agent/mcp_proxy_only.py— clean.