ci: cache cppcheck analysis - #7572
Conversation
|
✅ Final review complete — no blockers (commit b858f35) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe lint workflow now creates and restores a Cppcheck cache. The cache key uses a Dockerfile hash and supports prefix-based fallback. The lint step uses the shared cache directory. The workflow reports the cache size after linting. It saves the cache only for pushes to the repository’s default branch. Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The cache plumbing and default-branch-only save policy match the PR's goal, but the compatibility prefix does not account for changes to the script that defines the cppcheck invocation. Cppcheck 2.21 does not include all relevant invocation settings—most notably the language standard—in its analyzer hash, so this can incorrectly reuse stale translation-unit results.
Source: reviewer backend models: codex general = gpt-5.6-sol; codex dash-core-commit-history = gpt-5.6-sol; final verifier backend model = gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
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/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
🤖 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 `.github/workflows/lint.yml`:
- [SUGGESTION] .github/workflows/lint.yml:42-44: Invalidate fallback caches when cppcheck configuration changes
Include `test/lint/lint-cppcheck-dash.py` in the cache configuration hash used here and in the save key at line 84. The restored prefix currently changes only with `ci-slim.Dockerfile`, so modifying the script's cppcheck command can restore analyzer data created with the previous configuration. In Cppcheck 2.21, `CppCheck::calculateHash()` includes the version, severity flags, defines, check level, and source tokens, but not `mSettings.standards` or the selected language. For example, changing `--std=c++20` can therefore leave an unchanged translation unit's hash unchanged and cause Cppcheck to replay the old result rather than reanalyze it. A successful default-branch run would then save those stale results under the new commit key. Use `hashFiles('contrib/containers/ci/ci-slim.Dockerfile', 'test/lint/lint-cppcheck-dash.py')` consistently in the restore key, restore prefix, and save key.
| key: cppcheck-${{ hashFiles('contrib/containers/ci/ci-slim.Dockerfile') }}-${{ github.sha }} | ||
| restore-keys: | | ||
| cppcheck-${{ hashFiles('contrib/containers/ci/ci-slim.Dockerfile') }}- |
There was a problem hiding this comment.
🟡 Suggestion: Invalidate fallback caches when cppcheck configuration changes
Include test/lint/lint-cppcheck-dash.py in the cache configuration hash used here and in the save key at line 84. The restored prefix currently changes only with ci-slim.Dockerfile, so modifying the script's cppcheck command can restore analyzer data created with the previous configuration. In Cppcheck 2.21, CppCheck::calculateHash() includes the version, severity flags, defines, check level, and source tokens, but not mSettings.standards or the selected language. For example, changing --std=c++20 can therefore leave an unchanged translation unit's hash unchanged and cause Cppcheck to replay the old result rather than reanalyze it. A successful default-branch run would then save those stale results under the new commit key. Use hashFiles('contrib/containers/ci/ci-slim.Dockerfile', 'test/lint/lint-cppcheck-dash.py') consistently in the restore key, restore prefix, and save key.
source: ['codex']
Issue being fixed or feature implemented
The Dash-specific cppcheck lint now performs a real exhaustive analysis, which takes roughly 20–30 minutes from a cold start. Although the linter already writes incremental analysis data through
--cppcheck-build-dir, the standalone lint workflow discarded that directory after every job.What was done?
CACHE_DIRsolint-cppcheck-dash.pyuses the restored directory.How Has This Been Tested?
.github/workflows/lint.ymlwith Ruby's YAML parser.test/lint/lint-whitespace.py.git diff --check.actionlint; the modified workflow introduced no new findings. Existing findings forallow-unsafe-pr-checkoutand existing shell snippets remain unchanged.The first default-branch run will populate the cache; subsequent runs can reuse unchanged cppcheck translation-unit results.
Breaking Changes
None.
Checklist:
This pull request was created by Codex.