ci: fix remaining failures + migrate to self-hosted runners#91
ci: fix remaining failures + migrate to self-hosted runners#91
Conversation
_inject_metrics_headers(None) returns {'X-CacheKit-L1-Status': 'disabled'}
since #64, but 3 tests still expected {}. Session ID uniqueness test
used default function_identifier for all instances, producing identical IDs.
Every test in this file exercises pure Python functions (_inject_metrics_headers, _FunctionStats, get_session_id) with zero Redis or network I/O. Placing them in integration/ forced a pytest-redis dependency for no reason.
- Mark redis.md and encryption.md examples as notest (require DI/env setup the doc test harness can't provide) - Replace kani-github-action composite action with direct install on self-hosted runner — upstream action uses unpinned dtolnay/rust-toolchain and actions/checkout refs that violate org SHA-pinning policy
Move all jobs to cachekit runners except summary gates and release-please (needs cross-platform matrix). Remove dtolnay/rust-toolchain actions, Swatinem/rust-cache, actions/cache, and astral-sh/setup-uv — all pre-installed on self-hosted infra. Replaces Kani composite action with direct install to avoid upstream unpinned action refs.
|
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 13 minutes and 48 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 (9)
📝 WalkthroughWalkthroughGitHub Actions workflows are migrated from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cargo-deny-action uses Docker which breaks on self-hosted workspace paths — replace with direct cargo-deny install. Add pip GHSA-58qw-9mgm-455v ignore (pip tar/zip confusion, no fix available).
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/test_saas_observability.py (1)
368-390:⚠️ Potential issue | 🟡 MinorTest no longer matches its stated intent — consider renaming or strengthening.
The docstring frames this as verifying "per-instance" uniqueness for "multi-wrapper scenarios (e.g., Locust load testing where multiple users each decorate the same function)". After this change, what is actually verified is per-
function_identifieruniqueness. Per_FunctionStats._ensure_session_id(src/cachekit/decorators/wrapper.py), session_id isf"{get_session_id()}:{self._function_identifier}"with a process-scoped UUID — so two_FunctionStatsinstances created with the samefunction_identifier(precisely the "same function decorated by multiple users" scenario the docstring calls out) will still collide.The change is fine to unblock CI, but the test now trivially passes because the three identifiers differ. Consider either:
- renaming the test and updating the docstring to reflect what is actually tested (per-function-identifier uniqueness), or
- adding a separate assertion for the multi-wrapper-same-function scenario if per-instance uniqueness is genuinely required (which would require an implementation change, not just a test change).
📝 Suggested docstring/name realignment (test-only)
- def test_session_unique_per_stats_instance(self): - """Verify each _FunctionStats instance has its own unique session ID. - - This is critical for multi-wrapper scenarios (e.g., Locust load testing where - multiple users each decorate the same function). Without per-instance session IDs, - different wrappers would collide and cause 'counters_decreased' validation errors. - """ + def test_session_unique_per_function_identifier(self): + """Verify _FunctionStats instances with distinct function_identifier values produce + distinct session IDs. + + Note: Within a single process, two _FunctionStats with the same function_identifier + will share a session ID, since session_id = f"{process_uuid}:{function_identifier}". + """ stats1 = _FunctionStats(function_identifier="module.func_a") stats2 = _FunctionStats(function_identifier="module.func_b") stats3 = _FunctionStats(function_identifier="module.func_c")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_saas_observability.py` around lines 368 - 390, The test name and docstring for test_session_unique_per_stats_instance no longer match behavior: _FunctionStats._ensure_session_id composes session IDs as f"{get_session_id()}:{self._function_identifier}", so uniqueness is per function_identifier, not per instance. Rename the test (e.g., test_session_unique_per_function_identifier) and update its docstring to state it verifies per-function-identifier uniqueness, and keep using _inject_metrics_headers and the existing assertions; if you instead want to assert true per-instance uniqueness for the same function_identifier, add a new test that creates two _FunctionStats with the same function_identifier and asserts their session IDs differ—but note that making that test pass will require changing _FunctionStats._ensure_session_id / get_session_id implementation rather than just test code.
🧹 Nitpick comments (2)
docs/backends/redis.md (1)
9-9: Add explanatory comment fornotestdirective.The
notestdirective is appropriate since this example requires a real Redis connection. However, an explanatory comment should be added to clarify why the example is skipped. As per coding guidelines from CONTRIBUTING.md:66-90: "Always include a comment explaining why the example is skipped."📝 Suggested addition
```python notest +# Real Redis connection required - not available in test environment from cachekit.backends import RedisBackend🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/backends/redis.md` at line 9, Add an explanatory comment after the "python notest" directive in docs/backends/redis.md to explain why the example is skipped (e.g., requires a real Redis connection not available in test environment); specifically update the code block header that starts with "python notest" and insert a short comment line clarifying the reason so readers and automated checks understand why this example is excluded..github/workflows/fuzz-smoke.yml (1)
31-34: Optional: avoid mutating global rustup default on a persistent runner.Since
cargo +nightly fuzz runis already used at line 73, therustup default nightlycommand is unnecessary. On the self-hostedcachekitrunner, this mutates~/.rustup/settings.toml, persisting the nightly default into unrelated subsequent jobs on the same runner. Installing the toolchain without flipping the default preserves better job isolation.♻️ Suggested tweak
- name: Install Rust nightly run: | - rustup toolchain install nightly - rustup default nightly + rustup toolchain install nightly --profile minimal🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/fuzz-smoke.yml around lines 31 - 34, The workflow sets the global rustup default to nightly (rustup default nightly), which mutates runner state; remove the rustup default nightly line and only install the toolchain (keep rustup toolchain install nightly) so the job uses explicit toolchain invocations like cargo +nightly fuzz run (already used at line 73) without changing the runner-wide default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/security-deep.yml:
- Around line 24-32: The Install Kani step currently uses "cargo +stable install
--locked kani-verifier" which is not pinned; update that command in the "Install
Kani" step to pin the binary version (e.g., add "--version 0.67.0" or the
intended release) so nightly runs are reproducible, and keep the subsequent
"cargo-kani setup" call; additionally add a preceding step to ensure the stable
toolchain exists on the runner (e.g., run rustup toolchain install stable or
equivalent) so "cargo +stable" will not fail on runners without a stable alias.
---
Outside diff comments:
In `@tests/unit/test_saas_observability.py`:
- Around line 368-390: The test name and docstring for
test_session_unique_per_stats_instance no longer match behavior:
_FunctionStats._ensure_session_id composes session IDs as
f"{get_session_id()}:{self._function_identifier}", so uniqueness is per
function_identifier, not per instance. Rename the test (e.g.,
test_session_unique_per_function_identifier) and update its docstring to state
it verifies per-function-identifier uniqueness, and keep using
_inject_metrics_headers and the existing assertions; if you instead want to
assert true per-instance uniqueness for the same function_identifier, add a new
test that creates two _FunctionStats with the same function_identifier and
asserts their session IDs differ—but note that making that test pass will
require changing _FunctionStats._ensure_session_id / get_session_id
implementation rather than just test code.
---
Nitpick comments:
In @.github/workflows/fuzz-smoke.yml:
- Around line 31-34: The workflow sets the global rustup default to nightly
(rustup default nightly), which mutates runner state; remove the rustup default
nightly line and only install the toolchain (keep rustup toolchain install
nightly) so the job uses explicit toolchain invocations like cargo +nightly fuzz
run (already used at line 73) without changing the runner-wide default.
In `@docs/backends/redis.md`:
- Line 9: Add an explanatory comment after the "python notest" directive in
docs/backends/redis.md to explain why the example is skipped (e.g., requires a
real Redis connection not available in test environment); specifically update
the code block header that starts with "python notest" and insert a short
comment line clarifying the reason so readers and automated checks understand
why this example is 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97b6cb50-2e53-4ef0-a7af-e5212722eaab
📒 Files selected for processing (8)
.github/workflows/codeql.yml.github/workflows/fuzz-smoke.yml.github/workflows/security-deep.yml.github/workflows/security-fast.yml.github/workflows/security-medium.ymldocs/backends/redis.mddocs/serializers/encryption.mdtests/unit/test_saas_observability.py
- Pin kani-verifier to 0.67.0 for reproducible nightly runs - Ensure stable toolchain before cargo +stable install - Drop rustup default nightly in fuzz-smoke (already uses cargo +nightly) - Rename test to test_session_unique_per_function_identifier (matches behavior) - Add notest reason comments to redis.md and encryption.md
Security Deep: 2:00 UTC → 20:00 UTC (7 AM AEDT, results ready by morning) CodeQL: 3:00 UTC Sunday → 20:00 UTC Sunday (7 AM AEDT Monday)
validate_encryption_config() checks the env var independently of the inline master_key param passed to @cache.secure. The test was flaky (68% fail rate on main) because it depended on whether CACHEKIT_MASTER_KEY happened to be set from a prior test's singleton cache.
1 participant
Summary
_inject_metrics_headers(None)behavior changed in fix: default L1-Status header for standalone CachekitIO usage #64, tests not updated)test_saas_observability.pyfromintegration/tounit/(pure Python, no Redis needed)redis.mdandencryption.mddoc examples asnotest(require DI/env setup)cachekitself-hosted runners (except summary gates + release-please)dtolnay/rust-toolchain,Swatinem/rust-cache,actions/cache,astral-sh/setup-uv(pre-installed on infra)kani-github-actioncomposite action with direct install (upstream uses unpinned refs)Test plan
tests/unit/make test-docs-examples)Summary by CodeRabbit
Documentation
Chores
Tests