Splice line continuations before scanning, and self-test the guard - #154
Conversation
WalkthroughThe RNG hygiene guard now joins continued lines before scanning and reports their starting line. A Bash self-test covers rejection and acceptance cases. CI runs the self-test before the existing hygiene check. ChangesRNG Hygiene Validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
scripts/test-rng-hygiene.sh (1)
81-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact reported line, not just the filename.
run_probeforprobe_splice.candprobe_splice3.conly checks that the guard's output names the file. It does not check that the guard reports the first physical line, which is the specific behavior this PR fixes (startlineincheck-rng-hygiene.sh). A guard that regresses to reporting the last physical line of a spliced identifier would still pass these two cases.Add an assertion that checks for the exact
file:line:prefix in the fail branch for these two probes.♻️ Proposed tightening (illustrative; adapt to run_probe's structure)
run_probe main/probe_splice.c 'void f(void){ adc_oneshot\ _new_unit(); } ' fail "identifier split by a line continuation" +# Also verify the report points at the first physical line (line 1), not the +# continuation line, e.g. by checking $out for "main/probe_splice.c:1:".🤖 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 `@scripts/test-rng-hygiene.sh` around lines 81 - 88, Update the fail-case assertions for the probe_splice.c and probe_splice3.c run_probe calls in the test script to require the guard output’s exact file:line: prefix for the first physical line of each spliced identifier, rather than only matching the filename. Preserve the existing failure expectations and use run_probe’s established output-matching mechanism.
🤖 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 `@scripts/test-rng-hygiene.sh`:
- Around line 81-88: Update the fail-case assertions for the probe_splice.c and
probe_splice3.c run_probe calls in the test script to require the guard output’s
exact file:line: prefix for the first physical line of each spliced identifier,
rather than only matching the filename. Preserve the existing failure
expectations and use run_probe’s established output-matching mechanism.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a98b793-2f8d-4d72-9a67-a377091a479f
📒 Files selected for processing (3)
.github/workflows/rng-hygiene.ymlscripts/check-rng-hygiene.shscripts/test-rng-hygiene.sh
1 participant
Summary
A backslash-newline inside an identifier walked straight past the scanner. Splices continuations before matching, and adds
scripts/test-rng-hygiene.shso the rules are asserted rather than assumed.The bypass
That is legal C. Translation phase 2 removes the backslash-newline before tokenization, so the compiler sees
adc_oneshot_new_unit()while the guard, matching per physical line, seesadc_oneshot\and_new_unit();and matches neither.Verified against a positive control that does fail:
esp_wifi_start();(control)Why this rule in particular
It defeats rule 5, the one forbidding anything that contends for the SAR ADC. That rule is the enforcement behind the v0.2.1 entropy fix:
bootloader_random_enable()shares the SAR ADC, so a driver taking it back silently returns the device to the pseudo-random state that shipped in v0.1.0 through v0.2.0, with every health indicator still green. The whole point of that release was that the failure is invisible from the output, which is why the guard has to be the thing that catches it.Macro aliasing was tested too and is not a bypass:
#define WIFI_UP esp_wifi_startputs the banned token on the#defineline, which the scanner catches. Only the splice got through.Findings still point at the right line
Splicing shifts
FNR, so the finding is reported againststartline, the first physical line of the spliced group. Confirmed: the two-line probe reports:2where the identifier begins, the three-line probe reports:1.The self-test
Ported from the pattern added in keep-node, including the two things that made it trustworthy there:
WRONG REASONinstead of being credited as a detectionTen cases: seven rejections including both splice forms,
bootloader_random_disable, a raw draw outsidemain/hw_entropy.c, libc PRNG and I2S contention; three acceptances covering ordinary code, a banned token in a comment, and one inside#if 0.Probes stage into a throwaway
GIT_INDEX_FILE, so the real index is untouched. The file does exist on disk while the guard runs, because the scanner reads bytes, and is removed on every path including the EXIT trap.Test plan
bash -nclean on both scripts, workflow parses, dependency-pin guard still passesNot claimed: this closes a known evasion, it does not make a line-oriented scanner undefeatable. What changed is that this route is closed and every known route is now asserted on each run.
Summary by CodeRabbit
Bug Fixes
Tests
Chores