Migration to docker #41
Conversation
…hrome configuration
…g Chrome dependencies
…wser service and FastAPI application
… files to use new repository format
… Service in build-images.yml
…o install dependencies directly
WalkthroughThis PR introduces Docker containerization infrastructure for the application, including Dockerfiles for FastAPI and Browser Service with multi-stage builds, Docker Compose configurations for both development and production environments, a GitHub Actions workflow for automated image building and pushing, environment variable configurations for image tags and host paths, and backend code updates to support Docker-in-Docker volume mounting. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Fix all issues with AI Agents 🤖
In @.env.example:
- Line 23: The default .env entry uses HOST_ROBOT_TESTS_DIR=${PWD}/robot_tests
which will not be expanded by Docker Compose; update the .env.example to remove
the unexpandable ${PWD} and instead instruct users to set an absolute path (or
provide the suggested shell command), e.g., replace or annotate the
HOST_ROBOT_TESTS_DIR line to require an absolute path and add a short note
showing how to populate it via echo "HOST_ROBOT_TESTS_DIR=$(pwd)/robot_tests" >>
.env so users know to run that during setup.
In @docker-compose.dev.yml:
- Around line 21-26: Remove the redundant BROWSER_USE_SERVICE_URL environment
entry from docker-compose.dev.yml (the -
BROWSER_USE_SERVICE_URL=http://browser-service:4999 line) since it duplicates
the same key/value from the base docker-compose.yml; only keep environment
overrides in docker-compose.dev.yml for variables that differ from the base,
leaving DEBUG and LOG_LEVEL if they are intentionally set for dev.
In @docker-compose.yml:
- Around line 23-24: The docker-compose entries using ${PWD} for
HOST_ROBOT_TESTS_DIR and HOST_WORKSPACE_DIR are not portable across shells/OSs;
replace them with explicit Compose variable substitution and require users to
set HOST_ROBOT_TESTS_DIR and HOST_WORKSPACE_DIR in an .env file (or fall back to
COMPOSE_PROJECT_DIR-based relative paths), update docker-compose.yml to
reference those variables only, and add README instructions showing how to
export/set HOST_ROBOT_TESTS_DIR and HOST_WORKSPACE_DIR for Linux/macOS and
Windows (PowerShell) so CI and Windows users have correct absolute paths.
- Around line 84-88: The docker-compose declares named volumes (robot_tests,
logs, chroma_db, data) but services use bind mounts; either remove the unused
named volumes block or convert service mounts to use those named volumes
consistently: update service volume entries to reference
robot_tests:/app/robot_tests, logs:/path, chroma_db:/path, data:/path (replace
/path with the container paths already used), and if you choose named volumes
also change HOST_ROBOT_TESTS_DIR to the in-container path (/app/robot_tests) so
Docker-in-Docker behavior is correct.
- Line 17: The docker-compose service currently mounts the host Docker socket
(/var/run/docker.sock) which grants full daemon access; replace this direct
mount by adding or referencing a docker-socket-proxy sidecar (e.g.,
tecnativa/docker-socket-proxy) and update the service to use the proxy endpoint
instead of /var/run/docker.sock, document the security implications and chosen
mitigation in the README/docs, ensure the service runs with least-privilege
(avoid privileged mode, set read-only root filesystem where possible) and
configure the proxy to filter API calls to only the required Docker operations.
- Line 60: The environment variable BROWSER_SERVICE_URL set to
"http://browser-service:4999" in the browser-service container is redundant;
remove the BROWSER_SERVICE_URL entry from the browser-service service definition
in docker-compose.yml (or conditionally guard its injection) unless the
browser-service application actually reads that variable; search for uses of
BROWSER_SERVICE_URL in the codebase and only keep the variable when referenced
by other services (e.g., fastapi) to avoid configuration noise.
- Around line 26-27: Create the missing template file src/backend/.env.example
containing documented keys required by the app (at minimum GEMINI_API_KEY and
APP_PORT) with placeholder values and brief comments describing each variable;
ensure the file mirrors the .env layout referenced by docker-compose's env_file
and README (include any other environment variables used by backend services if
present), so contributors can copy src/backend/.env.example to src/backend/.env
and populate real secrets.
In @Dockerfile.browser-service:
- Around line 34-38: The ENV block sets PATH="/opt/venv/bin:$PATH" but no venv
is created and packages are installed to /usr/local; replace the invalid entry
with the actual binary location (e.g., PATH="/usr/local/bin:$PATH") or remove
/opt/venv/bin, ensuring PLAYWRIGHT_BROWSERS_PATH and other ENV vars remain
unchanged; mirror the same change applied in the other Dockerfile to keep both
Dockerfiles consistent.
- Around line 41-72: The piped command "wget -q -O -
https://dl-ssl.google.com/linux/linux_signing_key.pub | gpg --dearmor -o
/usr/share/keyrings/google-chrome-keyring.gpg" can hide wget failures because
shell pipefail isn't enabled; enable pipefail by running the RUN block under
bash with pipefail set (e.g., invoke bash -c 'set -o pipefail && ...' or change
the image SHELL to bash -o pipefail -c) so that any failure in wget causes the
whole RUN to fail, or alternatively avoid the pipe by downloading to a temporary
file then running gpg on that file. Ensure you target the RUN containing the
wget|gpg pipeline when applying the change.
In @Dockerfile.fastapi:
- Around line 50-53: The ENV line incorrectly prepends a non-existent virtualenv
path (/opt/venv/bin) to PATH; update the Dockerfile ENV declaration (the block
setting PYTHONUNBUFFERED, PYTHONDONTWRITEBYTECODE, PYTHONIOENCODING, PATH) to
either remove the PATH modification entirely or change it to the actual runtime
bin location used in the image (e.g., PATH="/usr/local/bin:$PATH") so installed
packages under /usr/local are found.
- Around line 44-82: The container currently runs as root; create a non-root
user and switch to it for runtime: after the apt-get installation block, add
steps to create a system user (e.g., appuser) and group, set /app ownership and
permissions (chown -R appuser:appuser /app /app/robot_tests /app/logs
/app/chroma_db /app/data), and drop privileges by adding a USER appuser before
the CMD; ensure HEALTHCHECK and CMD continue to reference the same port/path and
that any runtime files are writable by appuser.
- Around line 16-17: The Dockerfile installs the "uv" package without a pinned
version via the RUN pip install --no-cache-dir uv line; update this to pin a
specific version (or use a build ARG like UV_VERSION or reference a requirements
file) so the RUN pip install step installs uv at a fixed version (e.g.,
uv==<version>) to ensure reproducible builds and make future updates explicit.
In @src/backend/.env.example:
- Around line 78-79: The .env.example variables FASTAPI_IMAGE_TAG and
BROWSER_SERVICE_IMAGE_TAG use monkscode/...:latest but docker-compose.yml falls
back to devasy23/nlrf:fastapi-latest style tags, causing a mismatch; update
FASTAPI_IMAGE_TAG and BROWSER_SERVICE_IMAGE_TAG in src/backend/.env.example to
use the same registry and tag format as the docker-compose.yml fallbacks (e.g.,
devasy23/nlrf:fastapi-latest and devasy23/nlrf:browser-service-latest) or change
the docker-compose.yml defaults to match monkscode/...:latest—ensure both files
reference the identical image names/tags so the env variable names
FASTAPI_IMAGE_TAG and BROWSER_SERVICE_IMAGE_TAG resolve consistently.
In @src/backend/services/docker_service.py:
- Around line 170-181: run_test_in_container currently uses HOST_ROBOT_TESTS_DIR
directly for a bind mount which can cause cryptic Docker mount errors if it's
missing or not an absolute host path; before creating container_config validate
HOST_ROBOT_TESTS_DIR by checking os.path.exists(HOST_ROBOT_TESTS_DIR) and
os.path.isabs(HOST_ROBOT_TESTS_DIR) and if missing or not absolute, log a clear
error/warning (including the value of HOST_ROBOT_TESTS_DIR) and raise or return
early so container creation is skipped; update logging to include the validation
failure and reference the HOST_ROBOT_TESTS_DIR and run_id in messages so
failures are easier to debug.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/crew_ai/agents.py (1)
44-44: Remove or integrate the unused scrape_tool.The
scrape_toolis initialized but never used in any agent. Theelement_identifier_agentusesbatch_browser_use_tool(line 186), while other agents usekeyword_search_tool. This creates dead code and maintenance burden.🔎 Proposed fix to remove unused code
-# Initialize the tools -# Note: These are tool instances, not classes. CrewAI requires instantiated tools. -scrape_tool = ScrapeElementFromWebsiteTool() # Primary tool: Batch processing for multiple elements with full context batch_browser_use_tool = BatchBrowserUseTool()Alternatively, if this tool is intended for future use, please integrate it into one of the agents or add a comment explaining its purpose.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
.env.example.github/workflows/build-images.ymlDockerfile.browser-serviceDockerfile.fastapiDockerfile.test-runner.archivedocker-compose.dev.ymldocker-compose.ymlsrc/backend/.env.examplesrc/backend/crew_ai/agents.pysrc/backend/services/docker_service.py
🧰 Additional context used
🪛 Checkov (3.2.334)
Dockerfile.test-runner.archive
[low] 1-82: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[low] 1-82: Ensure that a user for the container has been created
(CKV_DOCKER_3)
Dockerfile.browser-service
[low] 1-95: Ensure that a user for the container has been created
(CKV_DOCKER_3)
Dockerfile.fastapi
[low] 1-82: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 11-11: [UnorderedKey] The BROWSER_SERVICE_IMAGE_TAG key should go before the FASTAPI_IMAGE_TAG key
(UnorderedKey)
src/backend/.env.example
[warning] 79-79: [UnorderedKey] The BROWSER_SERVICE_IMAGE_TAG key should go before the FASTAPI_IMAGE_TAG key
(UnorderedKey)
🪛 Hadolint (2.14.0)
Dockerfile.browser-service
[warning] 16-16: Pin versions in pip. Instead of pip install <package> use pip install <package>==<version> or pip install --requirement <requirements file>
(DL3013)
[warning] 41-41: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[warning] 41-41: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check
(DL4006)
Dockerfile.fastapi
[warning] 17-17: Pin versions in pip. Instead of pip install <package> use pip install <package>==<version> or pip install --requirement <requirements file>
(DL3013)
[warning] 20-20: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[info] 38-38: Multiple consecutive RUN instructions. Consider consolidation.
(DL3059)
[warning] 56-56: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
🪛 Ruff (0.14.10)
src/backend/services/docker_service.py
180-181: info() call on root logger
(LOG015)
181-181: Logging statement uses f-string
(G004)
🔇 Additional comments (16)
src/backend/crew_ai/agents.py (5)
14-14: LGTM!The import of
get_cleaned_llmis properly integrated and used in theget_llmfunction to provide robust LLM output parsing.
19-39: LGTM!The
get_llmfunction is well-documented and provides a clean abstraction for LLM initialization with automatic output cleaning. The implementation correctly delegates toget_cleaned_llmand includes helpful logging.
46-46: LGTM!The
batch_browser_use_toolis properly initialized and correctly integrated into theelement_identifier_agent(line 186). The comment clearly explains its purpose.
1-309: Clarify the connection to Docker migration.The PR title indicates "Migration to docker" and the AI summary mentions "backend code updates to support Docker-in-Docker volume mounting," but the changes in this file primarily involve tool refactoring (SeleniumScrapingTool → ScrapeElementFromWebsiteTool, BatchBrowserUseTool) and LLM wrapper improvements.
Could you clarify how these changes relate to the Docker migration? For example:
- Are the tool changes required for containerized browser automation?
- Does the cleaned LLM wrapper address Docker-specific issues?
- Are there Docker-related changes in the tool implementations themselves?
6-6: Migration to ScrapeElementFromWebsiteTool is complete and properly executed.No remaining references to SeleniumScrapingTool were found in the codebase. The new tool is correctly imported at line 6 and instantiated at line 44. No further action required for this migration.
Dockerfile.browser-service (1)
68-78: Verify if both Google Chrome and Playwright Chromium are required.The Dockerfile installs both
google-chrome-stable(line 71) and Playwright's Chromium (line 78). If only one browser is needed for your use case, consider removing the redundant installation to reduce image size.Dockerfile.test-runner.archive (1)
1-4: LGTM - Clear archive header.The archive header clearly indicates this file is deprecated and has been replaced by
Dockerfile.fastapiandDockerfile.browser-service. Consider deleting this file entirely if it's no longer needed, or moving it to adocs/orarchive/directory to keep the root clean.src/backend/.env.example (1)
81-86: Good documentation for Docker-in-Docker support.The
HOST_ROBOT_TESTS_DIRvariable is well-documented with clear guidance for both local development and Docker deployment scenarios..env.example (1)
25-27: LGTM - Clear reference to backend config.The comment appropriately directs users to
src/backend/.env.examplefor additional application settings.src/backend/services/docker_service.py (1)
18-21: LGTM - Docker-in-Docker host path support.The
HOST_ROBOT_TESTS_DIRenvironment variable with a sensible fallback to the local absolute path correctly addresses the Docker-in-Docker volume mounting challenge. The inline comment clearly explains the purpose..github/workflows/build-images.yml (2)
3-24: Good path coverage for workflow triggers.The workflow correctly triggers on changes to both
browser_service/andbrowser-service/directories, covering potential naming variations. The path filters are well-chosen to avoid unnecessary builds.
133-151: This code is correct as written.The
${{ env.IMAGE_NAME }}syntax properly expands in the run script. GitHub Actions processes${{ ... }}expressions at workflow parse time before the shell script executes, replacing them with their concrete values (devasy23/nlrf). The environment variable is defined at the workflow level and is available to all jobs, including the summary job. No changes are needed.docker-compose.dev.yml (3)
1-2: Good documentation of usage pattern.The comment clearly documents how to use this dev override file with the base compose. This helps prevent common mistakes like running
docker compose -f docker-compose.dev.yml upwithout the base file.Optional enhancement: Consider adding a dev-specific command to the README or Makefile:
.PHONY: dev dev: docker compose -f docker-compose.yml -f docker-compose.dev.yml up --build .PHONY: dev-down dev-down: docker compose -f docker-compose.yml -f docker-compose.dev.yml down
13-14: Concern about read-only mounts with hot-reload is already mitigated.The read-only source mounts are safe because
PYTHONDONTWRITEBYTECODE=1is already set inDockerfile.fastapi(lines 14 and 51), which prevents Python from writing bytecode files. No additional environment configuration is needed indocker-compose.dev.yml.Likely an incorrect or invalid review comment.
30-45: Remove the suggested command override—browser-service uses Flask, not FastAPI.The service runs with
app.run(debug=False, ...)and does not support uvicorn. If hot-reload is needed for development, enable Flask's debug mode via an environment variable instead:environment: - DEBUG=true - FLASK_ENV=developmentThen modify
tools/browser_use_service.pyto respect theFLASK_ENVvariable when callingapp.run().Likely an incorrect or invalid review comment.
docker-compose.yml (1)
39-39:curlis already properly installed in both Docker images.Verification confirms that
curlis explicitly installed in both Dockerfile.fastapi (line 60) and Dockerfile.browser-service (line 48) as part of the runtime dependencies, ensuring the healthchecks in docker-compose.yml will function correctly.
| # Example values: | ||
| # Linux/Mac: /home/user/Natural-Language-to-Robot-Framework/robot_tests | ||
| # Windows: C:/Users/Devasy/Documents/GitHub/Natural-Language-to-Robot-Framework/robot_tests | ||
| HOST_ROBOT_TESTS_DIR=${PWD}/robot_tests |
There was a problem hiding this comment.
${PWD} may not expand correctly in .env files.
Docker Compose does not perform shell variable expansion like ${PWD} when reading .env files. This default value will likely be interpreted literally as the string ${PWD}/robot_tests rather than the actual working directory.
🔎 Proposed fix
Update the comment to clarify this limitation and require users to set an absolute path:
# Example values:
# Linux/Mac: /home/user/Natural-Language-to-Robot-Framework/robot_tests
# Windows: C:/Users/Devasy/Documents/GitHub/Natural-Language-to-Robot-Framework/robot_tests
-HOST_ROBOT_TESTS_DIR=${PWD}/robot_tests
+# IMPORTANT: Set this to your actual absolute path - ${PWD} does NOT expand in .env files
+HOST_ROBOT_TESTS_DIR=Alternatively, document that users should run echo "HOST_ROBOT_TESTS_DIR=$(pwd)/robot_tests" >> .env during setup.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HOST_ROBOT_TESTS_DIR=${PWD}/robot_tests | |
| # Example values: | |
| # Linux/Mac: /home/user/Natural-Language-to-Robot-Framework/robot_tests | |
| # Windows: C:/Users/Devasy/Documents/GitHub/Natural-Language-to-Robot-Framework/robot_tests | |
| # IMPORTANT: Set this to your actual absolute path - ${PWD} does NOT expand in .env files | |
| HOST_ROBOT_TESTS_DIR= |
🤖 Prompt for AI Agents
In @.env.example around line 23, The default .env entry uses
HOST_ROBOT_TESTS_DIR=${PWD}/robot_tests which will not be expanded by Docker
Compose; update the .env.example to remove the unexpandable ${PWD} and instead
instruct users to set an absolute path (or provide the suggested shell command),
e.g., replace or annotate the HOST_ROBOT_TESTS_DIR line to require an absolute
path and add a short note showing how to populate it via echo
"HOST_ROBOT_TESTS_DIR=$(pwd)/robot_tests" >> .env so users know to run that
during setup.
| - name: Build and push FastAPI image | ||
| uses: docker/build-push-action@v5 | ||
| with: | ||
| context: . | ||
| file: ./Dockerfile.fastapi | ||
| push: true | ||
| tags: ${{ steps.meta.outputs.tags }} | ||
| labels: ${{ steps.meta.outputs.labels }} | ||
| cache-from: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:fastapi-buildcache | ||
| cache-to: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:fastapi-buildcache,mode=max | ||
| platforms: linux/amd64 |
There was a problem hiding this comment.
Security: Avoid pushing images on pull requests.
The workflow pushes images on pull requests (push: true), which could be a security concern if secrets are available to fork PRs. While GitHub Actions typically doesn't expose secrets to fork PRs, it's safer to conditionally push only on push events to main/develop.
🔎 Proposed fix
- name: Build and push FastAPI image
uses: docker/build-push-action@v5
with:
context: .
file: ./Dockerfile.fastapi
- push: true
+ push: ${{ github.event_name == 'push' }}
tags: ${{ steps.meta.outputs.tags }}
labels: ${{ steps.meta.outputs.labels }}Apply the same change to the browser-service job (lines 114-124).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Build and push FastAPI image | |
| uses: docker/build-push-action@v5 | |
| with: | |
| context: . | |
| file: ./Dockerfile.fastapi | |
| push: true | |
| tags: ${{ steps.meta.outputs.tags }} | |
| labels: ${{ steps.meta.outputs.labels }} | |
| cache-from: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:fastapi-buildcache | |
| cache-to: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:fastapi-buildcache,mode=max | |
| platforms: linux/amd64 | |
| - name: Build and push FastAPI image | |
| uses: docker/build-push-action@v5 | |
| with: | |
| context: . | |
| file: ./Dockerfile.fastapi | |
| push: ${{ github.event_name == 'push' }} | |
| tags: ${{ steps.meta.outputs.tags }} | |
| labels: ${{ steps.meta.outputs.labels }} | |
| cache-from: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:fastapi-buildcache | |
| cache-to: type=registry,ref=${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:fastapi-buildcache,mode=max | |
| platforms: linux/amd64 |
| environment: | ||
| # Development environment | ||
| - DEBUG=true | ||
| - LOG_LEVEL=DEBUG | ||
| # Browser service URL for Docker network | ||
| - BROWSER_USE_SERVICE_URL=http://browser-service:4999 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant environment variables already in base compose.
The BROWSER_USE_SERVICE_URL environment variable (line 26) duplicates what's already defined in the base docker-compose.yml (line 21 with the same value). When using compose file merging, these duplicate definitions add noise without changing behavior.
🔎 Suggested cleanup
volumes:
# Mount source code for hot-reload
- ./src:/app/src:ro
- ./tools:/app/tools:ro
# Keep persistent data volumes from base compose
- ./robot_tests:/app/robot_tests
- ./logs:/app/logs
- ./chroma_db:/app/chroma_db
- ./data:/app/data
- /var/run/docker.sock:/var/run/docker.sock
environment:
# Development environment
- DEBUG=true
- LOG_LEVEL=DEBUG
- # Browser service URL for Docker network
- - BROWSER_USE_SERVICE_URL=http://browser-service:4999
# Override image tag to use locally built version
image: nlrf-fastapi:devOnly override environment variables in the dev compose if they differ from the base configuration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| environment: | |
| # Development environment | |
| - DEBUG=true | |
| - LOG_LEVEL=DEBUG | |
| # Browser service URL for Docker network | |
| - BROWSER_USE_SERVICE_URL=http://browser-service:4999 | |
| environment: | |
| # Development environment | |
| - DEBUG=true | |
| - LOG_LEVEL=DEBUG |
🤖 Prompt for AI Agents
In @docker-compose.dev.yml around lines 21-26, Remove the redundant
BROWSER_USE_SERVICE_URL environment entry from docker-compose.dev.yml (the -
BROWSER_USE_SERVICE_URL=http://browser-service:4999 line) since it duplicates
the same key/value from the base docker-compose.yml; only keep environment
overrides in docker-compose.dev.yml for variables that differ from the base,
leaving DEBUG and LOG_LEVEL if they are intentionally set for dev.
| - ./chroma_db:/app/chroma_db | ||
| - ./data:/app/data | ||
| # Docker socket for robot test execution (Docker-in-Docker) | ||
| - /var/run/docker.sock:/var/run/docker.sock |
There was a problem hiding this comment.
Security risk: Docker socket mount grants full Docker daemon access.
Mounting /var/run/docker.sock gives the container unrestricted access to the Docker daemon, effectively granting root-level control over the host. This is necessary for Docker-in-Docker but creates a significant security boundary crossing.
Consider these mitigations:
- Document the security implications in README/docs
- Use Docker socket proxy (e.g.,
tecnativa/docker-socket-proxy) with restricted permissions instead of direct socket mount - Implement least-privilege API filtering to allow only required Docker operations
- Run container with read-only root filesystem where possible
- Add security scanning for the spawned containers
🔎 Example using docker-socket-proxy
+ docker-proxy:
+ image: tecnativa/docker-socket-proxy
+ container_name: nlrf-docker-proxy
+ privileged: true
+ environment:
+ - CONTAINERS=1
+ - IMAGES=1
+ - POST=1
+ volumes:
+ - /var/run/docker.sock:/var/run/docker.sock:ro
+ networks:
+ - nlrf-network
+
fastapi:
image: ${FASTAPI_IMAGE_TAG:-devasy23/nlrf:fastapi-latest}
# ... existing config ...
volumes:
- ./robot_tests:/app/robot_tests
- ./logs:/app/logs
- ./chroma_db:/app/chroma_db
- ./data:/app/data
- - /var/run/docker.sock:/var/run/docker.sock
+ environment:
+ - DOCKER_HOST=tcp://docker-proxy:2375
# ... rest of config ...
+ depends_on:
+ docker-proxy:
+ condition: service_startedCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @docker-compose.yml around line 17, The docker-compose service currently
mounts the host Docker socket (/var/run/docker.sock) which grants full daemon
access; replace this direct mount by adding or referencing a docker-socket-proxy
sidecar (e.g., tecnativa/docker-socket-proxy) and update the service to use the
proxy endpoint instead of /var/run/docker.sock, document the security
implications and chosen mitigation in the README/docs, ensure the service runs
with least-privilege (avoid privileged mode, set read-only root filesystem where
possible) and configure the proxy to filter API calls to only the required
Docker operations.
| - HOST_ROBOT_TESTS_DIR=${PWD}/robot_tests | ||
| - HOST_WORKSPACE_DIR=${PWD} |
There was a problem hiding this comment.
${PWD} may not resolve correctly across different environments.
The ${PWD} variable behaves differently across shells, operating systems (especially Windows), and CI/CD environments. In Windows, ${PWD} might use forward slashes while Docker expects Windows-style paths, or might not be set at all in some contexts.
🔎 Portable alternatives
Option 1: Use Docker Compose variable substitution with .env file
In .env or src/backend/.env:
HOST_ROBOT_TESTS_DIR=/absolute/path/to/robot_tests
HOST_WORKSPACE_DIR=/absolute/path/to/workspaceThen reference as-is in docker-compose.yml (already done on lines 23-24).
Option 2: Use relative paths with COMPOSE_PROJECT_DIR
- - HOST_ROBOT_TESTS_DIR=${PWD}/robot_tests
- - HOST_WORKSPACE_DIR=${PWD}
+ - HOST_ROBOT_TESTS_DIR=${COMPOSE_PROJECT_DIR:-$(pwd)}/robot_tests
+ - HOST_WORKSPACE_DIR=${COMPOSE_PROJECT_DIR:-$(pwd)}Option 3: Document in README
Add clear instructions for users to set these in their .env file:
# For Linux/macOS
export HOST_ROBOT_TESTS_DIR=$(pwd)/robot_tests
export HOST_WORKSPACE_DIR=$(pwd)
# For Windows (PowerShell)
$env:HOST_ROBOT_TESTS_DIR="$PWD/robot_tests"
$env:HOST_WORKSPACE_DIR="$PWD"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @docker-compose.yml around lines 23-24, The docker-compose entries using
${PWD} for HOST_ROBOT_TESTS_DIR and HOST_WORKSPACE_DIR are not portable across
shells/OSs; replace them with explicit Compose variable substitution and require
users to set HOST_ROBOT_TESTS_DIR and HOST_WORKSPACE_DIR in an .env file (or
fall back to COMPOSE_PROJECT_DIR-based relative paths), update
docker-compose.yml to reference those variables only, and add README
instructions showing how to export/set HOST_ROBOT_TESTS_DIR and
HOST_WORKSPACE_DIR for Linux/macOS and Windows (PowerShell) so CI and Windows
users have correct absolute paths.
| # Install UV package manager | ||
| RUN pip install --no-cache-dir uv |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
latest stable version uv python package manager
💡 Result:
Latest stable uv release: 0.9.21 — released December 30, 2025. [1][2]
Sources:
[1] PyPI project page for uv.
[2] GitHub releases for astral-sh/uv.
Pin the uv package version for reproducible builds.
The uv package manager is installed without version pinning. For reproducible builds, consider pinning to a specific version.
🔎 Proposed fix
# Install UV package manager
-RUN pip install --no-cache-dir uv
+RUN pip install --no-cache-dir uv==0.9.21📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Install UV package manager | |
| RUN pip install --no-cache-dir uv | |
| # Install UV package manager | |
| RUN pip install --no-cache-dir uv==0.9.21 |
🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 17-17: Pin versions in pip. Instead of pip install <package> use pip install <package>==<version> or pip install --requirement <requirements file>
(DL3013)
🤖 Prompt for AI Agents
In @Dockerfile.fastapi around lines 16-17, The Dockerfile installs the "uv"
package without a pinned version via the RUN pip install --no-cache-dir uv line;
update this to pin a specific version (or use a build ARG like UV_VERSION or
reference a requirements file) so the RUN pip install step installs uv at a
fixed version (e.g., uv==<version>) to ensure reproducible builds and make
future updates explicit.
| FROM python:3.12-slim | ||
|
|
||
| # Set working directory | ||
| WORKDIR /app | ||
|
|
||
| # Prevent Python from writing pyc files and enable unbuffered output | ||
| ENV PYTHONUNBUFFERED=1 \ | ||
| PYTHONDONTWRITEBYTECODE=1 \ | ||
| PYTHONIOENCODING=utf-8 \ | ||
| PATH="/opt/venv/bin:$PATH" | ||
|
|
||
| # Install only runtime dependencies (curl for healthcheck, ca-certificates for HTTPS) | ||
| RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \ | ||
| --mount=type=cache,target=/var/lib/apt,sharing=locked \ | ||
| apt-get update && \ | ||
| apt-get install -y --no-install-recommends \ | ||
| curl \ | ||
| ca-certificates && \ | ||
| rm -rf /var/lib/apt/lists/* /var/cache/apt/* /usr/share/doc/* /usr/share/man/* /tmp/* | ||
|
|
||
| # Copy installed Python packages from builder stage | ||
| COPY --from=builder /build/install /usr/local | ||
|
|
||
| # Copy application source code | ||
| COPY src/ /app/src/ | ||
| COPY tools/ /app/tools/ | ||
|
|
||
| # Create necessary directories for persistent data | ||
| RUN mkdir -p /app/robot_tests /app/logs /app/chroma_db /app/data | ||
|
|
||
| # Expose FastAPI port | ||
| EXPOSE 5000 | ||
|
|
||
| # Health check endpoint | ||
| HEALTHCHECK --interval=30s --timeout=10s --start-period=40s --retries=3 \ | ||
| CMD curl -f http://localhost:5000/health || exit 1 | ||
|
|
||
| # Run FastAPI server with uvicorn | ||
| CMD ["uvicorn", "src.backend.main:app", "--host", "0.0.0.0", "--port", "5000"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Security: Container runs as root.
The container runs as root by default, which is a security risk for production deployments. Consider adding a non-root user for the runtime stage.
🔎 Proposed fix - add non-root user
Add after the apt-get installation block (around line 62):
rm -rf /var/lib/apt/lists/* /var/cache/apt/* /usr/share/doc/* /usr/share/man/* /tmp/*
+# Create non-root user for security
+RUN useradd --create-home --shell /bin/bash appuser
+
# Copy installed Python packages from builder stage
COPY --from=builder /build/install /usr/localThen add before the CMD instruction:
+# Switch to non-root user
+USER appuser
+
# Run FastAPI server with uvicorn
CMD ["uvicorn", "src.backend.main:app", "--host", "0.0.0.0", "--port", "5000"]Note: Ensure the /app directories have appropriate permissions for appuser.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FROM python:3.12-slim | |
| # Set working directory | |
| WORKDIR /app | |
| # Prevent Python from writing pyc files and enable unbuffered output | |
| ENV PYTHONUNBUFFERED=1 \ | |
| PYTHONDONTWRITEBYTECODE=1 \ | |
| PYTHONIOENCODING=utf-8 \ | |
| PATH="/opt/venv/bin:$PATH" | |
| # Install only runtime dependencies (curl for healthcheck, ca-certificates for HTTPS) | |
| RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \ | |
| --mount=type=cache,target=/var/lib/apt,sharing=locked \ | |
| apt-get update && \ | |
| apt-get install -y --no-install-recommends \ | |
| curl \ | |
| ca-certificates && \ | |
| rm -rf /var/lib/apt/lists/* /var/cache/apt/* /usr/share/doc/* /usr/share/man/* /tmp/* | |
| # Copy installed Python packages from builder stage | |
| COPY --from=builder /build/install /usr/local | |
| # Copy application source code | |
| COPY src/ /app/src/ | |
| COPY tools/ /app/tools/ | |
| # Create necessary directories for persistent data | |
| RUN mkdir -p /app/robot_tests /app/logs /app/chroma_db /app/data | |
| # Expose FastAPI port | |
| EXPOSE 5000 | |
| # Health check endpoint | |
| HEALTHCHECK --interval=30s --timeout=10s --start-period=40s --retries=3 \ | |
| CMD curl -f http://localhost:5000/health || exit 1 | |
| # Run FastAPI server with uvicorn | |
| CMD ["uvicorn", "src.backend.main:app", "--host", "0.0.0.0", "--port", "5000"] | |
| FROM python:3.12-slim | |
| # Set working directory | |
| WORKDIR /app | |
| # Prevent Python from writing pyc files and enable unbuffered output | |
| ENV PYTHONUNBUFFERED=1 \ | |
| PYTHONDONTWRITEBYTECODE=1 \ | |
| PYTHONIOENCODING=utf-8 \ | |
| PATH="/opt/venv/bin:$PATH" | |
| # Install only runtime dependencies (curl for healthcheck, ca-certificates for HTTPS) | |
| RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \ | |
| --mount=type=cache,target=/var/lib/apt,sharing=locked \ | |
| apt-get update && \ | |
| apt-get install -y --no-install-recommends \ | |
| curl \ | |
| ca-certificates && \ | |
| rm -rf /var/lib/apt/lists/* /var/cache/apt/* /usr/share/doc/* /usr/share/man/* /tmp/* | |
| # Create non-root user for security | |
| RUN useradd --create-home --shell /bin/bash appuser | |
| # Copy installed Python packages from builder stage | |
| COPY --from=builder /build/install /usr/local | |
| # Copy application source code | |
| COPY src/ /app/src/ | |
| COPY tools/ /app/tools/ | |
| # Create necessary directories for persistent data | |
| RUN mkdir -p /app/robot_tests /app/logs /app/chroma_db /app/data | |
| # Expose FastAPI port | |
| EXPOSE 5000 | |
| # Health check endpoint | |
| HEALTHCHECK --interval=30s --timeout=10s --start-period=40s --retries=3 \ | |
| CMD curl -f http://localhost:5000/health || exit 1 | |
| # Switch to non-root user | |
| USER appuser | |
| # Run FastAPI server with uvicorn | |
| CMD ["uvicorn", "src.backend.main:app", "--host", "0.0.0.0", "--port", "5000"] |
🧰 Tools
🪛 Checkov (3.2.334)
[low] 1-82: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.14.0)
[warning] 56-56: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
🤖 Prompt for AI Agents
In @Dockerfile.fastapi around lines 44-82, The container currently runs as root;
create a non-root user and switch to it for runtime: after the apt-get
installation block, add steps to create a system user (e.g., appuser) and group,
set /app ownership and permissions (chown -R appuser:appuser /app
/app/robot_tests /app/logs /app/chroma_db /app/data), and drop privileges by
adding a USER appuser before the CMD; ensure HEALTHCHECK and CMD continue to
reference the same port/path and that any runtime files are writable by appuser.
| ENV PYTHONUNBUFFERED=1 \ | ||
| PYTHONDONTWRITEBYTECODE=1 \ | ||
| PYTHONIOENCODING=utf-8 \ | ||
| PATH="/opt/venv/bin:$PATH" |
There was a problem hiding this comment.
Incorrect PATH configuration: /opt/venv/bin does not exist.
The PATH includes /opt/venv/bin, but no virtual environment is created in this Dockerfile. Python packages are installed to /build/install in the builder stage and copied to /usr/local in the runtime stage. This PATH entry is unnecessary and misleading.
🔎 Proposed fix
ENV PYTHONUNBUFFERED=1 \
PYTHONDONTWRITEBYTECODE=1 \
- PYTHONIOENCODING=utf-8 \
- PATH="/opt/venv/bin:$PATH"
+ PYTHONIOENCODING=utf-8📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ENV PYTHONUNBUFFERED=1 \ | |
| PYTHONDONTWRITEBYTECODE=1 \ | |
| PYTHONIOENCODING=utf-8 \ | |
| PATH="/opt/venv/bin:$PATH" | |
| ENV PYTHONUNBUFFERED=1 \ | |
| PYTHONDONTWRITEBYTECODE=1 \ | |
| PYTHONIOENCODING=utf-8 |
🤖 Prompt for AI Agents
In @Dockerfile.fastapi around lines 50-53, The ENV line incorrectly prepends a
non-existent virtualenv path (/opt/venv/bin) to PATH; update the Dockerfile ENV
declaration (the block setting PYTHONUNBUFFERED, PYTHONDONTWRITEBYTECODE,
PYTHONIOENCODING, PATH) to either remove the PATH modification entirely or
change it to the actual runtime bin location used in the image (e.g.,
PATH="/usr/local/bin:$PATH") so installed packages under /usr/local are found.
| FASTAPI_IMAGE_TAG=monkscode/nlrf-fastapi:latest | ||
| BROWSER_SERVICE_IMAGE_TAG=monkscode/nlrf-browser-service:latest |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify image tag formats across env files
echo "=== src/backend/.env.example ==="
grep -E "(FASTAPI_IMAGE_TAG|BROWSER_SERVICE_IMAGE_TAG)" src/backend/.env.example
echo ""
echo "=== .env.example ==="
grep -E "(FASTAPI_IMAGE_TAG|BROWSER_SERVICE_IMAGE_TAG)" .env.example
echo ""
echo "=== docker-compose files ==="
find . -name "docker-compose*.yml" -o -name "docker-compose*.yaml" | head -20Repository: monkscode/Natural-Language-to-Robot-Framework
Length of output: 441
🏁 Script executed:
#!/bin/bash
echo "=== docker-compose.yml ==="
grep -A 2 -B 2 "FASTAPI_IMAGE_TAG\|BROWSER_SERVICE_IMAGE_TAG\|image:" docker-compose.yml | head -40
echo ""
echo "=== docker-compose.dev.yml ==="
grep -A 2 -B 2 "FASTAPI_IMAGE_TAG\|BROWSER_SERVICE_IMAGE_TAG\|image:" docker-compose.dev.yml | head -40
echo ""
echo "=== Check env file references in docker-compose files ==="
grep "env_file" docker-compose.yml docker-compose.dev.ymlRepository: monkscode/Natural-Language-to-Robot-Framework
Length of output: 791
🏁 Script executed:
#!/bin/bash
echo "=== Full env_file references in docker-compose.yml ==="
grep -A 1 "env_file:" docker-compose.ymlRepository: monkscode/Natural-Language-to-Robot-Framework
Length of output: 228
Inconsistent image tag formats between environment files.
The image tags in src/backend/.env.example use monkscode/nlrf-fastapi:latest format, but docker-compose.yml references these variables with fallbacks to devasy23/nlrf:fastapi-latest format. Since docker-compose.yml loads env from ./src/backend/.env, this inconsistency creates a mismatch between the configuration template and the production defaults. Align both files to use the same registry and tag format.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 79-79: [UnorderedKey] The BROWSER_SERVICE_IMAGE_TAG key should go before the FASTAPI_IMAGE_TAG key
(UnorderedKey)
🤖 Prompt for AI Agents
In @src/backend/.env.example around lines 78-79, The .env.example variables
FASTAPI_IMAGE_TAG and BROWSER_SERVICE_IMAGE_TAG use monkscode/...:latest but
docker-compose.yml falls back to devasy23/nlrf:fastapi-latest style tags,
causing a mismatch; update FASTAPI_IMAGE_TAG and BROWSER_SERVICE_IMAGE_TAG in
src/backend/.env.example to use the same registry and tag format as the
docker-compose.yml fallbacks (e.g., devasy23/nlrf:fastapi-latest and
devasy23/nlrf:browser-service-latest) or change the docker-compose.yml defaults
to match monkscode/...:latest—ensure both files reference the identical image
names/tags so the env variable names FASTAPI_IMAGE_TAG and
BROWSER_SERVICE_IMAGE_TAG resolve consistently.
| # Docker-in-Docker: Use HOST_ROBOT_TESTS_DIR for volume mount to reference host path | ||
| container_config = { | ||
| "image": IMAGE_TAG, | ||
| "command": robot_command, | ||
| "volumes": {os.path.abspath(ROBOT_TESTS_DIR): {'bind': '/app/robot_tests', 'mode': 'rw'}}, | ||
| "volumes": {HOST_ROBOT_TESTS_DIR: {'bind': '/app/robot_tests', 'mode': 'rw'}}, | ||
| "working_dir": "/app", | ||
| "detach": True, # Run detached to manage container lifecycle | ||
| "auto_remove": False, # Don't auto-remove so we can get logs properly | ||
| "name": f"robot-test-{run_id}" # Give container a unique name | ||
| } | ||
| logging.info( | ||
| f"🐳 DOCKER SERVICE: Container config created for robot-test-{run_id}") | ||
| f"🐳 DOCKER SERVICE: Container config created for robot-test-{run_id} with volume {HOST_ROBOT_TESTS_DIR}:/app/robot_tests") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating HOST_ROBOT_TESTS_DIR before container creation.
If HOST_ROBOT_TESTS_DIR is misconfigured (doesn't exist on the host or is set to a container-internal path), the spawned test container will fail with a cryptic mount error. Adding early validation could provide a clearer error message.
🔎 Proposed validation
Add at the start of run_test_in_container:
# Validate HOST_ROBOT_TESTS_DIR for Docker-in-Docker scenarios
if os.getenv('HOST_ROBOT_TESTS_DIR') and not os.path.isabs(HOST_ROBOT_TESTS_DIR):
logging.warning(
f"⚠️ HOST_ROBOT_TESTS_DIR ({HOST_ROBOT_TESTS_DIR}) is not an absolute path. "
"This may cause volume mount failures in Docker-in-Docker scenarios."
)🧰 Tools
🪛 Ruff (0.14.10)
180-181: info() call on root logger
(LOG015)
181-181: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
In @src/backend/services/docker_service.py around lines 170-181,
run_test_in_container currently uses HOST_ROBOT_TESTS_DIR directly for a bind
mount which can cause cryptic Docker mount errors if it's missing or not an
absolute host path; before creating container_config validate
HOST_ROBOT_TESTS_DIR by checking os.path.exists(HOST_ROBOT_TESTS_DIR) and
os.path.isabs(HOST_ROBOT_TESTS_DIR) and if missing or not absolute, log a clear
error/warning (including the value of HOST_ROBOT_TESTS_DIR) and raise or return
early so container creation is skipped; update logging to include the validation
failure and reference the HOST_ROBOT_TESTS_DIR and run_id in messages so
failures are easier to debug.
1 participant
@coderabbitai review
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.