Close the split-token and reflection bypasses, and self-test the guard

[kwsantiago]

Summary

Two ways to reach a banned generator without the scanner seeing it. Both closed, plus scripts/test-rng-hygiene.sh so the rules are asserted rather than assumed.

The bypasses

Reproduced against a positive control that does fail. Probes under app/src/main/kotlin, which matters: the guard deliberately skips test, androidTest and testFixtures, so a probe placed there passes and looks like a bypass when it is only misplaced. My first attempt did exactly that, and the control is what caught it.

probe	before	after
Math.random() (control)	caught	caught
Math. newline random()	passed	caught
java.util. newline Random()	passed	caught
Class.forName("java.util." + "Random")	passed	caught
classLoader.loadClass("java.util.Random")	passed	caught

Split member access. Kotlin continues an expression after a trailing dot, so Math. and random() on separate lines are the same call. The guard already buffered across unbalanced parentheses; a trailing dot now continues the buffer the same way. The join is deliberately without a separator: reassembling as Math. random() still fails to match Math[.]random, which is the bug the first version of this fix had.

Reflection. A class name built by concatenation defeats any literal match, and Class.forName reaches the same PRNG. There is no legitimate Class.forName or loadClass in production sources today (checked), so the rule is simply: don't, with a marker available if that changes.

Why this class keeps appearing

Third repository this week with the same shape. keep-node #108: variable indirection and string concatenation past a line matcher. keep-esp32 #154: a C line continuation splitting an identifier. Here: a Kotlin member access splitting a call. A line-oriented scanner loses to anything that splits the token, so each language needs its own splice rule, and each needs a test that says so.

The self-test

Ported from keep-esp32, keeping the two properties that make it trustworthy:

• every reject case requires the guard to name the probe file, so a guard aborting for an unrelated reason is reported as WRONG REASON rather than credited as a detection
• the harness aborts if fewer than 10 files stage, so "scanned almost nothing" cannot look like a pass

Ten cases: seven rejections including both split forms, both reflection routes, setSeed and ThreadLocalRandom; three acceptances.

Test plan

• All bypasses caught; clean tree still passes
• Self-test 10/10 on the fixed guard
• Regression control: against the pre-fix guard it reports 3 BYPASS and exits 1
• Confirmed no production use of Class.forName/loadClass before making it a hard rule
• bash -n clean, ci.yml parses, release-changelog gate still passes
• CI

Not claimed: this closes known evasions, it does not make a line-oriented scanner undefeatable.

Summary by CodeRabbit

• Tests

  • Added automated self-checks to verify detection of unsafe random-number usage, including split references, reflection, and class loading.
  • Expanded validation to confirm both prohibited patterns and approved code are handled correctly.

• Chores

  • Integrated the new safeguards into continuous integration.
  • Improved detection across multiline and concatenated code expressions for more reliable build-time checks.

[coderabbitai]

Warning

Review limit reached

@kwsantiago, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: dcf5cdfb-74a0-4f10-912c-85f647f9fc7a

📥 Commits

Reviewing files that changed from the base of the PR and between 26d9b09 and 753b9a9.

📒 Files selected for processing (1)

• scripts/test-rng-hygiene.sh

Walkthrough

The RNG hygiene guard now handles split member access, concatenated class names, and reflective loading. A self-test harness validates rejection and acceptance cases in an isolated Git index. CI runs the harness before the existing hygiene check.

Changes

RNG hygiene validation

Layer / File(s)	Summary
Expand RNG detection rules scripts/check-rng-hygiene.sh	The scanner preserves multiline member access, joins adjacent string literals, and detects reflective PRNG loading.
Add isolated guard self-tests scripts/test-rng-hygiene.sh	The harness stages temporary Kotlin probes, validates expected results, removes temporary state, and reports failures.
Run self-tests in CI .github/workflows/ci.yml	The build runs the self-test before the existing RNG hygiene validation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CI
  participant TestHarness as test-rng-hygiene.sh
  participant Guard as check-rng-hygiene.sh
  participant GitIndex as temporary Git index
  CI->>TestHarness: Run RNG hygiene self-test
  TestHarness->>GitIndex: Stage a temporary probe
  TestHarness->>Guard: Check the staged probe
  Guard-->>TestHarness: Return detection result
  TestHarness->>GitIndex: Remove the temporary probe
  TestHarness-->>CI: Return pass or failure

Loading

Possibly related PRs

• privkeyio/keep-android#470: Introduced the RNG hygiene guard that this change extends and self-tests.

Poem

A rabbit checks each random trail,
Split dots and class names cannot prevail.
Reflections now face the guard’s bright light,
CI tests each probe both wrong and right.
Clean paws, clear logs, and checks all green!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the guard fixes and the added self-test harness.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-guard-splice

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 `@scripts/test-rng-hygiene.sh`:
- Around line 45-46: Update the temporary-index setup in the test harness to
check the exit status of both `git read-tree` and `git add` instead of
suppressing failures. Record a harness failure when either command fails, before
invoking the guard, and prevent stale `$staged` contents from being treated as a
successful pass.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0e4b18f8-88c8-44e7-8d39-0aa78dab2374

📥 Commits

Reviewing files that changed from the base of the PR and between 12c39c7 and 26d9b09.

📒 Files selected for processing (3)

• .github/workflows/ci.yml
• scripts/check-rng-hygiene.sh
• scripts/test-rng-hygiene.sh

[kwsantiago]

Valid, fixed here and in the same harness in privkeyio/keep#936, which is still open.

The staged -lt 10 check does not cover this. A failed git add still leaves HEAD's whole tree in the index, so the count sails past 10 while the probe itself is missing, and the guard is then judged on a file it never saw. In the reject direction that surfaces as a spurious BYPASS, which is at least loud; in the accept direction it is a silent pass.

Now asserted directly:

if ! GIT_INDEX_FILE="$TMPD/index" git ls-files --error-unmatch "$name" >/dev/null 2>&1; then
    echo "  HARNESS BROKEN: $name was not staged; the guard would never see it"

Verified by sabotage rather than by reading: replaced the git add with a no-op and every case reported HARNESS BROKEN: ... was not staged instead of a verdict. Restored, 10/10 again.

That is the third harness defect found in this pattern, after the exit-code-only check and the two-line probes in keep-node#108. Each had the same shape: the test reaching a verdict without having established the conditions the verdict depends on. Worth noting since the same harness now runs in four repositories.

The two already merged, keep-node#108 and keep-esp32#154, carry this same gap. Following up there separately rather than leaving the four copies divergent.