feat: see the fleet the way providers do — by IP, not by machine (CashPilot-5qc) - #166
Merged
Conversation
…hPilot-5qc)
Every bandwidth provider caps per IP address, not per device. Honeygain
treats a second active device on a network as "network overused"; EarnApp
documents that extra devices behind one IP share one daily cap. CashPilot's
whole fleet model encourages deploying a service to several machines, and
warned about none of this: two workers in one house are two rows to us and
one customer to the provider, so the second earns nothing.
Seeing this requires knowing about the OTHER machines, which is why a
single-host tool structurally cannot do it.
Each worker now reports its public egress IP and, where it can tell, whether
it is a hosted machine. Deploying to a worker that shares an address with one
already running the service is called out by name, the fleet can be grouped by
exit rather than by host (/api/fleet/egress-groups), and a residential-only
service on a machine that identifies itself as a VPS is no longer a polite
"check this yourself" but a verdict.
Three rules keep the warning from being wrong, because a warning that cries
wolf gets ignored and would be worse than nothing:
* An undetected IP is neither shared nor distinct. Those workers go to a
bucket that says exactly that, and produce no finding at all.
* A private address is a detection failure, not an identity. This matters
concretely: every worker in the reference fleet has a 100.64/10 tailnet
address, and grouping on it would collapse the entire fleet into one
fabricated conflict.
* An absent devices_per_ip means nobody documented it — not "unlimited". The
schema uses 0 for a verified no-limit, which is a real answer; a missing key
is not, and only 4 of 50 services declare one today.
Detection is local first: the hosting hint comes from DMI vendor strings, so
nothing is disclosed to a third party and it works offline. A bare hypervisor
(QEMU, VMware) is deliberately NOT hosting — a VM on a home server is a
residential connection and the most common deployment there is. The IP lookup
is the one outbound call made purely to learn about the user, so it is
opt-outable and endpoint-overridable, and every failure returns None rather
than a guess.
Fixes two defects found while wiring this up, both live in 1.7.0:
* /api/services/{slug}/preflight?worker_id=N returned 500. list_workers and
get_worker hand back raw rows, so containers/system_info arrive as JSON
TEXT, and the endpoint passed a str to code expecting a mapping. Every
caller that reads inside them now goes through _decoded_worker.
* /api/services/{slug}/producer-state never matched a container. It filtered
on c["service"] while heartbeats emit "slug", so container_running was
always false and every service reported "unknown" — the feature was inert.
Both had passing tests, because both fixtures hand-fed a shape production
never produces. The tests now build worker rows the way SQLite actually
returns them, and container_slug accepts either key so neither shape can
silently match zero containers again.
Verification: ruff clean, 1654 passed, coverage 94.47%.
An independent read-only review of this branch raised 22 findings. Every one acted on below was reproduced by execution first rather than taken on trust; the two rated HIGH were both real. Correctness: * not_checked dropped "egress IP type" when the ADDRESS was known, but the label is about the connection TYPE. A worker with an IP and no type produced one response that said the type was checked and, beside it, a finding saying it could not be. Gated on the type now. The test that asserted the old behaviour asserted the bug, so it is replaced by two that pin both halves. * The instance already running on the target machine was never counted against a documented devices_per_ip > 1, so a third instance under a limit of two read as "reduced" instead of "will earn nothing" — under-warning in exactly the situation the feature exists for. * assess() read the connection type from the system_info kwarg while the fleet half read worker["system_info"], so the same worker produced different verdicts depending on whether the caller passed a redundant argument. One fact, one source: system_info now defaults from the worker. * Android workers report `apps` with a boolean `running`, not containers. A phone on the home WiFi beside a server is two devices on ONE public IP — the canonical case here — and it was invisible. * Self-exclusion keyed on client_id, but `None != None` is False, so two legacy rows with a NULL client_id cancelled each other and hid a real conflict. Keys on the primary key now. Robustness of the heartbeat, which is the one thing here that could hurt: * httpx's timeout is per-operation, not a deadline — its read timeout is the maximum gap between chunks — so an endpoint dribbling bytes could hold the request open indefinitely. The heartbeat loop is serial, so that stops heartbeats, the UI marks the worker offline after 180s, and deploys for that host start failing. A diagnostic must never be able to take the control plane down: the whole attempt is now bounded by one wall-clock budget. * The response body was read unbounded before being truncated to 64 chars. It is capped while streaming now. * Failures are cached briefly, so a blackholed network costs the timeout once every few minutes rather than on every single heartbeat. * The cache stamped time from the event loop's clock, whose epoch is unspecified; a loop starting near zero makes the age negative, which is always under the TTL, pinning a stale address forever. time.monotonic(). Honesty, which is the whole point of this feature: * A custom CASHPILOT_EGRESS_IP_URL fell back to the public endpoints on any failure — quietly undoing the one choice an operator makes specifically to avoid disclosing to a third party. It is used exclusively now, and a test asserts nothing else is ever contacted. * An invalid CASHPILOT_EGRESS_IP was discarded in total silence. "192.168.1.5" is what most people would call their IP, and the only symptom was that nothing happened. It warns and falls back to the lookup. * "microsoft corporation" is removed from the hosting hints: Surface hardware and Hyper-V guests report it too, including Hyper-V on a home Windows desktop, and a false hosting verdict fires a ban warning at a user who is fine — precisely what classify_vendor's own docstring says to avoid. * IPv4-mapped addresses are unwrapped rather than returned verbatim, so a host reported as both 81.61.1.9 and ::ffff:81.61.1.9 matches itself; this also makes the private-address check correct independently of the interpreter patch version. NAT64 and 6to4 are rejected outright — both can carry a private IPv4 past an is_global check. * The schema and CLAUDE.md claimed devices_per_ip: 0 SUPPRESSES the conflict warning. It downgrades it to a shared-bandwidth note, because two instances share one connection whatever the provider permits. A contributor who wrote 0 expecting silence would have deleted the key instead, which is the worse state. Docs corrected to match the code. * The IPv6 limitation is now stated in the module and the README instead of being left for a user to discover: grouping is exact-address equality, so a native-IPv6 line is never matched. It can miss a conflict; it cannot invent one, which is the right way round. Also: the new env vars are documented in the README rather than only in the agent-facing CLAUDE.md — detection is on by default and makes an outbound call, so a user who wants it off has to be able to find the switch. And a unit test was making a REAL request to api.ipify.org; the worker tests now run against a fake that fails loudly if anything tries to reach the network. Verification: ruff clean, 1671 passed, coverage 94.59%.
📝 WalkthroughWalkthroughWorkers now report egress IP and network type. The server groups workers by egress, applies fleet-aware preflight findings, exposes egress groups through an API, and matches producer containers by normalized service slug. Documentation and tests cover configuration, limits, address handling, and heartbeat recovery. ChangesFleet egress awareness
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #166 +/- ##
==========================================
+ Coverage 94.45% 94.63% +0.18%
==========================================
Files 39 40 +1
Lines 4775 4999 +224
==========================================
+ Hits 4510 4731 +221
- Misses 265 268 +3
🚀 New features to boost your workflow:
|
… the heartbeat (CashPilot-5qc) A second review, this time of the fixes from the first round, found three HIGH issues. All three were reproduced by execution before being changed, and two of them were introduced or worsened by that first round. **The instance count under-reported, on the line above the fix that added it.** The cosmetic set() dedup of peer NAMES fed the arithmetic, so two distinct machines sharing a display name counted as one: three instances against a limit of two produced "reduced" instead of "will earn nothing". WORKER_NAME defaults to the hostname, so two Raspberry Pis, two identical NAS images, or one compose file copy-pasted twice all hit this. The set is display-only now and the count uses the machines. **A retired machine fabricated conflicts against a live one.** list_workers has no status filter, and the stale-worker purge deliberately spares ENROLLED rows — so a worker that is merely switched off keeps its row forever, carrying a last heartbeat in which every container is still "running" and its last-known egress IP. Switch off one of two machines, deploy honeygain on the survivor a month later, and the preflight said it was already running on the dead one and (with the new arithmetic) escalated that to will_earn_nothing. This module promises the opposite failure direction in its own docstring: a missed conflict, never an invented one. Peers are now the online workers; the worker being deployed to is still looked up from the full set, so one that just restarted can be assessed rather than 404ing. **The heartbeat could still be killed permanently.** The previous round bounded the egress lookup, but _send_heartbeat builds its payload OUTSIDE its own try — docker_available, the egress lookup and the network probe all run while the dict literal is evaluated — and _heartbeat_loop had no guard at all, so any exception there ended the task silently and forever. That is strictly worse than the stall it replaced: a missed cycle costs one 180s offline window and self-heals; a dead task means offline until someone restarts the container, while the service containers keep earning and nothing surfaces it. Every cycle is guarded now, with CancelledError re-raised so shutdown still works. Also, from the same review: * public_ip accepted the deprecated IPv4-compatible form, so ::192.168.1.5 was reported global and a LAN address could have become a grouping key. It also broke self-matching: a host seen as 81.61.1.9 and as ::81.61.1.9 produced two keys. Both forms unwrap now, and fec0::/10 — site-local, which Python does not call private — joins the reject list. * The Android fix was unreachable. running_slugs returned early when `containers` was absent or malformed, before `apps` was ever read, so the guarantee only held because a different module injects that key. It falls through now, and the test covers a worker with apps and no containers. * not_checked gated on the worker while the residential finding read the system_info kwarg — the same two-source divergence the previous commit claimed to have removed, mirrored. One source now, and both use normalise_network_type rather than an inline string compare. * The lookup budget was named "total" but applied per endpoint, so three endpoints meant 30s plus the 15s POST on a 60s serial cycle, and a longer list would have breached the 180s offline threshold. It is one deadline shared across every endpoint now. * The response cap was applied AFTER appending, and aiter_bytes yields DECOMPRESSED bytes, so a gzipped reply could allocate megabytes inside a single chunk. Truncated before appending. * _same() returned False for everything when a worker had neither id nor client_id, making it its own peer. The comment justifying the primary-key switch also cited a NULL client_id, which the NOT NULL schema makes impossible; it now states the real reason. Verification: ruff clean, 1716 passed, coverage 94.64%.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_egress.py (3)
385-405: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe sentinel does not fail loudly.
_detect_egress_ipcatchesExceptionaround every lookup attempt. The default_FakeStream(exc=AssertionError(...))therefore never surfaces; the lookup logs at debug level and returnsNone. Only the tests that assert onseendetect an unexpected request. Raise aBaseExceptionsubclass instead, so the sentinel escapes the blanket handler.♻️ Proposed change
+class _NetworkAccessAttempted(BaseException): + """Escapes `except Exception` in the code under test.""" + + `@pytest.fixture` def no_network(monkeypatch): """Fail loudly if anything here would really reach the internet.""" @@ - install(_FakeStream(exc=AssertionError("a test tried to reach the network"))) + install(_FakeStream(exc=_NetworkAccessAttempted("a test tried to reach the network"))) return install, seen🤖 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_egress.py` around lines 385 - 405, Update the default _FakeStream exception configured by the no_network fixture to use a BaseException subclass rather than AssertionError, so _detect_egress_ip’s Exception handler cannot suppress unexpected network access. Preserve the existing message and ensure the sentinel escapes the lookup handler and fails the test immediately.
555-577: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
TestEndpoints._callandTestARetiredMachineMustNotFabricateAConflict._preflightare the same harness.Both build JSON-TEXT worker rows and patch the same four targets. Extract one module-level helper and call it from both classes. This keeps the row-shape fixture — the thing the regression depends on — in a single place.
Also applies to: 634-655
🤖 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_egress.py` around lines 555 - 577, The test harness setup is duplicated between TestEndpoints._call and TestARetiredMachineMustNotFabricateAConflict._preflight. Extract the shared JSON-text worker-row construction and four target patches into one module-level helper, then have both methods invoke that helper while preserving their existing callback arguments and async execution behavior.
496-502: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe oversized-body test does not exercise the streaming cap.
_FakeStream.aiter_bytesyields the whole body in one chunk, so_fetch_egress_ipaccumulates all 10 MB before it checkslen(body) >= _EGRESS_MAX_BYTES. The assertion still passes, but the memory bound the test names is not verified. Make the double yield small chunks so thebreakis the reason the read stops.♻️ Proposed change
async def aiter_bytes(self): - yield self.body + for i in range(0, len(self.body) or 1, 64): + yield self.body[i : i + 64]🤖 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_egress.py` around lines 496 - 502, Update test_an_oversized_body_cannot_exhaust_memory and its _FakeStream input so the oversized payload is yielded in multiple small chunks rather than one 10 MB chunk. Ensure the stream produces enough data to cross _EGRESS_MAX_BYTES, allowing _fetch_egress_ip to stop via its streaming cap and making the test verify bounded reading.
🤖 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 `@app/egress.py`:
- Around line 62-73: Update HOSTING_VENDOR_HINTS to replace the broad "oracle"
substring with a host-specific hint such as "oracle cloud", preventing Oracle
Corporation VirtualBox DMI values from being classified as hosting. Extend
test_a_home_lab_hypervisor_is_not_hosting to cover the VirtualBox case and
verify it remains non-hosting.
In `@app/preflight.py`:
- Around line 132-133: Update the residential check in the preflight requirement
evaluation to treat a missing reqs["vps_ip"] value as false, matching the schema
default. Resolve that default before the vps_ip comparison so entries with
residential_ip enabled and no vps_ip still follow the existing hosted-worker
EARNS_NOTHING and unverified-worker CHECK_YOURSELF outcomes.
In `@tests/test_egress.py`:
- Around line 724-730: Update the test around the heartbeat task created by
worker_api._heartbeat_loop() to await the task after calling task.cancel(), and
suppress the expected CancelledError before the test exits. Preserve the
existing call-count assertion and cancellation behavior.
---
Nitpick comments:
In `@tests/test_egress.py`:
- Around line 385-405: Update the default _FakeStream exception configured by
the no_network fixture to use a BaseException subclass rather than
AssertionError, so _detect_egress_ip’s Exception handler cannot suppress
unexpected network access. Preserve the existing message and ensure the sentinel
escapes the lookup handler and fails the test immediately.
- Around line 555-577: The test harness setup is duplicated between
TestEndpoints._call and TestARetiredMachineMustNotFabricateAConflict._preflight.
Extract the shared JSON-text worker-row construction and four target patches
into one module-level helper, then have both methods invoke that helper while
preserving their existing callback arguments and async execution behavior.
- Around line 496-502: Update test_an_oversized_body_cannot_exhaust_memory and
its _FakeStream input so the oversized payload is yielded in multiple small
chunks rather than one 10 MB chunk. Ensure the stream produces enough data to
cross _EGRESS_MAX_BYTES, allowing _fetch_egress_ip to stop via its streaming cap
and making the test verify bounded reading.
🪄 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: ecad71bc-a5ab-4d29-87e7-a78f2caf278c
📒 Files selected for processing (13)
CLAUDE.mdDockerfile.workerREADME.mdapp/egress.pyapp/main.pyapp/preflight.pyapp/worker_api.pydocs/AUTOPILOT-WORKLOG.mddocs/fleet.mdservices/_schema.ymltests/test_egress.pytests/test_preflight.pytests/test_producer_state.py
Comment on lines
+62
to
+73
| HOSTING_VENDOR_HINTS = ( | ||
| "amazon ec2", | ||
| "digitalocean", | ||
| "google compute engine", | ||
| "hetzner", | ||
| "linode", | ||
| "openstack", | ||
| "oracle", | ||
| "ovh", | ||
| "scaleway", | ||
| "vultr", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any repo doc/test asserts Oracle Cloud DMI strings, and how VirtualBox is treated.
rg -n -i -C3 'oracle|virtualbox|innotek' --glob '!**/.git/**'Repository: GeiserX/CashPilot
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)egress\.py$|(^|/)preflight|test' | head -200
echo "== app/egress.py outline =="
ast-grep outline app/egress.py --view expanded || true
echo "== app/egress.py lines 1-180 =="
cat -n app/egress.py | sed -n '1,180p'
echo "== search for HOSTING_VENDOR_HINTS and container_hint helpers =="
rg -n -C6 'HOSTING_VENDOR_HINTS|hosting|DMI|DMI|sys_vendor|product_name|container_slug|preflight' --glob '*.py' .Repository: GeiserX/CashPilot
Length of output: 50373
🌐 Web query:
VirtualBox DMI sys_vendor product_name Oracle Corporation system vendor
💡 Result:
In Oracle VM VirtualBox, the DMI (Desktop Management Interface) system information, including fields like System Vendor and Product Name, is provided by the virtual machine's emulated BIOS/UEFI firmware to the guest operating system [1][2]. By default, these fields typically report Oracle Corporation as the manufacturer and VirtualBox as the product [3]. You can modify these and other DMI values for a specific virtual machine using the VBoxManage command-line tool [1][2]. Commands for Modifying DMI Information To change DMI information, use the following syntax for a VM [1][2]: VBoxManage setextradata "VM-name" "VBoxInternal/Devices/pcbios/0/Config/DmiSystemVendor" "Your Vendor" VBoxManage setextradata "VM-name" "VBoxInternal/Devices/pcbios/0/Config/DmiSystemProduct" "Your Product" Important Considerations: 1. Firmware Type: If your VM is configured to use EFI firmware instead of the legacy BIOS (pcbios), you must replace pcbios with efi in the configuration key path (e.g., VBoxInternal/Devices/efi/0/Config/DmiSystemVendor) [2][4]. 2. String Prefixing: Some DMI parameters are expected to be strings. If you provide a value that looks like a number, the VM may fail to start with a VERR_CFGM_NOT_STRING error [2][4]. In such cases, prepend the value with "String:" (case-sensitive, e.g., "String:1234") to force it to be treated as a string [2][5]. 3. Resetting Values: To reset a field to its default VirtualBox value, remove the custom setting by running the VBoxManage setextradata command for that key without providing a value [6]. 4. Purpose: These modifications are often used to prevent guest operating systems (particularly Windows) from detecting that they are running in a virtual machine, which can help avoid reactivation requests or allow software that checks for specific hardware manufacturers to run [1][2][7]. You can verify the current DMI information within the guest system using tools such as dmidecode on Linux [1][2].
Citations:
- 1: https://docs.oracle.com/en/virtualization/virtualbox/6.0/admin/changedmi.html
- 2: https://www.virtualbox.org/manual/ch09.html
- 3: https://www.virtualbox.org/ticket/22101
- 4: https://documentation.help/VirtualBox/ch09s12.html
- 5: https://superuser.com/questions/1762661/virtualbox-7-how-to-change-motherboard-serial-number-so-windows-10-will-recogn
- 6: https://forums.virtualbox.org/viewtopic.php?t=63469
- 7: https://danaukes.com/notebook/virtualbox/modify-vm-parameters
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
app = Path("app/egress.py")
tree = ast.parse(app.read_text())
hints = next(
(node.value.values[0].elts for node in ast.walk(tree)
if isinstance(node, ast.Assign) and any(a.id == "HOSTING_VENDOR_HINTS" for a in node.targets)),
None,
)
if hints is None:
raise SystemExit("HOSTING_VENDOR_HINTS not found")
hints = [s.value for s in hints]
tests = Path("tests/test_egress.py").read_text()
print("hints=", hints)
print("oracle_hint=", "oracle")
print("contains_oracle_hint=", "oracle" in hints)
print("contains_oracle_cloud_hint=", "oracle cloud" in hints)
for vendor in ["Oracle Corporation", "Oracle VM VirtualBox", "VirtualBox", "Innotek GmbH"]:
print(vendor, "===>", any(h in vendor.lower() for h in hints))
print("oracle_nonhosting_test_present=", '"Oracle Corporation"' in tests or "'Oracle Corporation'" in tests)
print("oracle_virtualbox_test_present=", (
''.join(['"Oracle VM VirtualBox"' in tests, ''.join(["'Oracle VM VirtualBox'" in tests])))
)
PYRepository: GeiserX/CashPilot
Length of output: 446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
app = Path("app/egress.py")
tree = ast.parse(app.read_text())
hints = next(
(node.value.values[0].elts for node in ast.walk(tree)
if isinstance(node, ast.Assign) and any(a.id == "HOSTING_VENDOR_HINTS" for a in node.targets)),
None,
)
if hints is None:
raise SystemExit("HOSTING_VENDOR_HINTS not found")
hints = [s.value for s in hints]
tests = Path("tests/test_egress.py").read_text()
print("hints=", hints)
print("oracle_hint=", "oracle")
print("contains_oracle_hint=", "oracle" in hints)
print("contains_oracle_cloud_hint=", "oracle cloud" in hints)
for vendor in ["Oracle Corporation", "Oracle VM VirtualBox", "VirtualBox", "Innotek GmbH"]:
print(vendor, "===>", any(h in vendor.lower() for h in hints))
print("oracle_nonhosting_test_present=", '"Oracle Corporation"' in tests or "'Oracle Corporation'" in tests)
print("oracle_virtualbox_test_present =", '"Oracle VM VirtualBox"' in tests or "'Oracle VM VirtualBox'" in tests)
PYRepository: GeiserX/CashPilot
Length of output: 324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
app = Path("app/egress.py")
tree = ast.parse(app.read_text())
node = next(
node
for node in ast.walk(tree)
if isinstance(node, ast.Assign) and any(isinstance(a, ast.Name) and a.id == "HOSTING_VENDOR_HINTS" for a in node.targets)
)
hints = [elt.value for elt in node.value.elts]
tests = Path("tests/test_egress.py").read_text()
print("hints=", hints)
print("contains_oracle_hint=", "oracle" in hints)
print("contains_oracle_cloud_hint=", "oracle cloud" in hints)
for vendor in ["Oracle Corporation", "Oracle VM VirtualBox", "VirtualBox", "Innotek GmbH"]:
print(vendor, "===>", any(h in vendor.lower() for h in hints))
print("oracle_nonhosting_test_present=", '"Oracle Corporation"' in tests or "'Oracle Corporation'" in tests)
print("oracle_virtualbox_test_present =", '"Oracle VM VirtualBox"' in tests or "'Oracle VM VirtualBox'" in tests)
print("oracle_cloud_hosting_test_present =", '"Oracle Cloud"', "'Oracle Cloud'" in tests)
PYRepository: GeiserX/CashPilot
Length of output: 591
Narrow the Oracle hint to avoid flagging VirtualBox guests.
HOSTING_VENDOR_HINTS includes "oracle", and classify_vendor() matches hints by substring. Oracle VM VirtualBox DMI reports Oracle Corporation, so that DMI returns hosting and preflight.assess() emits the residential-only ban warning. Use a host-specific string like "oracle cloud" and add the VirtualBox case to test_a_home_lab_hypervisor_is_not_hosting.
🤖 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 `@app/egress.py` around lines 62 - 73, Update HOSTING_VENDOR_HINTS to replace
the broad "oracle" substring with a host-specific hint such as "oracle cloud",
preventing Oracle Corporation VirtualBox DMI values from being classified as
hosting. Extend test_a_home_lab_hypervisor_is_not_hosting to cover the
VirtualBox case and verify it remains non-hosting.
Comment on lines
132
to
+133
| if reqs.get("residential_ip") and reqs.get("vps_ip") is False: | ||
| findings.append( | ||
| { | ||
| "verdict": CHECK_YOURSELF, | ||
| "message": ( | ||
| "This needs a residential IP. On a VPS or datacentre connection it typically " | ||
| "earns far less, or the account is banned outright. CashPilot cannot check " | ||
| "your connection type, so this one is on you." | ||
| ), | ||
| } | ||
| ) | ||
| verdicts.append(CHECK_YOURSELF) | ||
| if egress.normalise_network_type(info.get("egress_network_type")) == egress.HOSTING: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How many catalog entries declare residential_ip without vps_ip?
fd -e yml -e yaml . services --exec sh -c '
if grep -q "residential_ip:[[:space:]]*true" "$1" && ! grep -q "vps_ip:" "$1"; then
echo "MISSING vps_ip: $1"
fi
' _ {}Repository: GeiserX/CashPilot
Length of output: 1300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== preflight outline =="
ast-grep outline app/preflight.py --match Preflight --view expanded || true
echo
echo "== preflight relevant section =="
sed -n '1,230p' app/preflight.py | cat -n
echo
echo "== schema around vps/residential =="
fd -e yml -e yaml _schema.yml services | xargs -r cat -n | sed -n '70,105p'
echo
echo "== current diff/stat =="
git diff --stat
git diff -- app/preflight.py -- services/_schema.yml | sed -n '1,260p'Repository: GeiserX/CashPilot
Length of output: 14373
Apply the vps_ip schema default in the residential check.
residential_ip: true with no vps_ip means vps_ip: false, but reqs.get("vps_ip") is False skips missing keys. Entries omitting vps_ip currently produce no residential IP finding, so hosted workers lose the EARNS_NOTHING verdict and unverified workers lose the CHECK_YOURSELF note. Resolve the default before reading it.
🐛 Proposed fix
- if reqs.get("residential_ip") and reqs.get("vps_ip") is False:
+ residential_required = bool(reqs.get("residential_ip"))
+ # Schema: vps_ip defaults to the opposite of residential_ip.
+ vps_allowed = reqs.get("vps_ip", not residential_required)
+ if residential_required and vps_allowed is False:📝 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
| if reqs.get("residential_ip") and reqs.get("vps_ip") is False: | |
| findings.append( | |
| { | |
| "verdict": CHECK_YOURSELF, | |
| "message": ( | |
| "This needs a residential IP. On a VPS or datacentre connection it typically " | |
| "earns far less, or the account is banned outright. CashPilot cannot check " | |
| "your connection type, so this one is on you." | |
| ), | |
| } | |
| ) | |
| verdicts.append(CHECK_YOURSELF) | |
| if egress.normalise_network_type(info.get("egress_network_type")) == egress.HOSTING: | |
| residential_required = bool(reqs.get("residential_ip")) | |
| # Schema: vps_ip defaults to the opposite of residential_ip. | |
| vps_allowed = reqs.get("vps_ip", not residential_required) | |
| if residential_required and vps_allowed is False: |
🤖 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 `@app/preflight.py` around lines 132 - 133, Update the residential check in the
preflight requirement evaluation to treat a missing reqs["vps_ip"] value as
false, matching the schema default. Resolve that default before the vps_ip
comparison so entries with residential_ip enabled and no vps_ip still follow the
existing hosted-worker EARNS_NOTHING and unverified-worker CHECK_YOURSELF
outcomes.
Comment on lines
+724
to
+730
| task = asyncio.create_task(worker_api._heartbeat_loop()) | ||
| for _ in range(20): | ||
| await asyncio.sleep(0) | ||
| if len(calls) >= 3: | ||
| break | ||
| task.cancel() | ||
| assert len(calls) >= 3, "the loop stopped after the first exception" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Await the cancelled task before the test ends.
task.cancel() only requests cancellation. The test returns without giving the loop a chance to process it, so the task can be garbage-collected while pending and asyncio logs "Task was destroyed but it is pending". Await it and suppress CancelledError.
🧹 Proposed fix
+ import contextlib
+
task = asyncio.create_task(worker_api._heartbeat_loop())
for _ in range(20):
await asyncio.sleep(0)
if len(calls) >= 3:
break
task.cancel()
+ with contextlib.suppress(asyncio.CancelledError):
+ await task
assert len(calls) >= 3, "the loop stopped after the first exception"📝 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
| task = asyncio.create_task(worker_api._heartbeat_loop()) | |
| for _ in range(20): | |
| await asyncio.sleep(0) | |
| if len(calls) >= 3: | |
| break | |
| task.cancel() | |
| assert len(calls) >= 3, "the loop stopped after the first exception" | |
| import contextlib | |
| task = asyncio.create_task(worker_api._heartbeat_loop()) | |
| for _ in range(20): | |
| await asyncio.sleep(0) | |
| if len(calls) >= 3: | |
| break | |
| task.cancel() | |
| with contextlib.suppress(asyncio.CancelledError): | |
| await task | |
| assert len(calls) >= 3, "the loop stopped after the first exception" |
🤖 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_egress.py` around lines 724 - 730, Update the test around the
heartbeat task created by worker_api._heartbeat_loop() to await the task after
calling task.cancel(), and suppress the expected CancelledError before the test
exits. Preserve the existing call-count assertion and cancellation behavior.
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.
Every bandwidth provider caps per IP address, not per device. Honeygain treats a second active device on a network as "network overused"; EarnApp documents that extra devices behind one IP share a single daily cap.
CashPilot's whole fleet model encourages deploying a service to several machines, and warned about none of it. Two workers in one house are two rows on the dashboard and one customer to the provider — so the second earns nothing.
Seeing this requires knowing about the other machines, which is why a single-host tool structurally cannot do it.
What lands
GET /api/fleet/egress-groupsgroups the fleet by exit rather than by host.Three rules that keep the warning from being wrong
A warning that cries wolf gets switched off, so:
100.64/10tailnet address — grouping on it would collapse the entire fleet into one fabricated conflict.devices_per_ipmeans undocumented, not unlimited.0is a verified no-limit; a missing key is not. Only 4 of 50 services declare one.Detection is local-first: the hosting hint comes from DMI vendor strings, so nothing is disclosed to a third party and it works offline. A bare hypervisor (QEMU, VMware) is deliberately not hosting — a VM on a home server is a residential connection.
Two defects fixed, one confirmed live in 1.7.0
/api/services/{slug}/preflight?worker_id=Nreturned 500. Confirmed against the deployed UI on real worker rows:AttributeError: 'str' object has no attribute 'get'.list_workersreturns JSON text columns; the endpoint treated them as dicts./api/services/{slug}/producer-statenever matched a container — it filtered onc["service"]while heartbeats emitslug, so every service reported UNKNOWN. Merged after 1.7.0, so it never reached production.Both had passing tests, because both fixtures hand-fed a shape production never produces. Tests now build rows the way SQLite actually returns them.
Review
An independent review raised 22 findings; each was reproduced by execution before being acted on. The serious ones: a stalled IP lookup could stall the serial heartbeat loop and take a worker offline (httpx's timeout is per-operation, not a deadline), and
not_checkedclaimed the connection type was checked while a finding beside it said it couldn't be. Also fixed: an off-by-one that under-warned, Android workers invisible to the check, a custom IP endpoint silently falling back to third parties, and a unit test making a real network request.Verification
ruff check .+ruff format --check .cleanpytest --cov=app --cov-fail-under=90→ 1704 passed, coverage 94.60%ASUSTeK COMPUTER INC.and correctly classify as not hosting; the DMI path is readable from inside the worker container; the worker image module set was simulated to proveapp/egress.pyadds no import the worker lacks.Detection is on by default and makes one outbound call per hour, so it is documented in the README (not just
CLAUDE.md) withCASHPILOT_EGRESS_DETECT=offand two ways to avoid the third party entirely.Summary by CodeRabbit