ci: run cppcheck in lint container, document CI-faithful lint one-liner - #7533
ci: run cppcheck in lint container, document CI-faithful lint one-liner#7533PastaPastaPasta wants to merge 2 commits into
Conversation
WalkthroughAdded matching “Running Linters Exactly Like CI” sections to Updated Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit f479e51) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf43d43bb7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ``` | ||
|
|
||
| ```bash | ||
| G="$(git rev-parse --path-format=absolute --git-common-dir)"; docker run --rm -v "$PWD":"$PWD" -v "$G":"$G" -w "$PWD" -e BUILD_TARGET=linux64 -e CHECK_DOC=1 -e PULL_REQUEST=true -e COMMIT_RANGE="$(git merge-base develop HEAD)..HEAD" dash-linter bash -c 'git config --global --add safe.directory "*" && ./ci/dash/lint.sh' |
There was a problem hiding this comment.
When the working tree contains non-conflicting uncommitted changes and COMMIT_RANGE includes a scripted-diff: commit, this command invokes test/lint/commit-script-check.sh, which checks out the commit's parent and runs git reset --hard HEAD; that can permanently discard the user's changes from the writable bind mount. Require a clean worktree or add a preflight guard before advertising this as a general local lint command.
Useful? React with 👍 / 👎.
| # One-time setup; rebuild only when ci/lint/ changes. | ||
| docker build -t dash-linter ci/lint |
There was a problem hiding this comment.
For C++ changes checked with this documented container, ci/lint/Dockerfile installs only the tools from ci/lint/04_install.sh and omits cppcheck, so lint-cppcheck-dash.py exits successfully after printing the skip mentioned below. The actual lint job uses contrib/containers/ci/ci-slim.Dockerfile (.github/workflows/build.yml:190-196), which builds cppcheck 2.17.1, meaning this command can pass locally while the supposedly reproduced CI job fails.
Useful? React with 👍 / 👎.
| ``` | ||
|
|
||
| ```bash | ||
| G="$(git rev-parse --path-format=absolute --git-common-dir)"; docker run --rm -v "$PWD":"$PWD" -v "$G":"$G" -w "$PWD" -e BUILD_TARGET=linux64 -e CHECK_DOC=1 -e PULL_REQUEST=true -e COMMIT_RANGE="$(git merge-base develop HEAD)..HEAD" dash-linter bash -c 'git config --global --add safe.directory "*" && ./ci/dash/lint.sh' |
There was a problem hiding this comment.
In a clone or worktree without a local develop branch, the host-side git merge-base develop HEAD fails before Docker starts; if that branch exists but is stale or divergent, the range also differs from CI. The inspected lint workflow fetches the target branch and computes its PR range from origin/develop (.github/workflows/lint.yml:31-49), so the documented command needs an equivalent fetch/ref strategy to reproduce that job reliably.
Useful? React with 👍 / 👎.
bf43d43 to
4de30bb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4de30bb3a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ```bash | ||
| # One-time setup; rebuild only when ci/lint/ changes. | ||
| docker build -t dash-linter ci/lint |
There was a problem hiding this comment.
On native arm64 hosts, this unqualified build selects an arm64 base image, but ci/lint/04_install.sh:44-47 unconditionally installs the linux.x86_64 ShellCheck binary. test/lint/lint-shell.py:20-25 only treats a missing executable as skippable, so an incompatible binary can terminate the advertised lint run with an exec-format error. Either select linux/amd64 explicitly for the build/run or make the ShellCheck download follow the target architecture as ci-slim.Dockerfile does.
Useful? React with 👍 / 👎.
| ``` | ||
|
|
||
| ```bash | ||
| G="$(git rev-parse --path-format=absolute --git-common-dir)"; docker run --rm -v "$PWD":"$PWD" -v "$G":"$G" -w "$PWD" -e BUILD_TARGET=linux64 -e CHECK_DOC=1 -e PULL_REQUEST=true -e COMMIT_RANGE="$(git merge-base develop HEAD)..HEAD" dash-linter bash -c 'git config --global --add safe.directory "*" && ./ci/dash/lint.sh' |
There was a problem hiding this comment.
On native Linux Docker installations, this command runs as root because ci/lint/Dockerfile declares no USER, while test/lint/lint-python.py:18-20 writes its cache into the bind-mounted test/.mypy_cache. A successful lint can therefore leave root-owned cache directories that subsequent host-side lint or cleanup commands cannot update or remove without elevated privileges; pass through the host UID/GID with a writable HOME, or keep generated caches outside the source bind mount.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
AGENTS.md (1)
145-145: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope Git trust to the mounted worktree.
safe.directory "*"treats every repository in the container as safe. Scope the exception to the mounted worktree instead.
AGENTS.md#L145-L145: replacesafe.directory "*"withsafe.directory "$PWD".CLAUDE.md#L145-L145: replacesafe.directory "*"withsafe.directory "$PWD".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` at line 145, Scope Git’s safe-directory exception to the mounted worktree by replacing the wildcard with "$PWD" in the docker lint command; apply this change in AGENTS.md at lines 145-145 and CLAUDE.md at lines 145-145.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@AGENTS.md`:
- Line 145: Scope Git’s safe-directory exception to the mounted worktree by
replacing the wildcard with "$PWD" in the docker lint command; apply this change
in AGENTS.md at lines 145-145 and CLAUDE.md at lines 145-145.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b205ea4a-712a-4e42-8c0e-9d3f2aae14b9
📒 Files selected for processing (2)
AGENTS.mdCLAUDE.md
4de30bb to
97ad6fb
Compare
PastaPastaPasta
changed the title
|
Addressed all three Codex findings:
🤖 Posted autonomously by Claude on behalf of pasta. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci/lint/Dockerfile (1)
21-26: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerify the Cppcheck archive before building it.
Both
ci/lint/Dockerfileandcontrib/containers/ci/ci-slim.Dockerfiledownloadcppcheck/${CPPCHECK_VERSION}.tar.gzinto a build container and run CMake without integrity or signature validation. Pin the archive to the intended commit and check the digest or signature beforetar, so a moved tag or compromised archive cannot execute build-time code and place a modifiedcppcheckbinary in the lint image.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/lint/Dockerfile` around lines 21 - 26, Update the Cppcheck download-and-build flow in the Dockerfile, and apply the same change to the corresponding ci-slim.Dockerfile flow: pin the archive to the intended immutable commit and verify its expected digest or signature before extracting it with tar or running CMake. Keep the existing build steps unchanged after validation succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ci/lint/Dockerfile`:
- Around line 21-26: Update the Cppcheck download-and-build flow in the
Dockerfile, and apply the same change to the corresponding ci-slim.Dockerfile
flow: pin the archive to the intended immutable commit and verify its expected
digest or signature before extracting it with tar or running CMake. Keep the
existing build steps unchanged after validation succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6b472b8-3f00-4890-991f-c04a6b5209de
📒 Files selected for processing (3)
AGENTS.mdCLAUDE.mdci/lint/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (2)
- AGENTS.md
- CLAUDE.md
|
Prepared the follow-up for the new Codex/CodeRabbit findings, but the active Final two-commit branch, rebased onto current
Validation passed: docs remain identical, To update this PR while preserving the intended two-commit history, the branch owner can fetch |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The cppcheck builder correctly mirrors the existing CI builder and enables a lint that previously self-skipped in the local image. The documented workflow still needs adjustments for ARM hosts, bind-mount ownership, and placement of its destructive-operation warning, and the commits should be reordered so the documentation is accurate at every point in history. No blocking correctness defects were found.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (failed),claude-sonnet-5— general (completed)
🟡 4 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `AGENTS.md`:
- [SUGGESTION] AGENTS.md:141: Force the CI architecture for the lint image
This build selects the host architecture, while `ci/lint/04_install.sh` unconditionally downloads the x86_64 ShellCheck executable. On a native arm64 host without x86 binary emulation, the image builds successfully but `lint-shell.py` later fails with an exec-format error. Since the CI lint job runs on amd64, pin this workflow to `linux/amd64`, or make the installer select ShellCheck using the target architecture as `ci-slim.Dockerfile` already does. Apply the same correction to the mirrored section in `CLAUDE.md`.
- [SUGGESTION] AGENTS.md:145: Run the bind-mounted lint job as the host user
The lint image has no `USER` directive and this invocation supplies no `--user`, so it writes through the native Linux bind mount as UID 0. The lint suite creates `test/.mypy_cache` and `ci-cache-linux64/cppcheck`, leaving root-owned cache contents that subsequent host-side runs cannot update or remove recursively without elevated privileges. CI also runs its container as root, but its checkout is disposable rather than a contributor's live working tree. Run the local container with the host UID/GID and a writable `HOME`, and apply the same correction to `CLAUDE.md`.
- [SUGGESTION] AGENTS.md:139-155: Put the destructive-operation warning before the copy-pasteable command
With `PULL_REQUEST=true`, `ci/dash/lint.sh` invokes `commit-script-check.sh`. If the selected range contains a `scripted-diff:` commit, that script checks out the parent and executes `git reset --hard` against the live bind-mounted working tree, which can erase dirty tracked changes that survive the checkout. The documentation discloses this only after the command users and agents are expected to copy. Move the commit-or-stash warning above the command or add a clean-working-tree preflight to the command itself. Make the same change in `CLAUDE.md`.
In `<commit:e37b1d5>`:
- [SUGGESTION] <commit:e37b1d5>:1: Place the cppcheck implementation before documentation that depends on it
Commit `e37b1d5` states that the lint image builds cppcheck and reproduces CI with pinned tools, but the Dockerfile does not acquire cppcheck until its child commit `97ad6fb`. Checking out or bisecting to the documentation commit therefore leaves an inaccurate command whose cppcheck lint still self-skips. Reorder the cppcheck implementation before the documentation commit, or squash the two commits so every retained commit is internally accurate.
|
|
||
| ```bash | ||
| # One-time setup; rebuild only when ci/lint/ changes. | ||
| docker build -t dash-linter ci/lint |
There was a problem hiding this comment.
🟡 Suggestion: Force the CI architecture for the lint image
This build selects the host architecture, while ci/lint/04_install.sh unconditionally downloads the x86_64 ShellCheck executable. On a native arm64 host without x86 binary emulation, the image builds successfully but lint-shell.py later fails with an exec-format error. Since the CI lint job runs on amd64, pin this workflow to linux/amd64, or make the installer select ShellCheck using the target architecture as ci-slim.Dockerfile already does. Apply the same correction to the mirrored section in CLAUDE.md.
| docker build -t dash-linter ci/lint | |
| docker build --platform=linux/amd64 -t dash-linter ci/lint |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Force the CI architecture for the lint image no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| ``` | ||
|
|
||
| ```bash | ||
| G="$(git rev-parse --path-format=absolute --git-common-dir)"; docker run --rm -v "$PWD":"$PWD" -v "$G":"$G" -w "$PWD" -e BUILD_TARGET=linux64 -e CHECK_DOC=1 -e PULL_REQUEST=true -e COMMIT_RANGE="$(git merge-base develop HEAD)..HEAD" dash-linter bash -c 'git config --global --add safe.directory "*" && ./ci/dash/lint.sh' |
There was a problem hiding this comment.
🟡 Suggestion: Run the bind-mounted lint job as the host user
The lint image has no USER directive and this invocation supplies no --user, so it writes through the native Linux bind mount as UID 0. The lint suite creates test/.mypy_cache and ci-cache-linux64/cppcheck, leaving root-owned cache contents that subsequent host-side runs cannot update or remove recursively without elevated privileges. CI also runs its container as root, but its checkout is disposable rather than a contributor's live working tree. Run the local container with the host UID/GID and a writable HOME, and apply the same correction to CLAUDE.md.
| G="$(git rev-parse --path-format=absolute --git-common-dir)"; docker run --rm -v "$PWD":"$PWD" -v "$G":"$G" -w "$PWD" -e BUILD_TARGET=linux64 -e CHECK_DOC=1 -e PULL_REQUEST=true -e COMMIT_RANGE="$(git merge-base develop HEAD)..HEAD" dash-linter bash -c 'git config --global --add safe.directory "*" && ./ci/dash/lint.sh' | |
| G="$(git rev-parse --path-format=absolute --git-common-dir)"; docker run --rm --user "$(id -u):$(id -g)" -e HOME=/tmp -v "$PWD":"$PWD" -v "$G":"$G" -w "$PWD" -e BUILD_TARGET=linux64 -e CHECK_DOC=1 -e PULL_REQUEST=true -e COMMIT_RANGE="$(git merge-base develop HEAD)..HEAD" dash-linter bash -c 'git config --global --add safe.directory "*" && ./ci/dash/lint.sh' |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Run the bind-mounted lint job as the host user no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| ```bash | ||
| # One-time setup; rebuild only when ci/lint/ changes. | ||
| docker build -t dash-linter ci/lint | ||
| ``` | ||
|
|
||
| ```bash | ||
| G="$(git rev-parse --path-format=absolute --git-common-dir)"; docker run --rm -v "$PWD":"$PWD" -v "$G":"$G" -w "$PWD" -e BUILD_TARGET=linux64 -e CHECK_DOC=1 -e PULL_REQUEST=true -e COMMIT_RANGE="$(git merge-base develop HEAD)..HEAD" dash-linter bash -c 'git config --global --add safe.directory "*" && ./ci/dash/lint.sh' | ||
| ``` | ||
|
|
||
| Notes: | ||
|
|
||
| - The dual mount (`$PWD` plus the git common dir) makes this work from git | ||
| worktrees as well as normal checkouts; run it from the repo/worktree root. | ||
| - Commit or stash before running. With `PULL_REQUEST=true` the range is fed to | ||
| `test/lint/commit-script-check.sh`, which checks out commits and runs | ||
| `git reset --hard` while replaying `scripted-diff:` commits; through the | ||
| writable bind mount this can discard uncommitted changes. |
There was a problem hiding this comment.
🟡 Suggestion: Put the destructive-operation warning before the copy-pasteable command
With PULL_REQUEST=true, ci/dash/lint.sh invokes commit-script-check.sh. If the selected range contains a scripted-diff: commit, that script checks out the parent and executes git reset --hard against the live bind-mounted working tree, which can erase dirty tracked changes that survive the checkout. The documentation discloses this only after the command users and agents are expected to copy. Move the commit-or-stash warning above the command or add a clean-working-tree preflight to the command itself. Make the same change in CLAUDE.md.
source: ['claude']
97ad6fb to
04d86ab
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Pushed
Since the claw had no Docker daemon, the scoped 🤖 Posted autonomously by Claude on behalf of pasta. |
04d86ab to
0523bda
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Restacked on #7535 ( Changes in the restack:
Re-validated end-to-end on the stacked branch from a git worktree (macOS, Docker 29.4.0): digest check passes during build, The base stays 🤖 Posted autonomously by Claude on behalf of pasta. |
|
This pull request has conflicts, please rebase. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
At head 0523bda, the two PR-owned commits build cppcheck 2.21.0 (SHA-256 verified) in the ci/lint image mirroring ci-slim.Dockerfile, and document a CI-faithful docker lint one-liner in byte-identical AGENTS.md/CLAUDE.md sections. All four prior findings (arm64 platform pin, host-UID/GID execution, warning placement, commit ordering) are independently verified fixed in the current source. No new in-scope issues were found; the Codex commit-history suggestions target the inherited #7535 stack, which the PR description explicitly excludes from this PR's scope. Source: reviewer backends gpt-5.6-sol (codex, general + dash-core-commit-history) and claude-sonnet-5 (general + dash-core-commit-history); final verifier backend claude-sonnet-5.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:e37b1d5>`:
- [SUGGESTION] <commit:e37b1d5>:1: Place the cppcheck implementation before documentation that depends on it
Commit `e37b1d5` states that the lint image builds cppcheck and reproduces CI with pinned tools, but the Dockerfile does not acquire cppcheck until its child commit `97ad6fb`. Checking out or bisecting to the documentation commit therefore leaves an inaccurate command whose cppcheck lint still self-skips. Reorder the cppcheck implementation before the documentation commit, or squash the two commits so every retained commit is internally accurate.
Running test/lint scripts on the host uses local tool versions and the default ci/lint container entrypoint diverges from CI (merge-base against master, extra check-doc/subtree checks). Document a single docker run invocation that reproduces ci/dash/lint.sh with CI-pinned tools and works from git worktrees.
The CI lint job runs in the ci-slim container, which includes cppcheck 2.17.1, while the local ci/lint image had none, so lint-cppcheck-dash.py self-skipped locally and could pass while CI failed. Build the same cppcheck version in a builder stage, mirroring ci-slim.Dockerfile.
0523bda to
f479e51
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f479e51f74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Builder for cppcheck; keep the version in sync with | ||
| # contrib/containers/ci/ci-slim.Dockerfile so results match the CI lint job. | ||
| FROM debian:bookworm-slim AS cppcheck-builder | ||
| ARG CPPCHECK_VERSION=2.17.1 |
There was a problem hiding this comment.
With the current lint workload, cppcheck 2.17.1 asserts under --check-level=exhaustive; test/lint/lint-cppcheck-dash.py:153-163 now treats that nonzero exit as a failure. CI instead receives 2.21.0 from contrib/containers/ci/ci-slim.Dockerfile:3 via .github/workflows/build.yml:190-196, so the documented local command fails rather than reproducing CI. Fresh evidence beyond the earlier missing-cppcheck report is that this revision now installs cppcheck but pins the known-broken version.
AGENTS.md reference: AGENTS.md:L168-L170
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head f479e51, the local lint image pins cppcheck 2.17.1 while CI pins 2.21.0, so the advertised CI-faithful workflow does not reproduce the current CI lint and can fail under the exhaustive cppcheck workload. Four previously verified documentation and commit-order findings have regressed, and the newly downloaded cppcheck archive is not verified despite the PR description claiming digest verification.
Source: codex-general reviewer backend gpt-5.6-sol; codex-dash-core-commit-history reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `ci/lint/Dockerfile`:
- [BLOCKING] ci/lint/Dockerfile:11: Pin the local image to cppcheck 2.21.0
The local lint image installs cppcheck 2.17.1, but the base branch's `contrib/containers/ci/ci-slim.Dockerfile` installs 2.21.0. The current cppcheck lint passes `--check-level=exhaustive` and treats every nonzero analyzer exit as a lint failure; 2.17.1 is the version that exhibited a signal-6 failure under this workload and prompted the CI version bump. The documented local command can therefore fail while the real CI job succeeds, directly contradicting the PR's CI-parity goal. Update this pin and amend the implementation commit message, which also incorrectly states that CI uses 2.17.1.
- [SUGGESTION] ci/lint/Dockerfile:20-22: Verify the downloaded cppcheck archive
The new builder downloads a mutable version-tagged archive and extracts and compiles it without verifying its contents. A moved tag or compromised download source could therefore execute different build input for the same repository revision. Pin the cppcheck 2.21.0 archive SHA-256 (`f028ff75ca5372738f3737c8b3e8611426a6526b6aea2ef01301ab0f5902f044`) and run `sha256sum -c` before extraction. Apply the same digest verification to the mirrored `ci-slim` builder, as the PR description already claims both builders do.
In `<commit:dbe1097a9f7>`:
- [SUGGESTION] <commit:dbe1097a9f7>:1: Place the cppcheck implementation before documentation that depends on it
The retained history places documentation commit `dbe1097a9f7` before implementation commit `f479e51f746`. At the documentation commit, the guides already state that the lint image builds cppcheck and runs that lint locally, but `ci/lint/Dockerfile` still contains no cppcheck builder, so the lint self-skips. Reorder the corrected implementation before the documentation commit or squash them so every retained revision is internally accurate, matching the ordering promised by the PR description.
| # Builder for cppcheck; keep the version in sync with | ||
| # contrib/containers/ci/ci-slim.Dockerfile so results match the CI lint job. | ||
| FROM debian:bookworm-slim AS cppcheck-builder | ||
| ARG CPPCHECK_VERSION=2.17.1 |
There was a problem hiding this comment.
🔴 Blocking: Pin the local image to cppcheck 2.21.0
The local lint image installs cppcheck 2.17.1, but the base branch's contrib/containers/ci/ci-slim.Dockerfile installs 2.21.0. The current cppcheck lint passes --check-level=exhaustive and treats every nonzero analyzer exit as a lint failure; 2.17.1 is the version that exhibited a signal-6 failure under this workload and prompted the CI version bump. The documented local command can therefore fail while the real CI job succeeds, directly contradicting the PR's CI-parity goal. Update this pin and amend the implementation commit message, which also incorrectly states that CI uses 2.17.1.
| ARG CPPCHECK_VERSION=2.17.1 | |
| ARG CPPCHECK_VERSION=2.21.0 |
source: ['codex']
| echo "Downloading Cppcheck version: ${CPPCHECK_VERSION}"; \ | ||
| curl -fL "https://github.com/danmar/cppcheck/archive/${CPPCHECK_VERSION}.tar.gz" -o /tmp/cppcheck.tar.gz; \ | ||
| mkdir -p /src/cppcheck && tar -xzf /tmp/cppcheck.tar.gz -C /src/cppcheck --strip-components=1; \ |
There was a problem hiding this comment.
🟡 Suggestion: Verify the downloaded cppcheck archive
The new builder downloads a mutable version-tagged archive and extracts and compiles it without verifying its contents. A moved tag or compromised download source could therefore execute different build input for the same repository revision. Pin the cppcheck 2.21.0 archive SHA-256 (f028ff75ca5372738f3737c8b3e8611426a6526b6aea2ef01301ab0f5902f044) and run sha256sum -c before extraction. Apply the same digest verification to the mirrored ci-slim builder, as the PR description already claims both builders do.
source: ['codex']
2 participants
Issue being fixed or feature implemented
Local lint runs frequently disagree with CI, which slows down and confuses both human contributors and coding agents:
test/lint/*scripts on the host uses whatever tool versions happen to be installed locally, while CI pins specific versions of codespell/flake8/mypy/shellcheck etc.test/lint/README.md(baredocker run ... dash-linter) is not CI-faithful either: its default entrypoint merge-bases againstmasterinstead ofdevelop, and runscheck-doc.pyplus git-subtree checks that the CI lint job (ci/dash/lint.sh) has disabled.ci/lintimage had no cppcheck, while CI's lint job runs in theci-slimcontainer which builds cppcheck — solint-cppcheck-dash.pysilently self-skipped locally. This matters more now that fix(scripts): make lint-cppcheck-dash actually report warnings #7535 makes that lint actually report findings.The result is failures locally that CI never reports, and vice versa.
What was done?
Two commits on top of
develop(after #7535 merged;cifirst so the docs are accurate at every point in history):ci/lint/Dockerfilevia a builder stage mirroringcontrib/containers/ci/ci-slim.Dockerfile(same pinned version — 2.21.0 after fix(scripts): make lint-cppcheck-dash actually report warnings #7535 — binary + cfg installed the same way), so the local container runs the cppcheck lint just like CI instead of self-skipping. Both mirrored Dockerfiles now pin and verify the SHA-256 of the cppcheck source archive before building it.CLAUDE.mdandAGENTS.md, kept identical per the guides' own convention). Host linters remain the first choice when they behave; the section documents a one-timedocker build --platform linux/amd64 -t dash-linter ci/lintplus a singledocker runone-liner that reproduces the CI lint job:ci/dash/lint.shwith the same environment CI sets (BUILD_TARGET=linux64,CHECK_DOC=1,PULL_REQUEST=true,COMMIT_RANGEfrommerge-base develop). The command pins the CI architecture (--platform linux/amd64, sinceci/lint/04_install.shinstalls x86_64 ShellCheck unconditionally), runs the container as the host UID/GID withHOME=/tmpso bind-mounted caches don't end up root-owned, and scopes git'ssafe.directorytrust to the mounted worktree rather than*. The section also covers the dual mount needed for git worktrees, an up-front commit-or-stash warning placed before the copy-pasteable command (commit-script-check.shhard-resets while replayingscripted-diff:commits), keeping localdevelopcurrent, why the baretest/lint/README.mdflow should not be used to reproduce CI, and theci-cache-*scratch directory the run creates.How Has This Been Tested?
Built the image and ran the final documented one-liner end-to-end from a git worktree on macOS (Apple Silicon, Docker 29.4.0), on this branch: the archive digest check passes during build,
cppcheck --versionreports 2.21.0 inside the image, the run exits 0 with only the known non-fatal codespell output,lint-cppcheck-dash.py— with #7535's fixes active — analyzes the commit range's 311 source files and reports "no issues found" (populating its--cppcheck-build-dir) instead of printing the skip message, the resulting caches are owned by the host user, and the worktree-scopedsafe.directoryproduces no dubious-ownership errors. Independently re-derived the cppcheck 2.21.0 archive SHA-256 from a fresh download; it matches the pinned digest. VerifiedCLAUDE.mdandAGENTS.mdsections are byte-identical viadiff.Breaking Changes
None.
Checklist: