test: pin the device ceiling's refusal, and stop asserting on source text - #282
Merged
Conversation
…text
Two halves of CashPilot-6zp, neither of which widens the ceiling -- the bead is
explicit that must not happen pre-emptively, and the premise still holds: all
six compute services ship an empty docker.image, so none can request anything.
1. THE MISSING TRIPWIRE. The existing test catches a compute service becoming
deployable WITHOUT declaring a device. Nothing caught the other order: it
declares one, and the ceiling refuses it anyway. A future GPU service will
hit exactly that, and the refusal is correct but unexpected -- so the new
test pins the 403 and its message says what to do, including that NVIDIA is
not a device at all (it needs `gpus: all` or a deploy.resources reservation,
and the worker spec has no field for either).
Plus a control that a device INSIDE the ceiling is allowed, or the refusal
test would pass on validation that refuses everything, and a canary on the
ceiling's size.
2. A SOURCE-TEXT ASSERTION, REMOVED. test_the_deploy_path_actually_forwards_it
read app/main.py as text and checked it CONTAINED
'"devices": docker_conf.get("devices") or None'
which passes against a build where that line exists but never runs -- and
its own docstring said "declared-but-ignored would make the corrected
CLAUDE.md wrong too", the very case it could not detect.
It now deploys Mysterium for real, intercepts _proxy_worker_deploy, and
asserts the spec handed to the worker carries the device. A control deploys
a service that declares none, so the assertion cannot pass on a spec that
always sends /dev/net/tun.
MUTATION-TESTED, and mutation 1 is the point: replacing the value with
'"devices": None, # line still here' keeps the old string's shape and breaks
the behaviour. The new test fails where the old one would have passed.
Widening the ceiling to admit /dev/dri fails 3 tests, including the existing
doc-sync one.
4351 passed, 95.57%.
📝 WalkthroughWalkthroughThe deployment tests now execute ChangesDeployment device validation
Estimated code review effort: 3 (Moderate) | ~20 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
🤖 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 `@tests/test_beads_batch_56.py`:
- Around line 431-434: Replace the hardcoded HONEYGAIN_EMAIL and
HONEYGAIN_PASSWORD values in the DeployRequest setup with test credentials
loaded from environment variables or the project’s documented encrypted storage
mechanism, while preserving the existing worker_id and request structure.
🪄 Autofix
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: 0dc293f8-4345-4bef-8056-dd0878991cce
📒 Files selected for processing (1)
tests/test_beads_batch_56.py
Comment on lines
+431
to
+434
| body = main_module.DeployRequest( | ||
| env={"HONEYGAIN_EMAIL": "someone@example.invalid", "HONEYGAIN_PASSWORD": "not-a-real-password"}, | ||
| worker_id=1, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Remove the hardcoded Honeygain credential values.
Line 432 places credential values in source code. Load test-only values from environment variables or the documented encrypted storage mechanism.
As per coding guidelines: “Never hardcode credentials; obtain secrets through environment variables or the documented encrypted storage mechanism.”
Proposed fix
import asyncio
+import os
from unittest.mock import AsyncMock, patch
body = main_module.DeployRequest(
- env={"HONEYGAIN_EMAIL": "someone@example.invalid", "HONEYGAIN_PASSWORD": "not-a-real-password"},
+ env={
+ "HONEYGAIN_EMAIL": os.environ["TEST_HONEYGAIN_EMAIL"],
+ "HONEYGAIN_PASSWORD": os.environ["TEST_HONEYGAIN_PASSWORD"],
+ },
worker_id=1,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| body = main_module.DeployRequest( | |
| env={"HONEYGAIN_EMAIL": "someone@example.invalid", "HONEYGAIN_PASSWORD": "not-a-real-password"}, | |
| worker_id=1, | |
| ) | |
| import asyncio | |
| import os | |
| from unittest.mock import AsyncMock, patch | |
| body = main_module.DeployRequest( | |
| env={ | |
| "HONEYGAIN_EMAIL": os.environ["TEST_HONEYGAIN_EMAIL"], | |
| "HONEYGAIN_PASSWORD": os.environ["TEST_HONEYGAIN_PASSWORD"], | |
| }, | |
| worker_id=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 `@tests/test_beads_batch_56.py` around lines 431 - 434, Replace the hardcoded
HONEYGAIN_EMAIL and HONEYGAIN_PASSWORD values in the DeployRequest setup with
test credentials loaded from environment variables or the project’s documented
encrypted storage mechanism, while preserving the existing worker_id and request
structure.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two halves of CashPilot-6zp, neither of which widens the ceiling — the bead is explicit that must not happen pre-emptively, and I verified the premise still holds: all six compute services ship an empty
docker.image, so none can request anything.1. The missing tripwire
The existing test catches a compute service becoming deployable without declaring a device. Nothing caught the other order: it declares one, and the ceiling refuses it anyway.
A future GPU service will hit exactly that. The refusal is correct but unexpected, so the new test pins the 403 and its failure message says what to do — including that NVIDIA is not a device at all: it needs
gpus: allor adeploy.resourcesreservation, and the worker spec has no field for either. That is a much bigger change than adding a string to a frozenset.Plus a control that a device inside the ceiling is allowed (or the refusal test would pass on validation that refuses everything), and a canary on the ceiling's size.
2. A source-text assertion, removed
test_the_deploy_path_actually_forwards_itreadapp/main.pyas text and checked it contained the literal line that forwardsdevices.That passes against a build where the line exists but never runs — and its own docstring said "declared-but-ignored would make the corrected CLAUDE.md wrong too", which is precisely the case it could not detect.
It now deploys Mysterium for real, intercepts
_proxy_worker_deploy, and asserts the spec handed to the worker carries the device. A control deploys a service that declares none, so the assertion cannot pass on a spec that always sends/dev/net/tun.Mutation-tested — and mutation 1 is the whole point
"devices": None)/dev/driFiled separately
CashPilot-jtg4— the source-text assertion as a class. This was the one I happened to read; a sweep forread_text()intests/followed by a substring check would find any others. That is a different job from 6zp, so I did not bundle it.4351 passed, coverage 95.57%, ruff clean. Verified locally — Actions is still in a major outage.
Summary by CodeRabbit