fix: releases that publish nothing, and docs naming the wrong encryption key (beads batch 7) - #189
Conversation
CashPilot-gn6 / CashPilot-l7t — release.yml decided what to build from two hand-maintained regexes, and both had drifted from the Dockerfiles they were meant to mirror. The UI regex named 13 of 27 modules while the UI image does COPY app/ ./app/ — the whole directory. A change to any of the other 14 (payouts, preflight, power, egress, lan_isolation, notify, ...) set BUILD_UI=false, which skipped the version step, which left new_tag empty, which skipped the tag, the GitHub Release and the entire build job. Skipped steps do not fail a run, so the release went GREEN having published nothing at all. The worker regex omitted egress.py and state_backup.py, both COPY'd into the worker image and imported by worker_api at runtime. With build_worker=false the pipeline RETAGS the previous image, so the worker could be published under a new version tag containing the previous release's code — and verify-tags only runs docker manifest inspect, which a retag satisfies. Neither regex is restated now. Any change under app/ builds the UI, because the image contains all of it; the worker list is parsed out of Dockerfile.worker at run time, so the next module added to that image is covered without anyone remembering. A test asserts every module worker_api imports is one the worker image actually contains — the contract that makes a miss a crash on the user's machine rather than a build failure here. CashPilot-dxi — six places told users CASHPILOT_SECRET_KEY encrypts their credentials. It signs login sessions; CASHPILOT_ENCRYPTION_KEY is the Fernet key, persisted at /data/.fernet_key. The advice was not merely wrong but harmful: following it, a user believes their credentials are portable, never backs up the key file, and loses every stored credential the first time the volume is recreated. README.md already said this correctly and the other six contradicted it. All six corrected, the key file named so it can actually be backed up, and the Unraid template — which is the entire configuration surface for those users — now exposes CASHPILOT_ENCRYPTION_KEY, masked. Negative controls: narrowing the UI trigger fails two tests, restoring the index.md claim fails two more.
|
Warning Review limit reached
Next review available in: 53 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 (4)
📝 WalkthroughWalkthroughThe release workflow now detects UI changes from all ChangesRelease trigger detection
Encryption-key documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_release_triggers.py (1)
92-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse syntax-aware import extraction.
Line 95 matches only one physical
from app import ...form. It ignoresimport app.module,from app.module import name, relative imports, and multiline imports. Ifworker_apiuses one of these forms, this test can pass while the worker image omits the dependency and fails at import time.Parse
worker_api.pywithastand compare every directappdependency withworker_copied_modules().🤖 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 `@tests/test_release_triggers.py` around lines 92 - 100, Replace the line-based import parsing in the release-trigger test with AST-based analysis of worker_api.py. Collect every direct app dependency from Import and ImportFrom nodes, including app.module, from app.module imports, relative imports, and multiline forms, then compare the normalized dependency names against worker_copied_modules(). Preserve the existing filtering of private names and missing-dependency assertion.
🤖 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 `@docs/index.md`:
- Line 129: Update the encryption-key description in the FAQ to state that
CASHPILOT_ENCRYPTION_KEY is used only when /data/.fernet_key is absent, matching
the precedence documented in getting-started.md; preserve the distinction from
CASHPILOT_SECRET_KEY.
---
Nitpick comments:
In `@tests/test_release_triggers.py`:
- Around line 92-100: Replace the line-based import parsing in the
release-trigger test with AST-based analysis of worker_api.py. Collect every
direct app dependency from Import and ImportFrom nodes, including app.module,
from app.module imports, relative imports, and multiline forms, then compare the
normalized dependency names against worker_copied_modules(). Preserve the
existing filtering of private names and missing-dependency assertion.
🪄 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: 55bdd152-565e-4e10-b123-372077054488
📒 Files selected for processing (6)
.github/workflows/release.ymldocs/fleet.mddocs/getting-started.mddocs/index.mdtests/test_release_triggers.pyunraid/cashpilot.xml
From CodeRabbit on this PR, and correct. I wrote 'overridable via CASHPILOT_ENCRYPTION_KEY', but app/database.py keeps the stored key and logs a warning when the variable differs, because switching keys would make every existing credential unreadable. That wording is wrong exactly where it matters most. Someone restoring a backup onto an instance that still has a stale key file would expect their value to take effect; it is ignored, and the credentials they were trying to recover stay unreadable. All three docs now state that the variable is adopted only when /data/.fernet_key is absent — which is also why setting it on a healthy instance is safe. Pinned with a test that rejects the word 'overridable' on any line naming the variable, and requires all three docs to state the precedence.
|
Fixed, and you were right about where it bites.
All three docs now state it is adopted only when that file is absent — which is also the reason setting it on a healthy instance is safe rather than dangerous. Pinned with a test that rejects "overridable" on any line naming the variable and requires all three docs to state the precedence, so the imprecision cannot come back quietly. 2478 tests, ruff clean. |
1 participant
gn6+l7t— a release could go green having published nothing.release.ymldecided what to build from two hand-maintained regexes, both drifted from the Dockerfiles they mirror.The UI regex named 13 of 27 modules, while the UI image does
COPY app/ ./app/— everything. A change to any of the other 14 (payouts,preflight,power,egress,lan_isolation,notify, …) setBUILD_UI=false→ version step skipped →new_tagempty → tag, Release and build job all skipped. Skipped steps don't fail a run, so it reported success.The worker regex omitted
egress.pyandstate_backup.py— both COPY'd into the worker image and imported byworker_apiat runtime. Withbuild_worker=falsethe pipeline retags the previous image, so the worker ships under a new version tag containing old code, andverify-tagsonly runsdocker manifest inspect, which a retag satisfies.Neither is restated now: any
app/change builds the UI, and the worker list is parsed out ofDockerfile.workerat run time. A test asserts every moduleworker_apiimports is actually in the worker image — the contract that decides whether a miss is a crash on the user's machine or a build failure here.dxi— six places named the wrong key, harmfully.They said
CASHPILOT_SECRET_KEYencrypts credentials. It signs sessions;CASHPILOT_ENCRYPTION_KEYis the Fernet key at/data/.fernet_key. Follow the old advice and you believe your credentials are portable, never back up the key file, and lose every one the first time the volume is recreated. README already said this correctly — the other six contradicted it.Fixed all six, named the key file so it can be backed up, and added the variable to the Unraid template (masked), since that's the whole configuration surface for those users.
Verification: 2476 tests, 95.13% coverage. Negative controls: narrowing the UI trigger fails 2 tests; restoring the
index.mdclaim fails 2 more.Summary by CodeRabbit
Documentation
CASHPILOT_SECRET_KEYsigns login sessions.CASHPILOT_ENCRYPTION_KEY, which encrypts stored credentials.Bug Fixes
Tests