Conversation
Wrong jq paths (.package.functions vs .unsafety.used.functions) produced null, and bc isn't on the runner. Replaced with correct paths and scoped check to cachekit-rs only — checking all deps is meaningless since crypto crates (ring, aes-gcm) are inherently unsafe.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe GitHub Actions security workflow is modified to replace a global unsafe-function ratio threshold with a stricter per-crate enforcement, specifically requiring zero unsafe functions in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
.github/workflows/security-medium.yml (1)
43-48: Harden the jq lookup against a missingcachekit-rsentry in the geiger report.When the
select(.package.id.name == "cachekit-rs")predicate matches nothing (e.g., if the crate is removed, renamed, or the geiger report schema changes),UNSAFE_FUNCSbecomes an empty string and the arithmetic comparison[ "$UNSAFE_FUNCS" -ne 0 ]fails with a cryptic bash error (integer expression expected) instead of an actionable diagnostic. The job still exits with failure (as intended), but the error message won't make the cause obvious.Add a pre-check to validate the lookup succeeded and produce a clear failure message:
Proposed hardening
- UNSAFE_FUNCS=$(jq '.packages[] | select(.package.id.name == "cachekit-rs") | .unsafety.used.functions.unsafe_' geiger-report.json) - echo "cachekit-rs unsafe functions: $UNSAFE_FUNCS" - if [ "$UNSAFE_FUNCS" -ne 0 ]; then - echo "cachekit-rs has $UNSAFE_FUNCS unsafe functions — must be zero" - exit 1 - fi + UNSAFE_FUNCS=$(jq -r 'first(.packages[] | select(.package.id.name == "cachekit-rs") | .unsafety.used.functions.unsafe_) // empty' geiger-report.json) + if [ -z "$UNSAFE_FUNCS" ]; then + echo "cachekit-rs package not found in geiger report" + exit 1 + fi + echo "cachekit-rs unsafe functions: $UNSAFE_FUNCS" + if [ "$UNSAFE_FUNCS" -ne 0 ]; then + echo "cachekit-rs has $UNSAFE_FUNCS unsafe functions — must be zero" + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/security-medium.yml around lines 43 - 48, The current jq lookup for UNSAFE_FUNCS can produce an empty string if select(.package.id.name == "cachekit-rs") finds nothing, causing the numeric test [ "$UNSAFE_FUNCS" -ne 0 ] to fail with a cryptic error; update the logic that sets UNSAFE_FUNCS (the jq select on geiger-report.json) to first detect a missing result and fail with a clear message, e.g., check if UNSAFE_FUNCS is empty (test -z "$UNSAFE_FUNCS" or use jq to provide a null/default) and if so echo a descriptive error like "cachekit-rs entry missing in geiger-report.json" and exit 1; otherwise proceed to the numeric comparison of UNSAFE_FUNCS against 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/security-medium.yml:
- Around line 43-48: The current jq lookup for UNSAFE_FUNCS can produce an empty
string if select(.package.id.name == "cachekit-rs") finds nothing, causing the
numeric test [ "$UNSAFE_FUNCS" -ne 0 ] to fail with a cryptic error; update the
logic that sets UNSAFE_FUNCS (the jq select on geiger-report.json) to first
detect a missing result and fail with a clear message, e.g., check if
UNSAFE_FUNCS is empty (test -z "$UNSAFE_FUNCS" or use jq to provide a
null/default) and if so echo a descriptive error like "cachekit-rs entry missing
in geiger-report.json" and exit 1; otherwise proceed to the numeric comparison
of UNSAFE_FUNCS against 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d26c3ec-22a6-4f42-83f5-d4f3e6f6af84
📒 Files selected for processing (1)
.github/workflows/security-medium.yml
- test_exists_expired_ttl: TTL 1s→2s, sleep 1.5s→2.5s (1s expired between set() and exists() under CI load) - test_concurrent_access: move asyncio.Lock() inside async function (Python 3.9 requires a running event loop for Lock constructor)
If cachekit-rs isn't found in the geiger report, jq returns empty string and the numeric comparison blows up. Fail cleanly instead.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
validate_encryption_config() reads CACHEKIT_MASTER_KEY from the pydantic-settings singleton. If a prior test already cached the singleton without the key, setting the env var has no effect. Must reset_settings() after setting env var and in finally cleanup.
The validator was checking CACHEKIT_MASTER_KEY env var via the pydantic-settings singleton, ignoring the master_key passed directly to @cache.secure(master_key=...). This caused flaky test failures depending on singleton cache state. Now: explicit master_key param takes precedence over env var. Production warning only fires when key came from env var (not inline).
1 participant
Summary
.package.functionsvs.unsafety.used.functions.unsafe_)bcdependency (not installed on runner)cachekit-rsonly — all-deps check is meaningless since crypto crates (ring, aes-gcm) are inherently unsafeTest plan
Summary by CodeRabbit
Note: This is an internal infrastructure change with no direct user-facing impact.