feat(ci): unify local and GitHub CI execution paths#123
Merged
Conversation
Review Summary by QodoUnify local and GitHub CI execution with lane-based entrypoint and container support
WalkthroughsDescription• Unified CI execution with single ci.sh entrypoint supporting format, build-test, and static-analysis lanes • Added container runner support for improved CI parity across local and GitHub Actions environments • Implemented stage-based timing metrics and hierarchical summary reporting for CI visibility • Refactored GitHub Actions workflows to call unified Pixi-backed commands instead of inline steps • Enhanced headless tests with shader parameter support for improved test coverage Diagramflowchart LR
A["CI Entrypoint<br/>ci.sh"] --> B["Lane Selection"]
B --> C["format"]
B --> D["build-test"]
B --> E["static-analysis"]
C --> F["Host Runner"]
D --> F
E --> F
C --> G["Container Runner"]
D --> G
E --> G
F --> H["Stage Metrics<br/>& Summary"]
G --> H
I["GitHub Actions<br/>Workflows"] --> A
J["Local Development"] --> A
File Changes1. scripts/task/ci.sh
|
Code Review by Qodo
1. clang-format version mismatch
|
📝 WalkthroughWalkthroughConsolidates CI steps into lane-based invocations: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as "Developer (CLI)"
participant CIsh as "scripts/task/ci.sh"
participant Engine as "Container Engine\n(Docker/Podman)"
participant Image as "CI Container Image"
participant Host as "Host FS / Cache"
participant Tests as "Build & Test Tools"
Dev->>CIsh: invoke with --lane / --runner / --cache-mode
CIsh->>CIsh: validate args, select lane(s)
alt runner = container
CIsh->>Engine: resolve engine (docker/podman)
Engine->>CIsh: engine available
CIsh->>Image: build or pull ci/local-runner image
CIsh->>Engine: run container with volumes (Host)
Engine->>Image: start container
Image->>CIsh: execute lane commands inside container
Image->>Host: read/write caches & stage metrics (mounted volumes)
Image->>Tests: run format/build/test/static-analysis
Tests-->>Image: results, artifacts
Image->>Engine: exit
else runner = host
CIsh->>Tests: run lane commands on host (clang-format, build, semgrep...)
Tests-->>CIsh: results, artifacts
end
CIsh->>CIsh: collect stage timings, write metrics TSV (optional)
CIsh->>Dev: print summary and exit status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/task/ci.sh (2)
435-439: The semgrep version check may be redundant.The
semgrep --versionstage will fail if semgrep isn't available, but so will the subsequentsemgrep scan. Consider whether this explicit check adds value or if it's just a timing artifact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/task/ci.sh` around lines 435 - 439, The semgrep version check is redundant because the later semgrep scan will already fail if semgrep is missing; remove the explicit version-check block that calls run_timed_stage with "semgrep --version", the status capture and the conditional return (references: run_timed_stage, semgrep --version, status variable) so the pipeline relies on the actual semgrep scan step to surface absence/errors and avoid an extra timing artifact.
296-319: Quote the environment variable value to prevent potential word splitting.On line 308, the variable
$metrics_container_pathshould be quoted. While this specific path is unlikely to contain spaces, consistent quoting prevents subtle bugs.🔧 Proposed fix
run_args+=( --user "$(id -u):$(id -g)" -e CI=true -e GITHUB_ACTIONS=true -e HOME=/tmp/goggles-home -e PIXI_HOME=/tmp/goggles-pixi-home -e PIXI_CACHE_DIR=/tmp/goggles-pixi-cache -e CCACHE_DIR=/tmp/goggles-ccache -e TMPDIR=/tmp/goggles-tmp -e XDG_RUNTIME_DIR=/tmp/goggles-runtime -e GOGGLES_CI_IN_CONTAINER=1 -e GOGGLES_CI_SUPPRESS_SUMMARY=1 - -e GOGGLES_CI_STAGE_METRICS_FILE=$metrics_container_path + -e "GOGGLES_CI_STAGE_METRICS_FILE=$metrics_container_path" -v "$REPO_ROOT:/work"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/task/ci.sh` around lines 296 - 319, The -e environment assignment using the variable metrics_container_path in the run_args array is unquoted and can cause word splitting; update the run_args entry that sets GOGGLES_CI_STAGE_METRICS_FILE to quote the variable (use "$metrics_container_path") so the run_args array contains a single safe token; locate the run_args array construction and modify the line with -e GOGGLES_CI_STAGE_METRICS_FILE to use the quoted variable.ci/local-runner/Dockerfile (1)
3-3: Consider adding a non-root user after package installation.While
ci.shoverrides the user at runtime via--user "$(id -u):$(id -g)", setting a non-root default in the Dockerfile provides defense-in-depth and silences security scanners. This is flagged by Trivy (DS-0002).🛡️ Proposed fix
RUN apt-get update -o Acquire::Retries=3 \ && apt-get install -y --no-install-recommends \ bash \ binutils \ ca-certificates \ git \ libdrm2 \ libegl1 \ libgbm1 \ libgles2 \ libvulkan1 \ mesa-vulkan-drivers \ xwayland \ && rm -rf /var/lib/apt/lists/* + +RUN useradd -m -s /bin/bash ciuser +USER ciuser WORKDIR /work🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/local-runner/Dockerfile` at line 3, The Dockerfile currently sets USER root; add a non-root user and switch to it after installing packages to satisfy scanners and provide defense-in-depth: create a dedicated user/group (e.g., via addgroup/adduser or useradd), set appropriate ownership on any runtime directories/artifacts the container needs, and replace the final USER root line with USER <nonroot-username> (ensure any build-stage operations that need root happen before creating/chowning and switching). Update any references to runtime paths if needed so the new user can access them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ci/local-runner/Dockerfile`:
- Line 3: The Dockerfile currently sets USER root; add a non-root user and
switch to it after installing packages to satisfy scanners and provide
defense-in-depth: create a dedicated user/group (e.g., via addgroup/adduser or
useradd), set appropriate ownership on any runtime directories/artifacts the
container needs, and replace the final USER root line with USER
<nonroot-username> (ensure any build-stage operations that need root happen
before creating/chowning and switching). Update any references to runtime paths
if needed so the new user can access them.
In `@scripts/task/ci.sh`:
- Around line 435-439: The semgrep version check is redundant because the later
semgrep scan will already fail if semgrep is missing; remove the explicit
version-check block that calls run_timed_stage with "semgrep --version", the
status capture and the conditional return (references: run_timed_stage, semgrep
--version, status variable) so the pipeline relies on the actual semgrep scan
step to surface absence/errors and avoid an extra timing artifact.
- Around line 296-319: The -e environment assignment using the variable
metrics_container_path in the run_args array is unquoted and can cause word
splitting; update the run_args entry that sets GOGGLES_CI_STAGE_METRICS_FILE to
quote the variable (use "$metrics_container_path") so the run_args array
contains a single safe token; locate the run_args array construction and modify
the line with -e GOGGLES_CI_STAGE_METRICS_FILE to use the quoted variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b221ec52-dd9b-4675-b65a-57a7e8dc22fe
📒 Files selected for processing (6)
.github/workflows/ci.ymlci/local-runner/Dockerfilepixi.tomlscripts/task/ci.shscripts/task/help.shtests/CMakeLists.txt
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
179
to
+185
| clang-tools = "21.*" | ||
| taplo = "==0.9.3" | ||
|
|
||
| [feature.lint.tasks.ci-format] | ||
| description = "Run the local CI format-check lane" | ||
| cmd = "bash scripts/task/ci.sh --lane format" | ||
|
|
There was a problem hiding this comment.
1. Clang-format version mismatch 🐞 Bug ✓ Correctness
GitHub format-check runs ci-format in the lint environment, but the lock resolves clang-format/tools 21.1.7 there while the default environment used by pixi run format resolves 21.1.6. This can cause CI to fail after developers run the local formatter (or vice versa) because the tool versions differ.
Agent Prompt
### Issue description
CI format-check (`pixi run -e lint ci-format`) and local formatting (`pixi run format`) currently run different clang-format/tools versions (21.1.7 vs 21.1.6 per pixi.lock), risking inconsistent formatting results.
### Issue Context
- Default environment uses clang-tools 21.1.6.
- Lint environment allows 21.* and currently resolves to 21.1.7.
- Workflow uses lint env for format-check; developers are instructed to run `pixi run format` (default env).
### Fix Focus Areas
- pixi.toml[90-93]
- pixi.toml[177-185]
- pixi.lock[1-266]
- .github/workflows/ci.yml[20-40]
### Suggested change
- Pin `feature.lint.dependencies.clang-tools` to the same exact version as default (e.g., `==21.1.6`) **or** update default to match lint and re-lock.
- Regenerate `pixi.lock` so both environments resolve the same clang-format/tools artifacts.
- Optional: run `format` using the lint environment (or re-home `ci-format` to default) so both paths share the same toolchain.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
- Establish one lane-based CI entrypoint for format, test, and analysis flows - Make GitHub Actions call the same Pixi-backed commands used locally - Add container execution support to improve CI parity across environments - Fold shader-backed headless tests into the shared validation path
K1ngst0m
force-pushed
the
dev/unifiy-ci-local
branch
from
f7050bb to
952bbcb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci/local-runner/Dockerfile (1)
3-3: Consider adding a non-root user for defense-in-depth.The static analysis tool flagged that the last USER is
root. While theci.shscript correctly overrides this at runtime with--user "$(id -u):$(id -g)"(line 297), adding a non-root default user provides defense-in-depth if the container is ever invoked without the user override.♻️ Suggested improvement
&& rm -rf /var/lib/apt/lists/* +RUN useradd -m -s /bin/bash -u 1000 ciuser +USER ciuser + WORKDIR /work🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/local-runner/Dockerfile` at line 3, Replace the final USER root with a non-root default by adding a dedicated user and group in the Dockerfile (e.g., create user "ci" with a non-zero UID/GID, chown relevant workspace and tool dirs to that user) and set USER to that non-root account instead of root; keep the existing ci.sh runtime override (which uses --user) intact but ensure the Dockerfile contains the createuser + chown steps and the final USER ci entry so the container defaults to non-root for defense-in-depth while still supporting the script’s runtime user override.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/task/ci.sh`:
- Line 308: The environment variable assignment for
GOGGLES_CI_STAGE_METRICS_FILE is unquoted which can cause word-splitting; update
the Docker -e argument to quote the value by changing the assignment that uses
$metrics_container_path so it becomes a quoted expansion (i.e., use
"$metrics_container_path") wherever GOGGLES_CI_STAGE_METRICS_FILE is set in the
script to ensure safe handling of spaces or special characters.
---
Nitpick comments:
In `@ci/local-runner/Dockerfile`:
- Line 3: Replace the final USER root with a non-root default by adding a
dedicated user and group in the Dockerfile (e.g., create user "ci" with a
non-zero UID/GID, chown relevant workspace and tool dirs to that user) and set
USER to that non-root account instead of root; keep the existing ci.sh runtime
override (which uses --user) intact but ensure the Dockerfile contains the
createuser + chown steps and the final USER ci entry so the container defaults
to non-root for defense-in-depth while still supporting the script’s runtime
user override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 650b0120-0ac9-4a3b-b9d5-eadf9384fafb
📒 Files selected for processing (6)
.github/workflows/ci.ymlci/local-runner/Dockerfilepixi.tomlscripts/task/ci.shscripts/task/help.shtests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/task/help.sh
- tests/CMakeLists.txt
- .github/workflows/ci.yml
| -e XDG_RUNTIME_DIR=/tmp/goggles-runtime | ||
| -e GOGGLES_CI_IN_CONTAINER=1 | ||
| -e GOGGLES_CI_SUPPRESS_SUMMARY=1 | ||
| -e GOGGLES_CI_STAGE_METRICS_FILE=$metrics_container_path |
There was a problem hiding this comment.
Quote the environment variable assignment to prevent word splitting.
The static analysis tool flagged this line. While $metrics_container_path is unlikely to contain spaces in practice, quoting the value is defensive and consistent with shell best practices.
🔧 Proposed fix
- -e GOGGLES_CI_STAGE_METRICS_FILE=$metrics_container_path
+ -e "GOGGLES_CI_STAGE_METRICS_FILE=$metrics_container_path"📝 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
| -e GOGGLES_CI_STAGE_METRICS_FILE=$metrics_container_path | |
| -e "GOGGLES_CI_STAGE_METRICS_FILE=$metrics_container_path" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 308-308: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/task/ci.sh` at line 308, The environment variable assignment for
GOGGLES_CI_STAGE_METRICS_FILE is unquoted which can cause word-splitting; update
the Docker -e argument to quote the value by changing the assignment that uses
$metrics_container_path so it becomes a quoted expansion (i.e., use
"$metrics_container_path") wherever GOGGLES_CI_STAGE_METRICS_FILE is set in the
script to ensure safe handling of spaces or special characters.
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
1 participant
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.
Summary by CodeRabbit