refactor: Rebase main branch to clean up messy commit history#53
Merged
Conversation
- Improve missing 'components' parameter error message for clearer startup failures. - Add constructible concept constraints for output registration and partner component creation. - Modernize component header declarations with safer defaulted destructor and std::size_t usage. Co-authored-by: Zihan Qin <zihanqin2048@gmail.com>
- Add compose environment variables for container user home mapping. - Switch submodule URL to https and route scripts through configurable RMCS_PATH. - Normalize shebang style and harden cleanup behavior for workspace artifacts. Co-authored-by: Zihan Qin <zihanqin2048@gmail.com>
- Add GitHub Actions workflow to build and push develop/runtime images. - Configure workflow to prepare SSH keys required by image build steps. - Add quick-start wiki link in README for faster onboarding. Co-authored-by: Zihan Qin <zihanqin2048@gmail.com>
- Add host-side rmcs launcher with zsh/nvim/neovide entry modes. - Add play-autoaim utility with remote copy and host playback options. - Wire zsh completion scripts and load completion path in env_setup.zsh. - Add foxglove launch helper script for local bridge startup. Co-authored-by: Zihan Qin <zihanqin2048@gmail.com>
- Upgrade Docker develop image with LLVM 22 toolchain setup and CMake 4.2 installation. - Add gstreamer packages and modernize clang/llvm alternatives configuration. - Improve build-rmcs with RMCS_PATH support, LLVM tool checks, and structured colcon args. - Add zsh completion proxy for build-rmcs to reuse colcon completion behavior. Co-authored-by: Zihan Qin <zihanqin2048@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough本PR增加了GitHub Actions工作流以构建和推送Docker镜像，添加了多个新的shell脚本工具（包括Foxglove启动、远程播放、Docker Compose管理），引入LLVM工具链和CMake支持到Docker构建，更新了环境变量配置，改进了C++模板约束，并优化了配置管理。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Member
Author
|
@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: 7
🧹 Nitpick comments (3)
.script/sync-remote (1)
11-12: RMCS_PATH 配置化改进很好，但建议添加验证。与
.script/clean-rmcs保持一致的RMCS_PATH配置化是正确的做法。不过，clean-rmcs中对RMCS_PATH为空或为/的情况进行了验证，而这里没有。虽然
sync-remote的操作风险较低（仅同步文件），但为了一致性和防御性编程，建议添加类似的验证：♻️ 建议添加路径验证
RMCS_PATH = os.getenv("RMCS_PATH") or "/workspaces/RMCS" + +if not RMCS_PATH or RMCS_PATH == "/": + raise ValueError(f"Invalid RMCS_PATH: '{RMCS_PATH}'") + SRC_DIR = os.path.join(RMCS_PATH, "rmcs_ws", "install")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.script/sync-remote around lines 11 - 12, The RMCS_PATH/SRC_DIR assignment in .script/sync-remote lacks the defensive validation present in clean-rmcs; update the script to validate RMCS_PATH after resolving (the RMCS_PATH variable used to build SRC_DIR) and reject empty or root ("/") values—log an error and exit if invalid—so that RMCS_PATH and derived SRC_DIR are never dangerously blank or "/". Ensure you reference and validate the RMCS_PATH variable before using it to compute SRC_DIR and mirror the same check/exit behavior used in clean-rmcs..script/clean-rmcs (1)
3-8: 路径验证逻辑可以简化。第 3 行使用了
${RMCS_PATH:-/workspaces/RMCS}语法，这意味着如果RMCS_PATH未设置或为空，会自动赋值为默认值。因此第 5 行的-z "${RMCS_PATH}"检查永远不会为真。可以简化验证逻辑，只保留
/检查：♻️ 简化验证逻辑
RMCS_PATH="${RMCS_PATH:-/workspaces/RMCS}" -if [[ -z "${RMCS_PATH}" || "${RMCS_PATH}" == "/" ]]; then +if [[ "${RMCS_PATH}" == "/" ]]; then echo "Invalid RMCS_PATH: '${RMCS_PATH}'" exit 1 fi或者，如果确实需要检测空值情况（防止未来代码变更引入问题），可以保留现有写法作为防御性编程。整体改动很好，使用
--和引号也是正确的安全实践。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.script/clean-rmcs around lines 3 - 8, The validation is redundant because RMCS_PATH is defaulted with ${RMCS_PATH:-/workspaces/RMCS}, so the [[ -z "${RMCS_PATH}" || "${RMCS_PATH}" == "/" ]] check will never hit the -z branch; update the conditional to only check for root (replace the [[ ... ]] test with [[ "${RMCS_PATH}" == "/" ]] ) or alternatively remove the default assignment and keep the current conditional—locate the RMCS_PATH assignment and the conditional block using RMCS_PATH and apply one of these two simplifications..script/host/rmcs (1)
5-6: 可选：整合用户配置，移除冗余的-u ubuntu标志。
ubuntu用户确实存在于镜像中（Dockerfile 第 135-140 行明确创建并配置），UID:GID1000:1000与 docker-compose.yml 第 4 行的user: "1000:1000"对应一致。当前实现在功能上是可行的。不过，用户配置现在分散在三个地方：
.env中的CONTAINER_USER=ubuntu、docker-compose.yml中的user: "1000:1000"、以及本脚本第 5 行的DEVELOPER_NAME="ubuntu"。为了简化维护，建议移除第 41 行的-u ubuntu标志，让docker compose exec继承服务用户配置，这样可以避免硬编码。建议修改
-readonly DEVELOPER_NAME="ubuntu" readonly NVIM_PATH="/opt/nvim-linux-x86_64/bin/nvim" readonly NVIM_PORT=6666 readonly NVIM_HOST="localhost" @@ - docker compose exec -u "$DEVELOPER_NAME" -d "$service" \ + docker compose exec -d "$service" \ "$NVIM_PATH" --headless --listen "$NVIM_HOST:$port"Also applies to: 41-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.script/host/rmcs around lines 5 - 6, Remove the hardcoded user flag from the docker compose exec invocation so the container command inherits the service user configuration instead of forcing -u ubuntu; update the script to stop using the DEVELOPER_NAME variable for the exec user (DEVELOPER_NAME="ubuntu" can remain if used elsewhere) and eliminate the explicit "-u ubuntu" argument in the command that calls docker compose exec (the spot that currently constructs the exec flags), ensuring NVIM_PATH (NVIM_PATH="/opt/nvim-linux-x86_64/bin/nvim") and other runtime variables are still passed through unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env:
- Around line 1-2: The .env file uses shell-style variable interpolation which
Docker Compose does not expand, so CONTAINER_HOME=/home/${CONTAINER_USER} will
remain literal; update the .env by hardcoding the resolved path for
CONTAINER_HOME (replace the interpolated value with the actual path, e.g.,
/home/ubuntu) so any references to ${CONTAINER_HOME} in docker-compose.yml
receive the correct value; ensure CONTAINER_USER remains if needed or remove if
unused.
In @.github/workflows/update-image.yml:
- Around line 28-43: The workflow currently writes the private key from
CONTAINER_ID_RSA into the build context (.ssh) and the Dockerfile copies
/tmp/.ssh/* into /home/ubuntu/.ssh, which bakes the secret into the pushed
rmcs-develop image; remove the "Set up SSH keys" step that writes files into
.ssh and stop adding secrets to the build context. Instead, switch the "Build
and push rmcs-develop:latest" step to use BuildKit ssh/secret mounts (enable
buildx/ssh support in docker/build-push-action and pass the key via the action's
ssh/secrets inputs) or update the Dockerfile to use RUN --mount=type=ssh (or
type=secret) so the private key is available only during build and not
persisted; alternatively, generate SSH keys at container startup (avoid copying
CONTAINER_ID_RSA into the image). Reference the workflow step names "Set up SSH
keys" and "Build and push rmcs-develop:latest" and the Dockerfile behavior that
copies /tmp/.ssh/* to /home/ubuntu/.ssh when making the change.
- Around line 3-9: The workflow's push trigger only watches 'Dockerfile', which
misses other files that affect the image; update the workflow 'on:' push.paths
configuration to include the Docker build inputs such as the scripts and source
directories referenced by the Dockerfile (e.g. add patterns for .script/**, any
template script directories, and rmcs_ws/src/**) so pushes that change those
files will also dispatch the image rebuild; ensure you keep the existing
'Dockerfile' entry and use glob patterns (/**) to capture nested changes.
In @.script/build-rmcs:
- Around line 38-45: The cmake_toolchain_args entries use space-separated quoted
strings (e.g. "-G Ninja", "-D CMAKE_MESSAGE_LOG_LEVEL=ERROR") which CMake
mis-parses; change each element to the concatenated form so each array item is a
single, correctly formatted CMake flag (e.g. "-GNinja",
"-DCMAKE_MESSAGE_LOG_LEVEL=ERROR", "-DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=lld",
"-DCMAKE_SHARED_LINKER_FLAGS=-fuse-ld=lld", "-DCMAKE_AR=$(command -v llvm-ar)",
"-DCMAKE_RANLIB=$(command -v llvm-ranlib)") by editing the cmake_toolchain_args
array to remove the spaces after -G and -D and join names/values.
In @.script/complete/_play-autoaim:
- Line 7: The completion entry for the --ip option is declared as a flag but the
CLI requires a parameter; update the --ip declaration in _play-autoaim so it
accepts an argument (e.g., change the flag-style "'--ip[IP address of monitor
host]'" to a form that declares a value, such as using an equals or parameter
placeholder like "'--ip=IP address of monitor host'" or "'--ip[IP address of
monitor host]:ip'"), ensuring the completion system switches into value-entry
mode for --ip to match the parser used by the script.
In @.script/play-autoaim:
- Around line 81-85: The --no-copy branch can point the remote player at a
non-existent file on MONITOR_HOST; update the logic around the ssh call that
runs "${MONITOR_PLAYER} '${SDP_PATH}'" so it first verifies the file exists on
the monitor host (e.g. run ssh "${MONITOR_USER}@${MONITOR_HOST}" "[ -f
'${SDP_PATH}' ]" and abort with a clear error if missing) or else change the
command to use a reachable URL/path; ensure you reference the same variables
MONITOR_USER, MONITOR_HOST, MONITOR_PLAYER and SDP_PATH and fail early with a
message when the remote file is absent.
In `@Dockerfile`:
- Around line 129-132: 在 Dockerfile 的 RUN 块中，先不要直接执行 the downloaded
install.sh;改为在下载后同时获取并验证其 SHA256 校验和（例如从同一 GitHub release 的 .sha256 或使用预定义的
EXPECTED_CMAKE_SHA256 build ARG），用 sha256sum 或 openssl dgst
比对校验值并在校验失败时退出构建，只有校验通过后才执行 bash install.sh 并在任何情况下清理安装脚本（install.sh）；目标准确定位到该
RUN 段、install.sh 文件名和 /opt/cmake/ 安装目标以便修改。
---
Nitpick comments:
In @.script/clean-rmcs:
- Around line 3-8: The validation is redundant because RMCS_PATH is defaulted
with ${RMCS_PATH:-/workspaces/RMCS}, so the [[ -z "${RMCS_PATH}" ||
"${RMCS_PATH}" == "/" ]] check will never hit the -z branch; update the
conditional to only check for root (replace the [[ ... ]] test with [[
"${RMCS_PATH}" == "/" ]] ) or alternatively remove the default assignment and
keep the current conditional—locate the RMCS_PATH assignment and the conditional
block using RMCS_PATH and apply one of these two simplifications.
In @.script/host/rmcs:
- Around line 5-6: Remove the hardcoded user flag from the docker compose exec
invocation so the container command inherits the service user configuration
instead of forcing -u ubuntu; update the script to stop using the DEVELOPER_NAME
variable for the exec user (DEVELOPER_NAME="ubuntu" can remain if used
elsewhere) and eliminate the explicit "-u ubuntu" argument in the command that
calls docker compose exec (the spot that currently constructs the exec flags),
ensuring NVIM_PATH (NVIM_PATH="/opt/nvim-linux-x86_64/bin/nvim") and other
runtime variables are still passed through unchanged.
In @.script/sync-remote:
- Around line 11-12: The RMCS_PATH/SRC_DIR assignment in .script/sync-remote
lacks the defensive validation present in clean-rmcs; update the script to
validate RMCS_PATH after resolving (the RMCS_PATH variable used to build
SRC_DIR) and reject empty or root ("/") values—log an error and exit if
invalid—so that RMCS_PATH and derived SRC_DIR are never dangerously blank or
"/". Ensure you reference and validate the RMCS_PATH variable before using it to
compute SRC_DIR and mirror the same check/exit behavior used in clean-rmcs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 699afba1-4f1f-4811-b956-bbb5728acdb4
📒 Files selected for processing (21)
.env.github/workflows/update-image.yml.gitmodules.script/attach-remote.script/build-rmcs.script/clean-rmcs.script/complete/_build-rmcs.script/complete/_play-autoaim.script/complete/_set-remote.script/foxglove.script/host/rmcs.script/launch-rmcs.script/play-autoaim.script/sync-remote.script/template/env_setup.bash.script/template/env_setup.zshDockerfileREADME.mddocker-compose.ymlrmcs_ws/src/rmcs_executor/include/rmcs_executor/component.hpprmcs_ws/src/rmcs_executor/src/main.cpp
Comment on lines
+3
to
+9
| on: | ||
| workflow_dispatch: | ||
| push: | ||
| paths: | ||
| - 'Dockerfile' | ||
| branches: | ||
| - main |
There was a problem hiding this comment.
自动触发范围太窄，会漏掉很多实际改变镜像内容的提交。
镜像还依赖 .script/**、模板脚本，以及 rmcs_ws/src（Dockerfile Line 60-63 的 rosdep install 直接读这里）。现在只监听 Dockerfile，这些路径变化后不会自动重建镜像，发布结果会悄悄落后于 main。
🔧 建议修正
push:
paths:
- 'Dockerfile'
+ - '.script/**'
+ - 'rmcs_ws/src/**'
+ - '.github/workflows/update-image.yml'
branches:
- main🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/update-image.yml around lines 3 - 9, The workflow's push
trigger only watches 'Dockerfile', which misses other files that affect the
image; update the workflow 'on:' push.paths configuration to include the Docker
build inputs such as the scripts and source directories referenced by the
Dockerfile (e.g. add patterns for .script/**, any template script directories,
and rmcs_ws/src/**) so pushes that change those files will also dispatch the
image rebuild; ensure you keep the existing 'Dockerfile' entry and use glob
patterns (/**) to capture nested changes.
Comment on lines
+81
to
+85
| else | ||
| echo "Skipping copy; opening remote SDP directly" | ||
| fi | ||
|
|
||
| ssh -f "${MONITOR_USER}@${MONITOR_HOST}" "DISPLAY=:0 ${MONITOR_PLAYER} '${SDP_PATH}' --network-caching=50" |
There was a problem hiding this comment.
--no-copy 分支没有保证监视主机上存在 SDP 文件。
Line 82 说“直接打开远端 SDP”，但 Line 85 实际上仍然是在 ${MONITOR_HOST} 上打开本地路径 ${SDP_PATH}。只要目标主机没有预先放好这个文件，这个分支就会把播放器指向不存在的路径。
建议修改
else
- echo "Skipping copy; opening remote SDP directly"
+ echo "--no-copy requires ${MONITOR_HOST} to already have ${SDP_PATH}" >&2
+ exit 1
fi📝 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.
Suggested change
| else | |
| echo "Skipping copy; opening remote SDP directly" | |
| fi | |
| ssh -f "${MONITOR_USER}@${MONITOR_HOST}" "DISPLAY=:0 ${MONITOR_PLAYER} '${SDP_PATH}' --network-caching=50" | |
| else | |
| echo "--no-copy requires ${MONITOR_HOST} to already have ${SDP_PATH}" >&2 | |
| exit 1 | |
| fi | |
| ssh -f "${MONITOR_HOST}" "DISPLAY=:0 ${MONITOR_PLAYER} '${SDP_PATH}' --network-caching=50" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.script/play-autoaim around lines 81 - 85, The --no-copy branch can point
the remote player at a non-existent file on MONITOR_HOST; update the logic
around the ssh call that runs "${MONITOR_PLAYER} '${SDP_PATH}'" so it first
verifies the file exists on the monitor host (e.g. run ssh
"${MONITOR_USER}@${MONITOR_HOST}" "[ -f '${SDP_PATH}' ]" and abort with a clear
error if missing) or else change the command to use a reachable URL/path; ensure
you reference the same variables MONITOR_USER, MONITOR_HOST, MONITOR_PLAYER and
SDP_PATH and fail early with a message when the remote file is absent.
Comment on lines
+129
to
+132
| # Install latest stable cmake for user ubuntu | ||
| RUN wget https://github.com/kitware/cmake/releases/download/v4.2.3/cmake-4.2.3-linux-x86_64.sh -O install.sh && \ | ||
| mkdir -p /opt/cmake/ && bash install.sh --skip-license --prefix=/opt/cmake/ --exclude-subdir && \ | ||
| rm install.sh |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -i dockerfile | head -20Repository: Alliance-Algorithm/RMCS
Length of output: 77
🏁 Script executed:
cat -n Dockerfile | head -150Repository: Alliance-Algorithm/RMCS
Length of output: 7833
🏁 Script executed:
web_search
CMake 4.2.3 release SHA256 checksum officialRepository: Alliance-Algorithm/RMCS
Length of output: 161
🌐 Web query:
CMake 4.2.3 release GitHub official checksums SHA256
💡 Result:
Official SHA-256 checksums shown on the GitHub release assets list for CMake v4.2.3 (released 2026-01-27) are: [1]
cmake-4.2.3-files-v1.json—e6ce51721f6409682aacb16f575d6e24510c19270dea77976a5354413f57380f[1]cmake-4.2.3-linux-aarch64.sh—8e65cb924c41e3f6c5c98d1900aeeab3cfe3494f18ed8e5e63b91022df710401[1]cmake-4.2.3-linux-aarch64.tar.gz—e529c75f18f27ba27c52b329efe7b1f98dc32ccc0c6d193c7ab343f888962672[1]cmake-4.2.3-linux-x86_64.sh—b760514fde7fc510fcd16e51a81a4d2687b1f051b263d40b6806789d3d9fd62c[1]cmake-4.2.3-linux-x86_64.tar.gz—5bb505d5e0cca0480a330f7f27ccf52c2b8b5214c5bba97df08899f5ef650c23[1]cmake-4.2.3-macos-universal.dmg—d3ac47185cbad9e264d7afac18eb964139f90a641f8442189e64d1da388f120d[1]cmake-4.2.3-macos-universal.tar.gz—c2302d3e9c48daabee5ea7c4db4b2b93b989bcc89dae8b760880e00120641b5b[1]cmake-4.2.3-macos10.10-universal.dmg—da2f41884400922fd396d774530a90e9f2ba8fbb404d8a1c629a02b767c78265[1]cmake-4.2.3-macos10.10-universal.tar.gz—910b965a6fc72928412dd369c957643ff17a0990cc2435a2573b04c1352d9ff3[1]cmake-4.2.3-SHA-256.txt(the release checksum file itself) —e47bde1d5fe4e68f548e468924a2291a1cb8656b7ad3911cefc4fdfb4583acad[1]
The full official per-asset list (including Windows and SunOS artifacts) is published as cmake-4.2.3-SHA-256.txt linked from the official download page. [2]
Sources: GitHub release assets listing [1], cmake.org download page (links to cmake-4.2.3-SHA-256.txt) [2].
在执行下载的 CMake 安装脚本前添加完整性校验。
执行来自网络的脚本而不进行完整性校验存在安全风险。CMake 官方在 GitHub 发布页面提供了 SHA256 校验和。建议在执行安装脚本前验证其完整性。
🔐 建议修正
+ARG CMAKE_SHA256=b760514fde7fc510fcd16e51a81a4d2687b1f051b263d40b6806789d3d9fd62c
RUN wget https://github.com/kitware/cmake/releases/download/v4.2.3/cmake-4.2.3-linux-x86_64.sh -O install.sh && \
+ echo "${CMAKE_SHA256} install.sh" | sha256sum -c - && \
mkdir -p /opt/cmake/ && bash install.sh --skip-license --prefix=/opt/cmake/ --exclude-subdir && \
rm install.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 129 - 132, 在 Dockerfile 的 RUN 块中，先不要直接执行 the
downloaded install.sh;改为在下载后同时获取并验证其 SHA256 校验和（例如从同一 GitHub release 的 .sha256
或使用预定义的 EXPECTED_CMAKE_SHA256 build ARG），用 sha256sum 或 openssl dgst
比对校验值并在校验失败时退出构建，只有校验通过后才执行 bash install.sh 并在任何情况下清理安装脚本（install.sh）；目标准确定位到该
RUN 段、install.sh 文件名和 /opt/cmake/ 安装目标以便修改。
qzhhhi
force-pushed
the
refactor/simplify
branch
from
c6982ee to
70a8811
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.script/play-autoaim (2)
85-85: SSH 命令可能因特殊字符导致问题。
${SDP_PATH}在远程 shell 中被单引号包裹是正确的，但如果路径包含单引号字符，仍可能导致命令注入或解析失败。考虑到当前SDP_PATH是硬编码的/tmp/auto_aim.sdp，这在实际中风险较低。另外，
-f标志会使 SSH 在执行前进入后台，这意味着脚本不会等待播放器启动完成。如果需要确认播放器成功启动，可能需要额外的检查逻辑。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.script/play-autoaim at line 85, The ssh invocation can break if SDP_PATH contains single quotes and using -f backgrounds ssh preventing confirmation of startup; update the ssh call that uses MONITOR_USER, MONITOR_HOST, MONITOR_PLAYER and SDP_PATH so the remote argument is safely escaped (e.g., use printf '%q' or pass SDP_PATH as a quoted/escaped remote argument or use a here-doc / bash -s to avoid raw single-quote interpolation) and remove or avoid -f (or add explicit post-ssh check) so the script can detect/confirm the player started successfully (implement a simple readiness check after ssh when using MONITOR_PLAYER).
61-72: 硬编码的remote:主机名缺乏灵活性。Line 62 中的
remote:是硬编码的 SSH 别名，用户无法通过命令行参数配置源远程主机。这与--ip可配置MONITOR_HOST的设计不一致。此外，Line 63-71 的交互式提示在非交互式环境（如 CI/CD 管道或后台任务）中可能导致脚本挂起或失败。
建议添加 `--remote-host` 参数以配置 SDP 源主机
MONITOR_USER="" MONITOR_PLAYER="vlc" MONITOR_HOST="localhost" +REMOTE_HOST="remote" SDP_PATH="/tmp/auto_aim.sdp"+ --remote-host) + if (($# < 2)); then + echo "Missing --remote-host argument" + usage + exit 1 + fi + REMOTE_HOST="$2" + shift 2 + ;;if [[ "${USE_REMOTE}" == true ]]; then - if ! scp "remote:${SDP_PATH}" "${SDP_PATH}"; then + if ! scp "${REMOTE_HOST}:${SDP_PATH}" "${SDP_PATH}"; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.script/play-autoaim around lines 61 - 72, The script hardcodes the SSH alias "remote:" when copying ${SDP_PATH} which prevents user configuration and uses an interactive read that will hang in CI; add a new CLI option/variable (e.g., --remote-host setting REMOTE_HOST, defaulting to MONITOR_HOST or "remote") and replace the hardcoded "remote:" in the scp invocation with "${REMOTE_HOST}:" so scp "remote:${SDP_PATH}" becomes scp "${REMOTE_HOST}:${SDP_PATH}"; also remove the blocking interactive prompt in the scp failure branch—instead honor a non-interactive mode or a --force/--continue flag (or exit with a clear error) and detect interactivity with a tty check (e.g., [[ -t 0 ]]) to decide behavior, updating references to USE_REMOTE and SDP_PATH accordingly..script/build-rmcs (1)
55-60: 考虑非 LLVM 构建时--cmake-args的行为。当
RMCS_USE_LLVM未启用时，cmake_args数组为空。虽然 colcon 能够正确处理空的--cmake-args，但可以考虑仅在数组非空时添加该参数，以避免传递空值。可选优化
-CLICOLOR_FORCE=1 NINJA_STATUS="" \ - colcon build "${colcon_args[@]}" "$@" --cmake-args "${cmake_args[@]}" +cmake_args_flag=() +if ((${`#cmake_args`[@]} > 0)); then + cmake_args_flag=(--cmake-args "${cmake_args[@]}") +fi + +CLICOLOR_FORCE=1 NINJA_STATUS="" \ + colcon build "${colcon_args[@]}" "$@" "${cmake_args_flag[@]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.script/build-rmcs around lines 55 - 60, cmake_args 可能为空（当 RMCS_USE_LLVM 未启用时），所以不要总是向 colcon build 传递空的 --cmake-args 参数；在调用 colcon build（行使用 colcon build "${colcon_args[@]}" "$@" --cmake-args "${cmake_args[@]}") 前检查 cmake_args 数组长度或内容（变量 cmake_args 和调用行 colcon build）并仅在数组非空时追加 --cmake-args 和其值，否则调用不带该选项。.script/complete/_build-rmcs (1)
32-32:_describe选项顺序不符合惯例。zsh 的
_describe函数通常将选项放在描述和数组参数之前。虽然当前写法可能仍能工作，但为了可读性和一致性，建议调整顺序。♻️ 建议调整
- _describe colcon completions -o nosort + _describe -o nosort 'colcon' completions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.script/complete/_build-rmcs at line 32, 找到调用 _describe 的地方（目前为 `_describe colcon completions -o nosort`）并将选项位置调整到描述字符串和数组参数之后以符合 zsh 约定；即把 `-o nosort` 从当前位置移到描述/数组参数之后，确保 `_describe` 的第一个实参为名字、第二个为描述字符串、第三个为候选数组，然后再跟选项（比如 `-o nosort`），保持其余参数顺序不变。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.script/complete/_build-rmcs:
- Around line 21-22: The COMP_POINT calculation is wrong when the cursor sits
inside the token "build-rmcs"; update the logic around _comp_line and
_comp_point to compute the true offset using the length difference between the
replacement and original token and the cursor's position relative to the token.
Specifically, locate the start index of "build-rmcs" in BUFFER, compute delta =
${`#new_token`} - ${`#old_token`} (new_token="colcon build",
old_token="build-rmcs"), and if CURSOR is after the token start adjust
_comp_point = CURSOR + delta (or keep CURSOR unchanged if before the token);
reference variables/functions: _comp_line, _comp_point, BUFFER, CURSOR and the
token "build-rmcs" when implementing this fix.
---
Nitpick comments:
In @.script/build-rmcs:
- Around line 55-60: cmake_args 可能为空（当 RMCS_USE_LLVM 未启用时），所以不要总是向 colcon build
传递空的 --cmake-args 参数；在调用 colcon build（行使用 colcon build "${colcon_args[@]}" "$@"
--cmake-args "${cmake_args[@]}") 前检查 cmake_args 数组长度或内容（变量 cmake_args 和调用行
colcon build）并仅在数组非空时追加 --cmake-args 和其值，否则调用不带该选项。
In @.script/complete/_build-rmcs:
- Line 32: 找到调用 _describe 的地方（目前为 `_describe colcon completions -o
nosort`）并将选项位置调整到描述字符串和数组参数之后以符合 zsh 约定；即把 `-o nosort` 从当前位置移到描述/数组参数之后，确保
`_describe` 的第一个实参为名字、第二个为描述字符串、第三个为候选数组，然后再跟选项（比如 `-o nosort`），保持其余参数顺序不变。
In @.script/play-autoaim:
- Line 85: The ssh invocation can break if SDP_PATH contains single quotes and
using -f backgrounds ssh preventing confirmation of startup; update the ssh call
that uses MONITOR_USER, MONITOR_HOST, MONITOR_PLAYER and SDP_PATH so the remote
argument is safely escaped (e.g., use printf '%q' or pass SDP_PATH as a
quoted/escaped remote argument or use a here-doc / bash -s to avoid raw
single-quote interpolation) and remove or avoid -f (or add explicit post-ssh
check) so the script can detect/confirm the player started successfully
(implement a simple readiness check after ssh when using MONITOR_PLAYER).
- Around line 61-72: The script hardcodes the SSH alias "remote:" when copying
${SDP_PATH} which prevents user configuration and uses an interactive read that
will hang in CI; add a new CLI option/variable (e.g., --remote-host setting
REMOTE_HOST, defaulting to MONITOR_HOST or "remote") and replace the hardcoded
"remote:" in the scp invocation with "${REMOTE_HOST}:" so scp
"remote:${SDP_PATH}" becomes scp "${REMOTE_HOST}:${SDP_PATH}"; also remove the
blocking interactive prompt in the scp failure branch—instead honor a
non-interactive mode or a --force/--continue flag (or exit with a clear error)
and detect interactivity with a tty check (e.g., [[ -t 0 ]]) to decide behavior,
updating references to USE_REMOTE and SDP_PATH accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fefedb45-c3d1-4da8-ad5d-9bb118b8f519
📒 Files selected for processing (20)
.github/workflows/update-image.yml.gitmodules.script/attach-remote.script/build-rmcs.script/clean-rmcs.script/complete/_build-rmcs.script/complete/_play-autoaim.script/complete/_set-remote.script/foxglove.script/host/rmcs.script/launch-rmcs.script/play-autoaim.script/sync-remote.script/template/env_setup.bash.script/template/env_setup.zshDockerfileREADME.mddocker-compose.ymlrmcs_ws/src/rmcs_executor/include/rmcs_executor/component.hpprmcs_ws/src/rmcs_executor/src/main.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- README.md
- .gitmodules
- .script/complete/_set-remote
- .github/workflows/update-image.yml
- docker-compose.yml
- .script/host/rmcs
- .script/clean-rmcs
- .script/foxglove
- .script/attach-remote
- .script/complete/_play-autoaim
Comment on lines
+21
to
+22
| _comp_line="colcon build${BUFFER#build-rmcs}" | ||
| _comp_point=$((CURSOR + 2)) |
There was a problem hiding this comment.
COMP_POINT 计算存在边界情况问题。
当光标位于 build-rmcs 命令名称内部时（例如用户输入 build-rm 后按 Tab），CURSOR + 2 的计算可能不准确。字符串替换将 "build-rmcs"（10字符）替换为 "colcon build"（12字符），偏移量 +2 仅在光标位于命令名之后时正确。
🛠️ 建议修复
- _comp_line="colcon build${BUFFER#build-rmcs}"
- _comp_point=$((CURSOR + 2))
+ local _cmd_len=${#${BUFFER%%[[:space:]]*}}
+ _comp_line="colcon build${BUFFER#build-rmcs}"
+ if (( CURSOR <= _cmd_len )); then
+ # Cursor within command name, map proportionally
+ _comp_point=$((CURSOR * 12 / 10))
+ else
+ _comp_point=$((CURSOR + 2))
+ fi📝 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.
Suggested change
| _comp_line="colcon build${BUFFER#build-rmcs}" | |
| _comp_point=$((CURSOR + 2)) | |
| local _cmd_len=${#${BUFFER%%[[:space:]]*}} | |
| _comp_line="colcon build${BUFFER#build-rmcs}" | |
| if (( CURSOR <= _cmd_len )); then | |
| # Cursor within command name, map proportionally | |
| _comp_point=$((CURSOR * 12 / 10)) | |
| else | |
| _comp_point=$((CURSOR + 2)) | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.script/complete/_build-rmcs around lines 21 - 22, The COMP_POINT
calculation is wrong when the cursor sits inside the token "build-rmcs"; update
the logic around _comp_line and _comp_point to compute the true offset using the
length difference between the replacement and original token and the cursor's
position relative to the token. Specifically, locate the start index of
"build-rmcs" in BUFFER, compute delta = ${`#new_token`} - ${`#old_token`}
(new_token="colcon build", old_token="build-rmcs"), and if CURSOR is after the
token start adjust _comp_point = CURSOR + delta (or keep CURSOR unchanged if
before the token); reference variables/functions: _comp_line, _comp_point,
BUFFER, CURSOR and the token "build-rmcs" when implementing this fix.
207128a to
473dde4
Compare
Closed
This was referenced Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a history-cleanup rebase from refactor/simplify-main-base.
The previous commit series was too fragmented and noisy, which made review and traceability difficult.
The rebase is intended to produce a cleaner, coherent history for safer long-term maintenance.
Note: This also includes fixes for several pre-existing issues on main.
拉取请求摘要
概述
这是一个历史清理的rebase操作，将
refactor/simplify分支的提交整理到refactor/simplify-main-base分支，以改善提交历史的清晰度和可追溯性。此外，PR还包含了对多个代码库组件的改进和功能扩展。主要变更
CI/CD与容器化
.github/workflows/update-image.yml）：自动构建并推送RMCS Docker镜像到Docker Hub，在Dockerfile变更时触发，支持两个构建目标（rmcs-develop和rmcs-runtime）/opt/cmake构建与开发脚本
.script/build-rmcs、.script/clean-rmcs等脚本改用可配置的RMCS_PATH环境变量替代硬编码路径，默认值为/workspaces/RMCSbuild-rmcs脚本LLVM构建支持，包括工具链检查和编译器配置.script/play-autoaim：媒体播放远程SDP文件，支持从远程设备或本地拉取.script/foxglove：启动Foxglove ROS 2桥接.script/host/rmcs：Docker Compose环境编排脚本，支持zsh进入和Neovide编辑器启动_build-rmcs、_play-autoaim、_set-remote）以改善开发体验环境配置
.script/template/env_setup.bash/zsh）：ROS_LOCALHOST_ONLY，新增ROS_AUTOMATIC_DISCOVERY_RANGE=LOCALHOST和RCUTILS_COLORIZED_OUTPUT=1RMCS_PATH变量实现动态路径解析代码质量改进
rmcs_executor/component.hpp）：register_output和create_partner_component模板函数添加std::constructible_from概念约束，提升类型安全性= default显式默认化size_t规范化为std::size_trmcs_executor/main.cpp）：改进缺失"components"参数时的错误提示信息依赖管理
文档
文件变更统计
涉及约95个文件，包括工作流、脚本、Docker配置、C++头文件和源文件等，总体代码行数增加超过250行。