ci(simulator): ratchet the compiler warnings instead of discarding them - #249
Open
tobymurray wants to merge 7 commits into
Open
ci(simulator): ratchet the compiler warnings instead of discarding them#249tobymurray wants to merge 7 commits into
tobymurray wants to merge 7 commits into
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesSimulator warning baseline enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SimulatorBuild
participant WarningBaseline
participant WarningArtifacts
participant WarningsJob
participant RatchetJob
participant CommittedBaseline
SimulatorBuild->>WarningBaseline: Extract compiler diagnostics
WarningBaseline->>WarningArtifacts: Upload per-project counts
WarningArtifacts->>WarningsJob: Download warning counts
WarningsJob->>CommittedBaseline: Read allowed counts
WarningsJob->>WarningsJob: Check warning regressions and coverage
WarningsJob->>RatchetJob: Permit ratcheting after successful full build
RatchetJob->>CommittedBaseline: Lower and commit baseline
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 4
🧹 Nitpick comments (2)
.github/workflows/linux-simulator.yml (2)
286-286: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePass the discover outputs through
envinstead of expanding them into the script.
needs.discover.outputs.countandneeds.discover.outputs.totalexpand directly into the shell body. The values come fromjq lengthandwc -l, so the practical risk is low, but env indirection removes the static-analysis finding and keeps the quoting predictable.♻️ Proposed fix for the two steps
- name: Compare against the baseline id: gate + env: + SELECTED: ${{ needs.discover.outputs.count }} run: |- echo "Checking ${`#files`[@]} of ${{ needs.discover.outputs.count }} selected project(s)." + echo "Checking ${`#files`[@]} of ${SELECTED} selected project(s)."- name: Summary if: always() + env: + LEGS: ${{ steps.gate.outputs.legs || 0 }} + SELECTED: ${{ needs.discover.outputs.count }} + TOTAL: ${{ needs.discover.outputs.total }} run: |- echo "Coverage: ${{ steps.gate.outputs.legs || 0 }} of ${{ needs.discover.outputs.count }} selected project(s), out of ${{ needs.discover.outputs.total }} in the repo." + echo "Coverage: ${LEGS} of ${SELECTED} selected project(s), out of ${TOTAL} in the repo."Also applies to: 308-308
🤖 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 @.github/workflows/linux-simulator.yml at line 286, Update the affected workflow steps around the selected-project count and total count to pass needs.discover.outputs.count and needs.discover.outputs.total through step environment variables, then reference those variables in the shell commands instead of embedding GitHub expressions directly. Preserve the existing echo behavior and apply the change to both occurrences.Source: Linters/SAST tools
251-258: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExclude cancelled runs from the gate.
always()also evaluates true when the run is cancelled. A cancelled build leg uploads no counts, so the gate reaches the "No warning counts" branch and reports a failed required check for a cancellation. Use!cancelled()to keep the failed-build behavior and drop the cancellation case.♻️ Proposed fix for the job condition
- if: always() && needs.discover.result == 'success' && needs.discover.outputs.projects != '[]' + if: !cancelled() && needs.discover.result == 'success' && needs.discover.outputs.projects != '[]'🤖 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 @.github/workflows/linux-simulator.yml around lines 251 - 258, Update the warnings job condition for the Warning ratchet job to include !cancelled() alongside always(), while preserving the existing discover-success and non-empty projects checks so failed builds are still evaluated but cancelled runs are excluded.
🤖 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 @.github/scripts/warning_baseline.py:
- Around line 58-68: Update the BASELINE_HEADER constant in
.github/scripts/warning_baseline.py at lines 58-68 to reference
.github/scripts/warning_baseline.py, then update the generated header in
.github/warning-baseline.txt at lines 1-2 to match the corrected text.
In @.github/workflows/linux-simulator.yml:
- Around line 28-34: Add .github/warning-baseline.txt to the pull_request paths
filter in the workflow while leaving the push paths filter unchanged. Update the
nearby exclusion comment to clarify that the file is excluded only from push
triggers, preserving the existing ratchet loop prevention.
- Around line 374-392: Check the exit status of the git commit in the baseline
update flow before attempting git push. If git commit fails, immediately fail
the workflow step and do not print “Baseline lowered” or report success; only
continue to the existing push and summary logic after a successful commit.
- Around line 344-352: Update the coverage guard around expected so it reads the
discovered count through an environment variable and validates that value is
present and strictly numeric before comparing it with the files array length.
Ensure invalid or empty expected values emit the existing error and exit
nonzero, preventing baseline rewriting; retain the existing mismatch rejection
for valid counts.
---
Nitpick comments:
In @.github/workflows/linux-simulator.yml:
- Line 286: Update the affected workflow steps around the selected-project count
and total count to pass needs.discover.outputs.count and
needs.discover.outputs.total through step environment variables, then reference
those variables in the shell commands instead of embedding GitHub expressions
directly. Preserve the existing echo behavior and apply the change to both
occurrences.
- Around line 251-258: Update the warnings job condition for the Warning ratchet
job to include !cancelled() alongside always(), while preserving the existing
discover-success and non-empty projects checks so failed builds are still
evaluated but cancelled runs are excluded.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a7474a86-f419-4ae0-98cd-8d770c341204
📒 Files selected for processing (3)
.github/scripts/warning_baseline.py.github/warning-baseline.txt.github/workflows/linux-simulator.yml
The gcc simulator build has always asked for -Wall -Wextra -Wformat=2, and we have always thrown the answers away: una/Makefile appends -Wno-error after requesting -Werror, and the log only ever became an uploaded artifact. UNAWatch#247 hand-fixed a printf("%d", size_t) that gcc had already reported 1,596 times in the CI run for the branch that landed its neighbours. At 75 unrelated warning sites, a number that large is indistinguishable from zero. Keys on (path, flag) with a count rather than on line numbers, so editing above a pre-existing warning doesn't churn the baseline, and aggregates across projects with max() rather than sum() so the baseline describes the code and a PR that builds one app can still be checked against it. The gate needs no write token, so it works from a fork. Only a push to main that built every project may lower the baseline, and it may only ever lower it: an increase there means the gate was bypassed and should stay a failure.
The generated baseline header named a path the script has never lived at; it is regenerated verbatim from BASELINE_HEADER, so both copies drifted together. warning-baseline.txt joins the pull_request paths filter. It stays out of the push filter for the reason the old comment gave -- the ratchet commits it to main, and a self-trigger would loop -- but excluding it from both meant a PR that edited only the baseline never started the workflow, so the hand-raised-baseline compare never ran and the required gate never reported. The coverage guard now reads the project count from env and rejects a non-numeric one before comparing. There is no `set -e` in that step, and `[ x -ne "" ]` exits 2, which an `if` reads as false: the guard meant to refuse a partial build would have fallen through and rewritten the baseline from it. Reading through env also drops the last inline expansion in a run: block that zizmor flagged. Same missing `set -e` made the commit unchecked: a failed `git commit` leaves `git push` with nothing to send, and it exits 0 saying "Everything up-to-date", so the run reported a lowered baseline it never wrote.
tobymurray
force-pushed
the
ci/ratcheting-warning-gate
branch
from
August 9, 2026 01:10
bd517ed to
4347807
Compare
…ting The gate could be satisfied without the warnings being checked, and the half that lowers the baseline could not run at all. Seven fixes, in rough order of how likely each was to bite. The gate never ran on changes to the gate. `.github/scripts/warning_baseline.py` and `.github/warning-baseline.txt` trigger the workflow but were absent from discover's build-all list, so a PR touching only them selected no project -- which skipped the build, the comparison, and the script's own selftest. A skipped job satisfies a required check, so the one change that can disable the ratchet outright was the one change it never inspected. Both are now in the build-all list, and the comment claiming otherwise is corrected. `bash -e` is GitHub's default shell and `set -uo pipefail` does not clear it, so the careful `rc=$?` handling in the gate and ratchet steps was dead code. `update` exits 2 for "baseline unchanged" -- the usual outcome of a merge -- and errexit turned that into a red ratchet job on almost every push to main. Both steps now `set +e` explicitly, which also restores the gate's error annotation. A push rejected by branch protection was reported as a lost race, retried three times and swallowed as a warning with exit 0. Making `warnings` required makes it apply to the bot's own push, so this was going to be the permanent state: the baseline could go up by review and never come back down, silently and green. Rejections are now classified, and protection failures are fatal. Nothing asserted the flags were still on the command line -- and nothing could, since una/Makefile compiles with a leading `@` and the flags never reach the log. Dropping -Wcast-qual from one app read as a fix to `check` and as a reason to delete those keys to `update`, after which restoring the flag failed the gate on every PR: the ratchet blocked its own repair. `flags` reads WARN/CXXWARN out of all fifteen Makefiles and fails on a missing warning, a new -Wno-, or a bare -w. Retiring a warning now means editing REQUIRED_WARNINGS, in one visible place. `update` had no floor. An empty observation from all fifteen legs -- a stripped flag, a parser that stopped matching -- passed every guard and wiped the baseline for good. Drops past half the total now need --allow-collapse. The gate passed on partial coverage: it only failed when *every* leg produced nothing, so one flaky apt-get was enough to leave a project unchecked and green. It now requires a count file per selected project, and fails if any build leg did not succeed -- removing -Wno-error, for instance, turns every warning into an `error:` line the parser cannot see, which used to read as zero warnings on a broken build. A dead discover job skipped the gate entirely; it is now asserted in a step, and a discover that finds no projects fails rather than vouching for an empty build. Two smaller ones: GNU make's own `Makefile:224: warning:` diagnostics have gcc's exact shape and were counted as code warnings, and the failure output named the file and flag but not the line, so reacting to it started with downloading an artifact. `extract --details` now carries line, column and message through to the failure text. Selftests grow 30 -> 47, including a check that reads the real Makefiles.
…ions too The flag policy only read una/Makefile, which is where the warning lists live but not the only file that reaches the command line. config/gcc/app.mk is included before them and already sets user_cflags in one project, and simulator/gcc/Makefile exports into the same sub-make; `override user_cflags +=` appends to whatever either left behind. So `-w` was still one line away, in a file the check was not looking at. Both siblings are now scanned for suppressions -- 45 make fragments across the 15 projects rather than 15 -- and the scan keys off the assigned variable name rather than a substring, so it covers CXXFLAGS and compiler_options as well and does not mistake `$(addprefix -W,$(WARN))` for one.
always() fires on cancellation too, so a push superseded by cancel-in-progress reached the warnings job, found discover 'cancelled', and posted a failing required check for a run nobody was waiting on. A gate that goes red on someone else's second push is the kind that gets turned off.
tobymurray
marked this pull request as ready for review
August 9, 2026 02:13
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/linux-simulator.yml (1)
454-518: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not update a newer
mainrevision with stale warning counts.
filescontains observations from the build atGITHUB_SHA. After a rejected push, the next clone can contain a newermainrevision. The retry then runsupdateagainst that newer baseline without rebuilding its source and Makefiles.If an intervening commit changes compiler flags, sources, or
warning_baseline.py, this can lower the baseline using incompatible evidence. Stop ratcheting whenmainadvances and let the workflow for the new tip produce fresh counts. If retries remain required, verify that intervening commits changed only the baseline before reusing these artifacts.🤖 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 @.github/workflows/linux-simulator.yml around lines 454 - 518, Prevent the retry loop from applying stale files observations after a rejected push: record the cloned main revision and compare it with the original GITHUB_SHA before running warning_baseline.py update. If main has advanced, stop with an error instead of recalculating against the newer checkout; only permit a retry when the intervening change is exclusively the warning baseline, otherwise require a fresh build for the new tip.
🧹 Nitpick comments (2)
.github/scripts/warning_baseline.py (2)
891-898: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestCommittedMakefilesdoes not cover the sibling fragments.
cmd_flagsscansuna/Makefileplusconfig/gcc/app.mkandsimulator/gcc/Makefilefor each project. This test scans onlyuna/Makefile. A committed sibling that adds-wtherefore passes--selftestand fails only in the workflow step that runsflags.Consider running both paths through one helper, or extending the loop with
check_suppressionson the siblings.♻️ Suggested extension
def test_every_project_still_requests_them(self): offenders = {} for rel in self.makefiles: with open(os.path.join(REPO_ROOT, rel), "r", encoding="utf-8") as fh: problems = check_makefile_flags(fh.read()) if problems: offenders[rel] = problems + project = posixpath.dirname(posixpath.dirname(rel)) + for sibling in SUPPRESSION_SCAN_SIBLINGS: + sib_rel = posixpath.join(project, sibling) + sib_abs = os.path.join(REPO_ROOT, sib_rel) + if not os.path.exists(sib_abs): + continue + with open(sib_abs, "r", encoding="utf-8") as fh: + sib_problems = check_suppressions(fh.read()) + if sib_problems: + offenders[sib_rel] = sib_problems self.assertEqual(offenders, {})🤖 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 @.github/scripts/warning_baseline.py around lines 891 - 898, Update TestCommittedMakefiles.test_every_project_still_requests_them to validate every fragment scanned by cmd_flags, not only each project’s una/Makefile. Reuse the existing check_makefile_flags/check_suppressions helpers as appropriate and include config/gcc/app.mk and simulator/gcc/Makefile for each project, preserving the offenders aggregation and empty-result assertion.
312-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider stripping
#comments before the flag scan.
_logical_lineskeeps comment text. A commented-out line such as# user_cflags += -wtherefore produces adisables all warnings with -wfailure. The direction is safe, but the message names a line that has no effect on the build.♻️ Optional: drop comments in
_logical_linesdef _logical_lines(text): """Makefile lines with backslash continuations joined.""" - return re.sub(r"\\\n\s*", " ", text).splitlines() + joined = re.sub(r"\\\n\s*", " ", text) + return [re.sub(r"(?<!\\)#.*$", "", line) for line in joined.splitlines()]Note that a
#inside a recipe or a quoted value would also be removed, so confirm no Makefile in the tree relies on that.🤖 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 @.github/scripts/warning_baseline.py around lines 312 - 330, Update _logical_lines to remove or ignore Makefile comment text before check_suppressions scans assignments, preventing commented-out flag assignments from being reported. Preserve meaningful flag values while confirming that stripping comments does not break recipes or quoted values used by the repository.
🤖 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 @.github/scripts/warning_baseline.py:
- Around line 333-366: Update the assignment parsing in check_makefile_flags to
distinguish += from replacement operators accepted by ASSIGN_RE. Append tokens
to the existing lists[name] value for += while retaining replacement behavior
for =, :=, and ?=, so both required warnings and suppressions across multiple
assignments are validated.</code>
In @.github/workflows/linux-simulator.yml:
- Around line 372-379: Update the warning-ratchet summary in the workflow’s
gate-reporting block to base the heading on the overall job result, marking
failures in project selection, self-test, or flag validation as failures rather
than successes. Emit the no-selection message only when the discover step
succeeded and steps.gate was intentionally skipped; preserve the existing
success and warning details for all other outcomes.
---
Outside diff comments:
In @.github/workflows/linux-simulator.yml:
- Around line 454-518: Prevent the retry loop from applying stale files
observations after a rejected push: record the cloned main revision and compare
it with the original GITHUB_SHA before running warning_baseline.py update. If
main has advanced, stop with an error instead of recalculating against the newer
checkout; only permit a retry when the intervening change is exclusively the
warning baseline, otherwise require a fresh build for the new tip.
---
Nitpick comments:
In @.github/scripts/warning_baseline.py:
- Around line 891-898: Update
TestCommittedMakefiles.test_every_project_still_requests_them to validate every
fragment scanned by cmd_flags, not only each project’s una/Makefile. Reuse the
existing check_makefile_flags/check_suppressions helpers as appropriate and
include config/gcc/app.mk and simulator/gcc/Makefile for each project,
preserving the offenders aggregation and empty-result assertion.
- Around line 312-330: Update _logical_lines to remove or ignore Makefile
comment text before check_suppressions scans assignments, preventing
commented-out flag assignments from being reported. Preserve meaningful flag
values while confirming that stripping comments does not break recipes or quoted
values used by the repository.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f12acb10-c720-4f4d-9dde-6b665c3bb07d
📒 Files selected for processing (2)
.github/scripts/warning_baseline.py.github/workflows/linux-simulator.yml
Both from CodeRabbit review, both reproduced first. The flag policy read every assignment operator as a replacement, so only the last WARN/CXXWARN line survived. An ordinary `WARN = ...` / `WARN += ...` split reported the first line's flags as missing -- a red required check with a wrong reason -- and, the other way round, a suppression on the first line hid behind a clean += on the last. That second one is a working bypass, not a theoretical one: make emits -Wno-pedantic ahead of the required flags, nothing later re-enables it, and every -Wpedantic entry in the baseline disappears while the check reports OK. Confirmed against gcc. The operators now follow make's own semantics, including ?= not overriding a variable that is already set. The job summary keyed its heading on the gate step's outcome, which is 'skipped' when the selection check, the selftest or the flag policy failed first -- so a red job printed a tick and "no simulator project was selected". It now keys on job.status, distinguishes "nothing to check" from "the comparison never ran", and the build-leg assertion moved above it so the summary can see that failure too.
The key has no line number by design, so for a 2 -> 3 regression the tool cannot know which of the three sites is the new one -- but listing them bare read as if it did, and would send someone to fix a warning the baseline already tolerated. The list now says so, and says how many sites it truncated. Also records, where the flag policy is defined, that it reads assignments rather than the command line: `EXTRA := -w` expanded into a flags variable later still gets through. It is a tripwire against casual weakening, not a sandbox, and closing it properly means observing gcc's actual invocation -- a Makefile change, not a CI one.
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.
The gcc simulator build has always asked for -Wall -Wextra -Wformat=2, and we have always thrown the answers away. una/Makefile appends -Wno-error after requesting -Werror, and the log only ever became an uploaded artifact.
#247 hand-fixed a
printf("%d", size_t)that gcc had already reported many times in the CI run for the branch that landed its neighbours. At 75 unrelated warning sites, a number that large is indistinguishable from zero.Keys on (path, flag) with a count rather than on line numbers, so editing above a pre-existing warning doesn't churn the baseline, and aggregates across projects with max() rather than sum() so the baseline describes the code and a PR that builds one app can still be checked against it.
The gate needs no write token, so it works from a fork. Only a push to main that built every project may lower the baseline, and it may only ever lower it: an increase there means the gate was bypassed and should stay a failure.
A count only means something while the compiler is still being asked for the warnings, and no build log can show that: una/Makefile compiles with a leading
@, so the flags never reach the log. The gate reads the warning lists out of the Makefiles instead, and fails if one is dropped, a suppression is added, or-wturns up. Retiring a warning is then an edit to the script, where a reviewer sees it, rather than to one app's Makefile.Setup:
Warning ratchethas to be made a required check, or none of this blocks anything. Required checks apply to direct pushes too, sogithub-actions[bot]also needs a bypass on main. Without one the baseline can be raised by review but never lowered again. The job fails loudly on that rejection rather than mistaking it for a lost push race.Summary by CodeRabbit
New Features
Documentation