Conversation
…ust numpy installation
…all requirements from a file
Signed-off-by: Hyungyu Kim <kimhg@robotis.com>
…rfile and update PYTHONPATH
Signed-off-by: Hyungyu Kim <kimhg@robotis.com>
… into feature-system
…d adjust PYTHONPATH
…agent path to talos package. Adjust permissions and clean up Dockerfile for better organization of services.
…hysical AI server, simplifying the setup by removing the build context and Dockerfile specification.
…rvice startup diagnostics
Add s6-overlay and Talos system manager support for physical_ai_server
Summary of ChangesHello @GyuH13, 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 primarily focuses on bumping the project version to Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces "s6-overlay" for service management in Docker containers and updates package versions to "0.8.1", which is a positive step for robust service handling. However, a security audit revealed critical vulnerabilities, including world-writable directory permissions for a host-mapped socket directory and potential command injection in shell scripts due to insufficient input sanitization. These issues could lead to unauthorized access or arbitrary command execution. Additionally, the review suggests improving reproducibility by pinning "git clone" to a specific commit, adjusting overly permissive file modes, and cleaning up redundant script code.
|
|
||
| # Ensure socket directory exists | ||
| mkdir -p /var/run/agent | ||
| chmod 777 /var/run/agent |
There was a problem hiding this comment.
The script sets permissions of /var/run/agent to 777 (world-writable), which is a critical security vulnerability. This directory is mounted from the host, making the host directory /var/run/robotis/agent_sockets/physical_ai_server world-writable. This allows any user on the host to modify, delete, or replace the Unix Domain Socket, potentially leading to Denial of Service or unauthorized access. It is recommended to use more restrictive permissions like 755.
mkdir -p /var/run/agent
chmod 755 /var/run/agent
| SERVICE_NAME="${SERVICE_NAME}" | ||
| if [ -z "${SERVICE_NAME}" ]; then | ||
| echo "Error: SERVICE_NAME environment variable must be set" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The script is vulnerable to command injection via the SERVICE_NAME environment variable. The variable is used to construct a file path for redirection on line 39 and is part of a command string executed via bash -c on line 59. If SERVICE_NAME contains shell metacharacters (e.g., ;, $(...)), they will be executed by the shell. As a 'reusable template', this script should strictly validate or sanitize its inputs.
| SERVICE_NAME="${SERVICE_NAME}" | |
| if [ -z "${SERVICE_NAME}" ]; then | |
| echo "Error: SERVICE_NAME environment variable must be set" >&2 | |
| exit 1 | |
| fi | |
| # Sanitize SERVICE_NAME to allow only alphanumeric characters, underscores, and hyphens | |
| SERVICE_NAME=$(echo "${SERVICE_NAME}" | tr -cd '[:alnum:]_-') | |
| if [ -z "${SERVICE_NAME}" ]; then | |
| echo "Error: SERVICE_NAME environment variable is invalid or empty" >&2 | |
| exit 1 | |
| fi |
| RUN mkdir -p /var/run/agent && \ | ||
| chmod 777 /var/run/agent | ||
|
|
||
| RUN git clone -b main https://github.com/ROBOTIS-GIT/talos.git /opt/talos |
There was a problem hiding this comment.
Cloning the main branch of the talos repository makes the Docker build non-reproducible. Changes to the main branch can introduce unexpected issues or break the build in the future. It is strongly recommended to pin this dependency to a specific git commit hash or tag to ensure deterministic and stable builds.
| RUN mkdir -p /var/run/agent && \ | ||
| chmod 777 /var/run/agent | ||
|
|
||
| RUN git clone -b main https://github.com/ROBOTIS-GIT/talos.git /opt/talos |
There was a problem hiding this comment.
Cloning the main branch of the talos repository makes the Docker build non-reproducible. Changes to the main branch can introduce unexpected issues or break the build in the future. It is strongly recommended to pin this dependency to a specific git commit hash or tag to ensure deterministic and stable builds.
|
|
||
| # Create directory for agent Unix Domain Socket | ||
| RUN mkdir -p /var/run/agent && \ | ||
| chmod 777 /var/run/agent |
There was a problem hiding this comment.
The Dockerfile sets permissions of /var/run/agent to 777 (world-writable), which is insecure. This allows any user within the container to modify its contents, and when combined with the volume mount, also affects the host system's directory permissions. This directory creation and permission setting is also redundant, as it's handled in the docker/s6-agent/run script at service startup, and /var/run often doesn't persist from image build. It's better to manage this exclusively in the runtime script with more restrictive permissions.
RUN mkdir -p /var/run/agent && \
chmod 755 /var/run/agent
|
|
||
| # Create directory for agent Unix Domain Socket | ||
| RUN mkdir -p /var/run/agent && \ | ||
| chmod 777 /var/run/agent |
There was a problem hiding this comment.
The Dockerfile sets permissions of /var/run/agent to 777 (world-writable), which is insecure. This allows any user within the container to modify its contents, and when combined with the volume mount, also affects the host system's directory permissions. This directory creation and permission setting is also redundant, as it's handled in the docker/s6-agent/run script at service startup, and /var/run often doesn't persist from image build. It's better to manage this exclusively in the runtime script with more restrictive permissions.
RUN mkdir -p /var/run/agent && \
chmod 755 /var/run/agent
| # Reusable ROS2 service finish script template | ||
| # This script runs when the service is stopped. | ||
| # Usage: SERVICE_NAME=<name> /path/to/ros2_service_finish.sh [exit_code] | ||
|
|
There was a problem hiding this comment.
For improved robustness and to prevent unexpected behavior in case of errors, it's recommended to add set -e at the beginning of this script. This ensures that the script will exit immediately if any command fails. Most other new scripts in this pull request already follow this practice.
| set -e | |
| export SERVICE_NAME="${SERVICE_NAME}" | ||
| export ROS2_COMMAND="${ROS2_COMMAND}" |
There was a problem hiding this comment.
These export commands are redundant because the variables are passed to the exec command on line 16 using env. Removing them will make the script cleaner and less prone to confusion.
There was a problem hiding this comment.
Pull request overview
Updates the repository release to 0.8.1 across ROS 2 packages and the web UI, and extends the physical_ai_server container runtime to use s6-overlay with bundled services (including an agent via Talos).
Changes:
- Bump versions to 0.8.1 across multiple packages (ROS
package.xml, Pythonsetup.py, Nodepackage.json/lockfile). - Add 0.8.1 entries to package changelogs.
- Introduce s6-overlay + s6-rc service definitions and switch the
physical_ai_servercontainer entrypoint to/init, with docker-compose updates for the agent socket mount.
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| rosbag_recorder/package.xml | Version bump to 0.8.1 |
| rosbag_recorder/CHANGELOG.rst | Add 0.8.1 changelog entry |
| physical_ai_tools/package.xml | Version bump to 0.8.1 |
| physical_ai_tools/CHANGELOG.rst | Add 0.8.1 changelog entry noting s6-related additions |
| physical_ai_server/setup.py | Python package version bump to 0.8.1 |
| physical_ai_server/package.xml | ROS package version bump to 0.8.1 |
| physical_ai_server/Dockerfile.arm64 | Install/configure s6-overlay, Talos agent, and s6 services; set /init entrypoint |
| physical_ai_server/Dockerfile.amd64 | Install/configure s6-overlay, Talos agent, and s6 services; set /init entrypoint |
| physical_ai_server/CHANGELOG.rst | Add 0.8.1 changelog entry noting s6-related additions |
| physical_ai_manager/package.json | Version bump to 0.8.1 |
| physical_ai_manager/package-lock.json | Lockfile version bump to 0.8.1 |
| physical_ai_manager/CHANGELOG.rst | Add 0.8.1 changelog entry |
| physical_ai_interfaces/package.xml | Version bump to 0.8.1 |
| physical_ai_interfaces/CHANGELOG.rst | Add 0.8.1 changelog entry |
| physical_ai_bt/setup.py | Python package version bump to 0.8.1 |
| physical_ai_bt/package.xml | Version bump to 0.8.1 |
| physical_ai_bt/CHANGELOG.rst | Add 0.8.1 changelog entry |
| docker/s6-services/physical_ai_server/type | Define physical_ai_server s6-rc service type |
| docker/s6-services/physical_ai_server/run | Run script delegating to common ROS2 service runner |
| docker/s6-services/physical_ai_server/producer-for | Wire service output to log consumer |
| docker/s6-services/physical_ai_server/finish | Finish script delegating to common ROS2 service finisher |
| docker/s6-services/physical_ai_server/dependencies | Service dependency file (currently empty) |
| docker/s6-services/physical_ai_server-log/type | Define log service type |
| docker/s6-services/physical_ai_server-log/run | Log service run script using logutil-service |
| docker/s6-services/physical_ai_server-log/pipeline-name | Define pipeline name for log/service pairing |
| docker/s6-services/physical_ai_server-log/consumer-for | Wire log service as consumer of main service |
| docker/s6-services/common/ros2_service_run.sh | Common ROS2 launch wrapper for s6 services |
| docker/s6-services/common/ros2_service_finish.sh | Common service shutdown helper (PGID-based) |
| docker/s6-agent/type | Define s6-agent s6-rc service type |
| docker/s6-agent/run | Run uvicorn agent over UDS socket |
| docker/s6-agent/finish | Cleanup agent socket on stop |
| docker/s6-agent/dependencies | Service dependency file (currently empty) |
| docker/docker-compose.yml | Mount host agent socket directory into container; remove interactive shell command |
Files not reviewed (1)
- physical_ai_manager/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ARG S6_OVERLAY_VERSION=3.2.1.0 | ||
|
|
||
| # Install s6-overlay | ||
| ADD https://github.com/just-containers/s6-overlay/releases/download/v${S6_OVERLAY_VERSION}/s6-overlay-noarch.tar.xz /tmp | ||
| RUN tar -C / -Jxpf /tmp/s6-overlay-noarch.tar.xz | ||
| ADD https://github.com/just-containers/s6-overlay/releases/download/v${S6_OVERLAY_VERSION}/s6-overlay-aarch64.tar.xz /tmp | ||
| RUN tar -C / -Jxpf /tmp/s6-overlay-aarch64.tar.xz |
There was a problem hiding this comment.
s6-overlay tarballs are extracted with tar -J before xz-utils is installed. This makes the Docker build rely on xz support being present in the base image; move the apt-get install xz-utils step (or otherwise ensure xz is available) before these tar -Jxpf commands to avoid non-reproducible build failures.
|
|
||
| # Create directory for agent Unix Domain Socket | ||
| RUN mkdir -p /var/run/agent && \ | ||
| chmod 777 /var/run/agent |
There was a problem hiding this comment.
chmod 777 /var/run/agent makes the socket directory world-writable. Prefer a tighter permission model (e.g., 0755 or 0770 with a dedicated group/user) to reduce the risk of unintended write access when the directory is bind-mounted from the host.
| chmod 777 /var/run/agent | |
| chmod 755 /var/run/agent |
| RUN git clone -b main https://github.com/ROBOTIS-GIT/talos.git /opt/talos | ||
|
|
There was a problem hiding this comment.
git clone -b main ... /opt/talos makes the image build non-deterministic because the contents can change over time. Pin to a tag or a specific commit SHA (and consider shallow clone) to improve reproducibility and make rollbacks/auditing possible.
| ARG S6_OVERLAY_VERSION=3.2.1.0 | ||
|
|
||
| # Install s6-overlay | ||
| ADD https://github.com/just-containers/s6-overlay/releases/download/v${S6_OVERLAY_VERSION}/s6-overlay-noarch.tar.xz /tmp | ||
| RUN tar -C / -Jxpf /tmp/s6-overlay-noarch.tar.xz | ||
| ADD https://github.com/just-containers/s6-overlay/releases/download/v${S6_OVERLAY_VERSION}/s6-overlay-x86_64.tar.xz /tmp | ||
| RUN tar -C / -Jxpf /tmp/s6-overlay-x86_64.tar.xz |
There was a problem hiding this comment.
s6-overlay tarballs are extracted with tar -J before xz-utils is installed. This makes the Docker build rely on xz support being present in the base image; move the apt-get install xz-utils step (or otherwise ensure xz is available) before these tar -Jxpf commands to avoid non-reproducible build failures.
|
|
||
| # Create directory for agent Unix Domain Socket | ||
| RUN mkdir -p /var/run/agent && \ | ||
| chmod 777 /var/run/agent |
There was a problem hiding this comment.
chmod 777 /var/run/agent makes the socket directory world-writable. Prefer a tighter permission model (e.g., 0755 or 0770 with a dedicated group/user) to reduce the risk of unintended write access when the directory is bind-mounted from the host.
| chmod 777 /var/run/agent | |
| chmod 0755 /var/run/agent |
| RUN git clone -b main https://github.com/ROBOTIS-GIT/talos.git /opt/talos | ||
|
|
There was a problem hiding this comment.
git clone -b main ... /opt/talos makes the image build non-deterministic because the contents can change over time. Pin to a tag or a specific commit SHA (and consider shallow clone) to improve reproducibility and make rollbacks/auditing possible.
| FROM robotis/ros:jazzy-ros-base-torch2.7.0-cuda12.8.0 AS physical-ai-tools | ||
|
|
||
| ARG S6_OVERLAY_VERSION=3.2.1.0 | ||
|
|
||
| # Install s6-overlay | ||
| ADD https://github.com/just-containers/s6-overlay/releases/download/v${S6_OVERLAY_VERSION}/s6-overlay-noarch.tar.xz /tmp |
There was a problem hiding this comment.
PR title suggests a pure version bump, but this change set also introduces a new init system (s6-overlay), adds new longrun services, and changes container entrypoint behavior. Please update the PR title/description to reflect the operational change so reviewers/release notes capture the real scope.
2 participants
No description provided.