ci: S24.13 switch IWYU lanes to advisory#469
Conversation
Step-level continue-on-error on the Run include-what-you-use step in both analyze-iwyu and analyze-iwyu-freertos-plustcp. Findings still upload via the existing iwyu-report* artifacts; release prep is now responsible for sweeping them. docs/local-checks.md Tier B refreshed to drop IWYU as a required pre-push step. Story originally scoped as "replace IWYU with clang-tidy misc-include-cleaner" — survey revealed misc-include-cleaner's built-in stdlib mapping has ~75% false-positive rate on POSIX extension symbols (CLOCK_REALTIME, SOL_SOCKET, timeval, etc.) with no IWYU-style mapping file support to filter. Switching IWYU to advisory delivers the throughput win the original swap was meant to deliver, without trading one FP surface for another. Branch protection unchanged — step-level continue-on-error keeps the job reporting success on findings, so required-checks still satisfied. Closes #468 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughIWYU analysis is marked advisory in CI: both the standard and FreeRTOS+PlusTCP IWYU lanes now run with ChangesIWYU advisory mode
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@DEVLOG.md`:
- Around line 48-50: The DEVLOG entry that reads "Branch protection list
unchanged. Step-level `continue-on-error` makes the job report success on
findings..." conflicts with existing CI documentation that still treats IWYU as
a failing gate; either add a follow-up note/link in that DEVLOG entry pointing
to the CI documentation and explicitly stating IWYU is now advisory under
step-level continue-on-error, or update the CI docs to reflect the new IWYU
advisory behavior so both places are consistent (look for the exact DEVLOG
sentence and the IWYU/CI docs references to patch).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e26cdd7b-bc54-4d7c-a05f-94aa0fc328d2
📒 Files selected for processing (3)
.github/workflows/ci.ymlDEVLOG.mddocs/local-checks.md
| - Branch protection list unchanged. Step-level `continue-on-error` | ||
| makes the job report success on findings, so the required-check | ||
| contract is still satisfied without a UI edit. |
There was a problem hiding this comment.
Document consistency risk: branch-protection note may conflict with CI docs.
This says branch protection is unchanged with advisory step behavior, but docs/ci.md still describes IWYU as a failing gate. Please add a follow-up note/link here (or update the CI docs in the same change) to avoid conflicting guidance for contributors.
🤖 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 `@DEVLOG.md` around lines 48 - 50, The DEVLOG entry that reads "Branch
protection list unchanged. Step-level `continue-on-error` makes the job report
success on findings..." conflicts with existing CI documentation that still
treats IWYU as a failing gate; either add a follow-up note/link in that DEVLOG
entry pointing to the CI documentation and explicitly stating IWYU is now
advisory under step-level continue-on-error, or update the CI docs to reflect
the new IWYU advisory behavior so both places are consistent (look for the exact
DEVLOG sentence and the IWYU/CI docs references to patch).
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1344 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
Closes #468 (parent epic #254 E24).
S24.13 started out scoped as "replace IWYU with clang-tidy
misc-include-cleaner, diff-scoped" — the swap would have foldedinto the existing per-TU
analyze-tidy*lanes viaCMAKE_C_CLANG_TIDY, deleting the standalone IWYU lanes entirely.A finding survey halfway through revealed
misc-include-cleaner'sbuilt-in stdlib mapping has substantial gaps on POSIX extension
symbols (
CLOCK_REALTIMEnot mapped to<time.h>,SOL_SOCKET/SO_KEEPALIVEnot mapped to<sys/socket.h>, POSIXtimevalnotmapped to
<sys/time.h>). Roughly 75% of the findings onPlatform/Posix/were false positives. The check's only twoconfiguration knobs (
IgnoreHeaders,DeduplicateFindings) can'tfilter the FPs cleanly without also losing the signal — there's no
IWYU-style mapping file support.
The original motivation for the swap was IWYU's per-PR friction
(two-pass output, container drift, line-shift cascades into
suppression line numbers). The simpler move that actually delivers
that throughput win is to keep IWYU but stop it blocking PRs.
Change Description
Two
continue-on-error: trueadditions at theRunstep in eachIWYU lane:
analyze-iwyu(cpputest-clang image)analyze-iwyu-freertos-plustcp(cpputest-freertos image)Step-level continue-on-error keeps the JOB reporting success on
findings, so branch-protection's required-checks contract is still
satisfied — no GitHub UI edit needed.
docs/local-checks.mdTier B narrative refreshed: drops IWYU as arequired pre-push step (Tier B now just
clang-format -i+ MISRAline-drift fix), and gains an "(optional, advisory)" subsection
documenting the local invocation for anyone who wants to look ahead
of CI.
DEVLOG.mdentry documents the pivot, the misc-include-cleanerfinding survey, and what stays / what changed / what's left for
future work.
cppcheck stays required — S24.12 just audited its suppressions
and the FP rate is low. The advisory move is reserved for the
high-friction gate here.
Test Evidence
CI/tooling change only. No production-code or test-code
modifications.
Sanity check on the YAML edit:
workflow — the step is unconditional).
iwyu-report*artifacts (verified bythe existing
if: success() || failure()on the upload steps).continue-on-error: truecauses the step to mark failed but the job to mark success — that
is the GitHub Actions documented behavior, no separate proof
needed.
Areas Affected
.github/workflows/ci.yml— IWYU lane Run-step semantics only.docs/local-checks.md— Tier B refresh.DEVLOG.md— entry.No production source, no test source, no public-header changes. No
container-image bump. No CMake-preset change. The
iwyupreset andthe
ENABLE_IWYUplumbing inCMakeLists.txtstay — the local-runrecipe still works for anyone who wants advisory output ahead of CI.
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Documentation