Visitar URL original
perf+security+test: deferred audit follow-ups (health growth, first-run token, orchestrator coverage) by GeiserX · Pull Request #100 · GeiserX/CashPilot · GitHub
Skip to content

perf+security+test: deferred audit follow-ups (health growth, first-run token, orchestrator coverage) - #100

Merged
GeiserX merged 5 commits into
mainfrom
feat/web-deferred-followups
Jul 10, 2026
Merged

GeiserX merged 5 commits into
mainfrom
feat/web-deferred-followups

Conversation

@GeiserX

@GeiserX GeiserX commented Jul 10, 2026 •

Copy link
Copy Markdown
Owner

Implements three items deferred in #98, each independently verified (ruff check + ruff format --check + full pytest 1075 passed). (Re-opened as a fresh PR — the original #99 auto-closed when its stacked base branch was deleted on merge; identical content, now rebased directly on main.)

1. Bound health_events growth ([PERF])

check_ok/check_down uptime samples (one per service every 5 min) dominate the table, but get_health_scores only reads a bounded window (/api/health/scores caps at 90 days) while retention kept everything 400 days.

  • Trim samples to HEALTH_CHECK_RETENTION_DAYS=95 (just past the 90-day max query, so no allowed query can out-range its own samples — uptime% unchanged) while keeping rare lifecycle events (start/stop/restart/crash) the full 400 days.
  • Add a weekly VACUUM job (commit-first, wal_checkpoint(TRUNCATE)) — SQLite never shrinks the file on DELETE alone.

2. First-run setup token + trusted-proxy client IP ([SEC])

The "private network only" guard on first-run /register trusts request.client.host, which behind a reverse proxy is the proxy (loopback/private) — so it always passes and the first public visitor could seize the owner account.

  • While no users exist, startup generates a one-time setup token, persists it (encrypted config), and logs it; /register now requires it (query param / X-Setup-Token header / form field) and it is retired the instant the owner is created.
  • Resolve the real client IP behind a trusted proxy (opt-in CASHPILOT_TRUSTED_PROXY, right-most XFF) — used by the first-run network check and the login rate limiter, which previously collapsed every proxied client into one bucket keyed by the proxy IP.

3. orchestrator/worker_api coverage ([TEST])

Two lowest-covered, most Docker-privileged modules: orchestrator.py 38%→73%, worker_api.py 44%→74% (61 tests, Docker SDK fully mocked). Plus a _login_attempts/setup-token autouse reset fixture removing latent order-dependent flake.

Verified: ruff (check + format) clean; full pytest 1075 passed.

Summary by CodeRabbit

  • Security

    • Added first-run setup-token protection for creating the initial account.
    • Restricted first-run access to private or loopback networks.
    • Improved client IP detection when operating behind a trusted proxy.
    • Setup tokens are cleared after successful initial registration.
  • Maintenance

    • Added a weekly database maintenance task.
  • Reliability

    • Expanded automated coverage for authentication, deployment, container management, and setup-token behavior.

GeiserX added 3 commits July 10, 2026 18:42
health_events is dominated by check_ok/check_down uptime samples (one per
service every 5 minutes), yet get_health_scores only reads a bounded window
(/api/health/scores caps it at 90 days) while retention kept everything 400
days. Trim samples to HEALTH_CHECK_RETENTION_DAYS=95 (just past the max query
window, so no allowed query can out-range its own samples) while keeping the
rare lifecycle events (start/stop/restart/crash) the full 400 days. Add a weekly
VACUUM job (commit-first, WAL-checkpoint TRUNCATE) since SQLite never shrinks the
file on DELETE alone — without it the DB keeps its high-water-mark size forever.

Uptime% is unchanged for every allowed query. Tests cover the split-retention
boundary + a clean VACUUM run.
Add 61 tests (Docker SDK fully mocked) for the two lowest-covered, most
privileged modules: orchestrator.py 38%->73%, worker_api.py 44%->74%. Covers
_collect_stats (zero-delta CPU, missing keys, APIError), _find_container name/
label/ValueError fallback, get_status[_light] docker-unavailable + corrupted-
container-skip paths, _verify_api_key negative paths, _validate_deploy_spec
rejections (privileged, cap_add, network_mode, blocked volume roots incl.
subpaths; host/bridge + named volumes allowed), and every command endpoint's
success + ValueError->404 / RuntimeError->503 branches.

Also update the lifespan-scheduler test to expect the new db_vacuum interval
job (added in the previous commit).
The "private network only" guard on first-run /register trusts
request.client.host, which behind a reverse proxy is the proxy (a loopback/
private address) — so the check always passes and the first public visitor could
seize the owner account. Add a proxy-independent gate: while no users exist,
startup generates a one-time setup token, persists it (encrypted config), and
logs it; /register then requires it (query param, X-Setup-Token header, or form
field) and it is retired the moment the owner account is created.

Also resolve the real client IP behind a trusted proxy (opt-in
CASHPILOT_TRUSTED_PROXY, right-most X-Forwarded-For), used by both the first-run
network check and the login rate limiter — which previously collapsed every
proxied client into one bucket keyed by the proxy IP.

Tests: token module + client_ip (trusted/untrusted) + the first-run guard
(missing/wrong/query/header/form token, public-IP-still-blocked, network-only
fallback once retired) + register integration (403 without token, 303 + token
cleared with it); conftest resets the token global between tests.
@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@GeiserX, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b9bf2868-ae22-4b4e-a710-0bc8d6e9593e

📥 Commits

Reviewing files that changed from the base of the PR and between 1812260 and f78c323.

📒 Files selected for processing (8)
  • app/database.py
  • app/deps.py
  • app/main.py
  • app/routers/auth.py
  • app/templates/auth.html
  • tests/test_database.py
  • tests/test_main_routes.py
  • tests/test_setup_token.py
📝 Walkthrough

Walkthrough

Changes

First-run access and lifecycle

Layer / File(s) Summary
Setup-token lifecycle
app/setup_token.py
Adds URL-safe token generation, activation, clearing, retrieval, and constant-time verification.
Client-IP and access guard
app/deps.py, app/main.py
Derives client IPs with optional trusted-proxy support and enforces private-network plus setup-token access.
Startup and maintenance scheduling
app/main.py
Initializes first-run tokens during startup and schedules weekly database vacuuming.
Registration integration
app/routers/auth.py, app/templates/auth.html
Passes setup tokens through first-user registration and clears persisted and active token state after owner creation.
Registration and lifecycle validation
tests/conftest.py, tests/test_main_routes.py, tests/test_setup_token.py
Covers token gating, client-IP derivation, cleanup, and scheduler wiring.

API and orchestrator coverage

Layer / File(s) Summary
Orchestrator branch coverage
tests/test_orchestrator_coverage.py
Covers Docker statistics, container lookup, status reporting, deduplication, and failure paths.
Worker API coverage
tests/test_worker_api_coverage.py
Covers authentication, deployment validation, container commands, logs, removal, and error mappings.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

  • GeiserX/CashPilot#46: Updates first-run registration access with private or loopback network restrictions.
  • GeiserX/CashPilot#85: Provides the earlier private-network registration guard extended here with client-IP and setup-token handling.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.55% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the three main follow-up areas: retention/perf, first-run token security, and orchestrator test coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/web-deferred-followups

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 10, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.61%. Comparing base (3e10aa4) to head (f78c323).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
+ Coverage   92.46%   92.61%   +0.14%     
==========================================
  Files          30       31       +1     
  Lines        3118     3180      +62     
==========================================
+ Hits         2883     2945      +62     
  Misses        235      235              
Files with missing lines Coverage Δ
app/database.py 87.35% <100.00%> (+0.32%) ⬆️
app/deps.py 100.00% <100.00%> (ø)
app/main.py 92.89% <100.00%> (+0.09%) ⬆️
app/routers/auth.py 100.00% <100.00%> (ø)
app/setup_token.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@app/deps.py`:
- Around line 97-112: The first-run setup flow accepts the sensitive token
through a query parameter, exposing it in access logs and browser history.
Update `_require_first_run_access` and its startup/help messaging to remove or
de-emphasize `request.query_params.get("setup_token")`, directing operators to
the `X-Setup-Token` header or `setup_token` form field instead; ensure no
generated URL or log advertises the token in a query string.
🪄 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

Run ID: 48045f47-2d6f-4e34-a724-0277adcad7a9

📥 Commits

Reviewing files that changed from the base of the PR and between 3e10aa4 and 1812260.

📒 Files selected for processing (10)
  • app/deps.py
  • app/main.py
  • app/routers/auth.py
  • app/setup_token.py
  • app/templates/auth.html
  • tests/conftest.py
  • tests/test_main_routes.py
  • tests/test_orchestrator_coverage.py
  • tests/test_setup_token.py
  • tests/test_worker_api_coverage.py

Comment thread app/deps.py Resolved
GeiserX added 2 commits July 10, 2026 18:56
…r _run_vacuum + first-run token

A messy multi-rebase during PR stacking silently dropped the database.py half of
the health_events change (HEALTH_CHECK_RETENTION_DAYS, the check-sample purge, and
vacuum_database) while keeping main.py's _run_vacuum + scheduler job — leaving
main calling a database function that no longer existed. Restore the database.py
logic and its tests, and add coverage for main._run_vacuum (success + error) and
the lifespan first-run setup-token generation (the codecov/patch gaps).

Full pytest 1081 passed; ruff check + format clean.
…odeRabbit)

A ?setup_token= URL leaks the secret into reverse-proxy access logs and browser
history. Drop query-param support in _require_first_run_access (form field +
X-Setup-Token header only); the GET /register page now renders behind just the
network check so the operator can reach it, the token is a visible form field
they type from the logs, and the startup log no longer embeds the token in a URL.
@GeiserX

GeiserX commented Jul 10, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

The query-param setup-token finding is addressed in f78c323: _require_first_run_access no longer reads request.query_params, the token is a form field / X-Setup-Token header only, the GET /register page renders behind just the network check, and the startup log no longer embeds the token in a URL.

@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
cd /tmp
gh pr view 100 --repo GeiserX/CashPilot --json commits --jq '.commits[] | select(.oid | startswith("f78c323")) | .oid' || true
echo "---"
gh api repos/GeiserX/CashPilot/commits/f78c323 --jq '.files[].filename' || true

Length of output: 533


🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "=== deps.py ==="
cat app/deps.py
echo "=== auth.py ==="
cat app/routers/auth.py
echo "=== main.py (relevant parts) ==="
grep -n "setup_token\|_setup_token\|client_ip" app/main.py

Length of output: 13220


🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '300,345p' app/main.py

Length of output: 2326


@GeiserX, confirmed. Commit f78c323 addresses the finding correctly:

  • _require_first_run_access only checks setup_token_value (form field) or the X-Setup-Token header, no request.query_params read.
  • GET /register gates only via main._require_private_network(request) — token isn't required/exposed on that path.
  • Startup log at Line 333 in app/main.py prints the token as plain text, not embedded in a URL.
  • tests/test_setup_token.py covers the token/header/network enforcement paths.

Nice fix. I'll trigger a full review of the changes.

ヾ(⌐■_■)ノ♪

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@GeiserX
GeiserX merged commit 8595a89 into main Jul 10, 2026
7 checks passed
@GeiserX
GeiserX deleted the feat/web-deferred-followups branch July 10, 2026 17:14
GeiserX added a commit that referenced this pull request Aug 3, 2026
The onboarding page — where a fresh install redirects you — had no
field for the one-time setup token, and never sent one. /register
requires it on first run, so every new installation answered the form
with a 403 and the message "Registration failed. Please try again."
Trying again did the same thing. Forever.

This is not a regression from this branch. v1.10.1 ships it: `git show
v1.10.1:app/templates/onboarding.html` contains no setup_token at all.
The gate landed in PR #100 and the page it gates was never updated. It
survived because existing installs were unaffected — the people who
could have noticed already had accounts, and every test in the suite
creates its owner by calling database.create_user directly, so the
actual install flow had never once been exercised.

Found by the user-story audit asking the obvious question nobody had
asked: can a new user actually install this?

Fixed: the field, the value in the POST, and the message. A 403 here
has exactly one cause, so it now says so and points at
`docker logs cashpilot-ui` and the log line to look for, instead of
suggesting a retry that cannot work.

Verified by driving a genuinely fresh instance in a browser — land on
/, follow the redirect, fill the form, press the button — and then
asking the SERVER whether the account exists rather than trusting the
page. Removing the token from the POST turns that check red with a 403.
The harness is committed, since the suite could not see this and would
not have.

The pytest additions carry their own correction: the first version of
the gate test passed a wrong token and was told "allowed", which looked
like a security hole. It was an unarmed fixture — conftest clears the
token global before every test and verify() returns True when no token
is active. Now armed, with a positive case so the two negative ones
cannot pass with the gate welded shut.
GeiserX added a commit that referenced this pull request Aug 4, 2026
…ard the next one (#181)

* feat: somewhere to answer "did you get paid?"

The backend has been detecting payouts for a release and had nowhere to
ask. A balance drop is recorded as a PROBABLE payout, and until someone
answers it never counts toward lifetime earnings — so a real payout
looked exactly like a loss, permanently. The endpoints to confirm or
reject one existed, were tested, and were called by nothing.

A dashboard card, hidden until there is something to answer, because an
empty card every day trains people to ignore the one day it matters.
Confirm is one click; reject asks first, since it is a hard DELETE with
no undo. A failed fetch leaves the card alone rather than hiding it —
unknown is not "nothing pending", and hiding it would silently drop a
question the user still owes an answer to.

Verified in a real browser over CDP, not by reading: the card renders,
both buttons carry data-action with zero inline handlers, CP resolves
the handlers, clicking Confirm actually calls the API and the row
disappears, and no CSP violation or JS error fires.

That browser check earned its keep immediately. A 24.90 USD payout
rendered as "£18.55", because the dashboard's display currency was
applied to it. Everywhere else that is correct; here it is a converted
approximation of one specific real transaction that the user is about
to check against the provider's own page. The native amount now leads
and the conversion follows, marked approximate and suppressed when the
two are identical.

The tests carry their own correction: the first version sliced a fixed
number of characters from app.js and read on into the next function, so
it reported that confirmPayout asks for confirmation when what it had
actually found was rejectPayout. Bounded on the next function instead.

* feat: show how far off the payout is, on the service that earns it

The second orphaned endpoint. payout-progress answers three questions
that used to be one number which went DOWN when the user got paid:
what is sitting there now, what this service has earned in total, and
when it can be cashed out. The backend has answered all three since
1.10.0 and nothing ever asked.

It nearly went into app/templates/service_detail.html — which turns out
to be a DEAD template. No route renders it and nothing references it;
the real detail view is a modal built by renderServiceDetail. A card
added there would have passed every string test and never once appeared
on screen. Found by driving the page rather than reading it. There is
now a test asserting that template stays unrendered, which should fail
and be deleted the day someone wires it up.

Two things the browser caught that no string test would have:

The card first rendered "BALANCE NOW £3.73" directly above "to the 20
minimum" — two halves of one comparison in different units, with a
progress bar agreeing with neither. Everything in the card is now
stated in the cashout currency the provider's own minimum is declared
in, which is the only unit that comparison makes sense in. This is the
opposite call to the payout queue, and for the same reason: there, the
figure is a specific transaction to check against a provider's page;
here, it is a value being compared to a threshold.

The bar is derived from `remaining`, because the API returns no
`threshold` key — checked against payouts.project() rather than
assumed, which is how the first version came to reference one. A
service with no documented minimum omits `remaining` entirely, and that
absence is what suppresses the bar: stuck at zero it would say the
wrong thing far more loudly than no bar at all.

Verified over CDP: card visible, bar at 25% for 5.00 of a 20 minimum,
"About 15 day(s) to the 20 minimum, at 1.0000 a day", no CSP violation,
no JS error, zero inline handlers.

* fix: never label an unconverted amount as the display currency

CodeRabbit flagged the payout queue printing a duplicate amount. Chasing
it in a browser turned up a larger defect underneath, in code every
figure on the dashboard goes through.

formatCurrency left the amount in USD when the display currency had no
rate, and then formatted it AS that currency — so $24.90 rendered as
"£24.90". Not a conversion: the same number wearing a different sign.
Rates are fetched asynchronously, so this hit EVERY figure on the page
until they arrived, and hit permanently for any currency with no rate.
The label now follows the conversion that actually happened; with no
rate the value stays in USD and says USD.

The duplicate itself was the reported symptom. It compared formatted
strings — "$24.90" against "24.90 USD" — which differ even when nothing
was converted. Comparing against _displayCurrency was still wrong for
the no-rate case, so the decision now asks which currency the formatter
will ACTUALLY use, including an unpriced token, which stays in its own
units.

My test for that was worse than the bug: it asserted the literal source
text `converted !== native`, so it passed while that very comparison
produced the duplication it claimed to prevent. Asserting the
implementation is not asserting the behaviour. Replaced, and the real
proof is scripts/currency_check.mjs, which runs the actual functions
against controlled rate state and covers five cases including both
no-rate paths.

Also from the same review: confirm and reject require writer access on
the backend, and the queue showed the buttons to everyone, so a viewer
could only ever click into a 403. Gated on _canWrite, following the
idiom the deploy button already uses, with a line saying why rather
than an unexplained empty row.

* feat: warn before a credential dies, not after collection stops

Third orphaned endpoint. Several providers issue session cookies
measured in hours — Bytelixir's is about two. When one dies, collection
stops and NOTHING says so: the dashboard keeps showing the last balance
it managed to read, which looks exactly like a service that is simply
not earning. The endpoint that computes this shipped in 1.10.x and was
called by nothing, so the warning existed and nobody ever saw it.

On Settings rather than the dashboard, and always visible there when
anything is configured. That is the opposite call to the payout card,
deliberately: the dashboard is glanceable and a permanent card becomes
furniture, while Settings is somewhere you go on purpose.

Worst first, because the only rows worth acting on are the bad ones.
Where the provider offers a longer-lived credential the row says so —
re-pasting a two-hour cookie restarts the cycle, setting the durable
one ends it, and that is the actual fix rather than a nag.

Verified over CDP against a credential aged past its lifetime: three
rows, "Likely expired" first, the reason and the durable alternative
both shown, Update offered only on the actionable row and only to an
owner, no CSP violation, no JS error — and the seeded credential value
never appears anywhere in the page, which the check asserts explicitly.

Also in this iteration: the daily-chart currency item from the 1.11.0
list is a FALSE ALARM, and re-verifying it rather than fixing it was
the right call. The dataset holds raw USD, but the y-axis ticks and the
tooltip both format through the same converter, so bars and ticks share
one space and the reading is consistent. That consistency is one line
from breaking in a way that would silently misplace every bar against
its own axis, so it now has a test.

* refactor: move service-specific knowledge out of app/ and into the catalog

The repo's own rule is "never hardcode service-specific logic in app/",
and it had drifted twice.

Thirteen credential hints — prose telling a user where to find a token
in a provider's own UI — lived in a dict inside api_collectors_meta,
out of reach of anyone editing the service they describe. They now live
in collector.credential_hint in each service's YAML. Extracted by AST
and verified byte-identical through the endpoint afterwards: 13 of 13,
none altered, none invented.

Given a home rather than reusing collector.notes, which the schema
documents as implementation hints for developers. That is a different
audience: notes explain the API to whoever writes the collector, the
hint is what someone reads while looking at a provider dashboard they
have never seen before. Conflating them would have made both worse.

api_per_node_earnings branched on `slug == "mysterium"` and imported
that collector class by name — two pieces of service knowledge in a
route handler, so a second service reporting per-node figures meant
editing the handler. It now reads collector.per_node_earnings from the
catalog and builds through the existing factory. A service that
declares the capability without implementing it logs and returns empty
rather than 500ing.

The rule now has a test, because a rule without one is a preference: an
AST scan for catalogued slugs appearing as literals anywhere in app/
outside app/collectors, which is exempt by design. The one remaining
match is allowlisted with its reason — "mysterium" in exchange_rates is
a CoinGecko coin id, not a service reference — and a further test fails
if that exemption ever goes stale.

The guard checks CODE, not text. The first version failed on the
comment explaining the branch it had removed, which is the third time
this session a text-matching check has fired on its own explanation. A
guard that reads comments punishes documenting the fix.

* fix: connect the guard that was never wired, delete the constant that was

Two half-alive things in machine_economics, decided rather than left.

per_service_is_meaningful() was called only by its own tests. That
looked like dead code, but the one path that needed it was doing
without: api_earnings_net attributes electricity per service, and a
bandwidth container adds roughly 1-3 W to a host that is already on —
below what a consumer smart plug can resolve. The module has held that
rule since it was written and nothing had ever asked it. So this is a
guard that was never connected, not code to remove.

Connected as a per-row `cost_attributable` flag rather than by dropping
the figure. The host really does draw the power; what is unreliable is
which service to blame. Nulling the cost would have understated the
fleet total, trading a precision problem for an accuracy one — so the
cost still reports and still counts, and only the attribution is
doubted. Verified: 2 W flags false, 120 W flags true, and the total
still counts both. The flag defaults to True, so a caller that does not
pass it is unaffected.

DEFAULT_IDLE_WATTS genuinely was dead, and worse than dead: 65.0 with
the same meaning as power.DEFAULT_HOST_TDP_WATTS, which is the same
number and has a real consumer. Two constants for one quantity in two
modules drift apart silently and force the next reader to guess which
is authoritative. Deleted, with the reason left where it stood so the
next person does not re-add it.

* feat: show what the fleet costs to run, without inventing the parts we cannot know

Fourth and fifth orphaned endpoints. fleet/economics answers "is this
machine worth leaving on" and earnings/net answers it per service; both
shipped in 1.10.x and were called by nothing.

Rendered on the fleet page, hidden until there is a tariff to answer
with, because "unknown" repeated on every visit is noise rather than
information.

The endpoints already return a finished sentence for every branch —
"not billed by the kilowatt-hour", "without this machine's power draw
CashPilot cannot say whether that covers the electricity, and guessing
would be worse than not saying" — so those are rendered rather than
reworded into something cheerier. The verdicts exist because the answer
is often that a machine is not worth its electricity, and softening
that would defeat the point of computing it.

An unknown cost renders as a dash, never as zero. A zero would turn
gross into apparent profit, which is precisely what app/power.py exists
to prevent, and it must not creep back in at the presentation layer
just because the API got it right.

Per-service rows appear only where the cost can honestly be attributed,
using the cost_attributable flag wired up in the previous commit. What
is withheld is COUNTED OUT LOUD — "1 service(s) draw too little on a
machine that is already on ... their cost is still counted in the
machine totals above" — because dropping the rows silently would read
as those services being free.

Verified over CDP against a metered worker with no declared wattage:
card visible, the machine's unknown-cost sentence intact, cost and net
both rendered as dashes, zero fabricated per-service rows, the
withholding explained, no CSP violation and no JS error.

* feat: say what a deploy will do, and ask before it earns nothing

The last two orphaned endpoints, and the worst place for them to have
been silent: the backend knew that a second instance behind one IP can
make a provider forfeit the account balance, and the deploy button
never mentioned it.

deploy-risk renders in the service modal — what running this does with
the connection, and whether it can be kept off the LAN. "Nobody has
documented this" is rendered as prominently as a known risk, because
undocumented is not safe; collapsing that distinction would undo the
design of lan_isolation.py, which goes out of its way to preserve it.

preflight now runs at the deploy step, which is where the backend's own
comments say it belongs rather than buried in a FAQ. It WARNS and never
blocks: the assessment reports blocking=false and says "you can deploy
it anyway", and the person at the keyboard knows things CashPilot does
not. Only findings that say the service will earn nothing interrupt —
advisory notes on every deploy would train people to click through the
one that matters. An unreachable preflight proceeds, because failing to
fetch advice is not grounds to refuse a deploy the user asked for.

Verified over CDP in three cases, which is what it takes to show a gate
works rather than merely exists:

  objection, declined : asked, confirmed, NOT deployed
  objection, accepted : asked, confirmed, deployed
  no objection        : not asked, deployed

A gate that always fires is a broken button; one that never fires is
decoration. Both halves are asserted.

The check needed two corrections of its own. It first called the deploy
function cold, which hit the "select a worker" guard and proved
nothing — so it now drives the real modal, ticking the checkbox the
user would tick. Then the accepted case deployed to the only free node
and the next case found every checkbox disabled: the test consumed its
own fixture. The third case uses a different service for that reason.

* test: cover the catalog-driven branches that were only ever proven by hand

codecov/patch was red, and it was right. Four lines this branch added
had no test: the three per-node-earnings branches and the credential
hint assignment. I had verified all of them in a scratch script when
writing them and never committed the check — which is the same as not
having one, because a path proven once and never again silently rots.

Covered now: a service that declares per_node_earnings gets its rows; a
service that does not declare it gets nothing; an unknown slug gets
nothing; a service that declares the capability without implementing it
warns and returns empty rather than 500ing; missing credentials return
empty rather than calling a collector with empty auth and surfacing a
provider error as if the feature were broken; and the 13 credential
hints are still served from YAML with their text intact.

Fixed by writing the tests rather than by telling codecov to ignore
frontend paths. The frontend genuinely is uncovered by pytest — that is
what the browser harnesses are for — but these four were Python, in
reach, and simply untested.

* fix: degrade on a provider failure, and put the rel where it survives

Two findings from review, one of which needed fixing somewhere other
than where it was reported.

api_per_node_earnings awaited the collector without catching. That call
reaches a THIRD-PARTY API which times out, rate-limits and returns HTML
instead of JSON on a bad day — none of it a fault in CashPilot, all of
it arriving as a raw 500. Every other collector call in this file
degrades; this one did not. An empty per-node breakdown alongside the
account total is a smaller loss than a broken page. The collector is
still closed on that path, so a flaky provider cannot leak a session
per request.

The review also asked for rel="noopener noreferrer" on the credential
hint anchors, listing five service YAMLs. Both parts needed correcting.

It is 13 files, not 5 — every migrated hint carries a target='_blank'
anchor. And writing the rel there would have done NOTHING: sanitizeHint
strips every attribute it does not explicitly keep, so the rel would
have been removed on its way to the DOM. The fix would have looked
applied, passed review, and had no effect.

So the rel is set by the sanitiser, after the stripping rather than
before it, which covers all 13 hints and every future one and cannot be
forgotten by whoever writes the next. Verified in a real DOM against
the real shipped hint: the anchor comes out with the rel; a rel written
in the YAML is overwritten rather than honoured; a link with no target
gains nothing; and a javascript: href, an onclick and a script tag are
all still stripped. That harness is committed, because the property it
checks is invisible in the source.

Worth being accurate about the severity: modern browsers imply noopener
for target=_blank, so this was defence in depth rather than a live
hole. noreferrer is the part still worth having — these links point at
a provider dashboard and the referrer would otherwise name the user's
CashPilot host.

* fix: a brand-new user could not create an account at all

The onboarding page — where a fresh install redirects you — had no
field for the one-time setup token, and never sent one. /register
requires it on first run, so every new installation answered the form
with a 403 and the message "Registration failed. Please try again."
Trying again did the same thing. Forever.

This is not a regression from this branch. v1.10.1 ships it: `git show
v1.10.1:app/templates/onboarding.html` contains no setup_token at all.
The gate landed in PR #100 and the page it gates was never updated. It
survived because existing installs were unaffected — the people who
could have noticed already had accounts, and every test in the suite
creates its owner by calling database.create_user directly, so the
actual install flow had never once been exercised.

Found by the user-story audit asking the obvious question nobody had
asked: can a new user actually install this?

Fixed: the field, the value in the POST, and the message. A 403 here
has exactly one cause, so it now says so and points at
`docker logs cashpilot-ui` and the log line to look for, instead of
suggesting a retry that cannot work.

Verified by driving a genuinely fresh instance in a browser — land on
/, follow the redirect, fill the form, press the button — and then
asking the SERVER whether the account exists rather than trusting the
page. Removing the token from the POST turns that check red with a 403.
The harness is committed, since the suite could not see this and would
not have.

The pytest additions carry their own correction: the first version of
the gate test passed a wrong token and was told "allowed", which looked
like a security hole. It was an unarmed fixture — conftest clears the
token global before every test and verify() returns True when no token
is active. Now armed, with a positive case so the two negative ones
cannot pass with the gate welded shut.

* fix: net earnings charged no electricity at all, and called it known

/api/earnings/net filtered running containers on c["service"], while
_get_all_worker_containers emits "slug". It therefore matched NOTHING
in production, every time.

The consequence was not a missing panel. Every service was charged 0 W,
so cost came out 0.00 and net was reported EQUAL TO GROSS with
cost_known: true. The endpoint's docstring promises it reports net
alongside gross and never presents earnings as profit; it did exactly
that, and said the cost was known while doing it.

Measured on a seeded instance before and after:

  before   gross 4.00   cost 0.00   net 4.00
  after    gross 4.00   cost 3.97   net 0.03

The electricity was consuming almost the entire gross. A user reading
this would have concluded a service was worth running when it was
barely breaking even.

This is the SECOND time this key has bitten the project.
egress.container_slug() exists because of the first time and accepts
both spellings; its docstring says code reading "service" matched
nothing in production while its tests passed. That is precisely what
happened again, in a function written after the helper existed. Both
readers in this endpoint now use it, so the legacy shape still works.

The regression tests carry their own correction. The first negative
control reported PASS with the fix reverted, which would have meant the
tests were worthless — in fact `ruff format` had collapsed the
comprehension onto one line, so the revert patch never matched and the
control was a no-op. Re-run against the real text, it fails exactly the
two tests that name the bug. A control that cannot fail proves nothing,
and one that silently does not apply is worse than none.

* test: nothing parsed the JavaScript, so a dead dashboard shipped green

Demonstrated rather than argued: appending `function broken( {` to
app/static/js/app.js leaves the entire suite green — 2306 passed — and
CI green with it, because nothing in test.yml so much as looks at a .js
file.

What ships in that state is a completely dead dashboard. app.js is one
IIFE assigned to CP, so a parse failure anywhere in its 2900 lines
means CP is never defined; delegate.js falls through to
`typeof CP !== 'undefined' ? CP : {}` and every data-action button logs
"No handler named X" and does nothing. No error page, no failed
request, no alert — a UI where nothing works and nothing says so.

That risk grew with this release rather than shrinking: 1.11.0 moved
the payout queue, payout progress, credential health, running costs and
the deploy-risk gate into that file, and every one of them is reachable
only through CP. The audit was right to call this the highest-leverage
gap it found.

Now gated in both places. pytest runs `node --check` over every JS file
so it fails locally, skipping only where node is absent, since a missing
toolchain is not a code defect. CI installs node and runs the same
check as a hard step, because a skip is not a check.

CI also runs scripts/currency_check.mjs, which needs no browser and
pins behaviour pytest cannot see — it exists because formatCurrency
once labelled an unconverted USD amount with the viewer's currency
symbol, rendering $24.90 as "£24.90".

The three committed browser harnesses are asserted to exist and to be
capable of failing, so deleting one is a red test rather than a silent
loss of the only check on that behaviour.

* fix: the wizard selected services invisibly, and the chart invented zeros

Two audit findings, the same mistake in different costumes: the code
had no value, so it made one up.

The wizard's service cards passed `data-a2="this"` — a leftover from
the inline-handler migration, where `this` really was the element.
delegate.js reads arguments out of `dataset`, and dataset values are
always strings, so the handler received the literal string "this" and
threw on `.classList`.

The throw was the harmless half. The selection is mutated BEFORE the
line that throws, so a click added the service and then died before
highlighting anything. Nothing appeared to happen; clicking again — the
natural reaction — silently removed it; and the user then pressed Next
and was told to select a service. Demonstrated by running delegate.js's
own argument extraction: args are ["honeygain", "this"], the call
raises TypeError, and selectedServices already contains the slug.

The handler now resolves its own element, because an element cannot
travel through a data-* attribute and pretending otherwise is what
caused this.

The earnings chart, on a failed fetch, drew one bar per day at exactly
0.00 with real-money axes and tooltips. The user read "I earned nothing
every day for a month" when the browser could not reach the server. Of
everywhere in this app that could render unknown as a number, this was
the biggest on screen. It now leaves an existing chart alone — stale
real data beats invented data — and otherwise says the figures could
not be fetched and that this is not a reading of zero.

The guard against the data-a2 class needed fixing too, and this is the
FOURTH time in this session that a text-matching check has fired on the
comment explaining the thing it forbids. Rewording the comment each
time makes the code worse to read in order to keep a weak test green,
so the scan now strips comments first. Reintroducing the real markup
still fails it; the explanation no longer does.

* fix: a live credential was written to the container log, twice over

Several providers authenticate with a bare header value — Salad's auth
cookie, Grass's access token, ProxyRack's API key, EarnApp's OAuth
token, PacketStream's JWT. When httpx rejects one, the exception TEXT
is the credential. Two independent sites printed it in plaintext.

app/main.py logged result.error raw, one line ABOVE the comment
explaining that collector errors must be redacted because "an httpx
exception embeds the offending header or URL — which for several
providers is a live credential". The alert built from that same string
was carefully sanitised; the log line above it was not, which is what
made this easy to miss for so long.

All sixteen collectors then did it again, independently:
logger.error("X collection failed: %s", exc, exc_info=True) — the
message leaking it once and the chained httpcore/httpx traceback
repeating it. Fixing main.py alone would have left that fully intact.

The result went to stdout, so into `docker logs cashpilot-ui` and into
whatever ships those logs off the box — Loki, journald, the Unraid
syslog. Anyone in the docker group could read a live provider
credential.

notify.redact() already existed and already worked; it was simply
applied one step too late. It is now applied before both log calls, and
the collectors route through a single base.log_failure() helper so a
new collector cannot reintroduce this by copying the idiom from its
neighbours — which is exactly how it reached all sixteen.

Tested with a distinctive fake JWT asserted to appear nowhere in
captured log output, which is the only formulation that cannot be
satisfied by redacting in the wrong place. exc_info is checked with an
AST walk rather than a grep, because the helper's own docstring quotes
the idiom it forbids and a text scan would flag the explanation.

Also fixes MystNodes' per-node fetch, which had the same shape and was
not in the audit's list.

* style: apply ruff format to the redacted log call

ruff check was green and ruff format --check was not. CI runs both, and
my final gate only ran the first — the exact gap I have hit on this repo
before.

* fix: a token price move invented earnings for anyone-protocol

The collector reported balance in USD, converted from the token count
at collection time. balance is CUMULATIVE lifetime rewards and earnings
are the delta between two readings, so pricing before the subtraction
turned an exchange-rate movement into income: the same 1000 ANYONE read
at $0.10 and then at $0.12 produced $20 "earned today" without a single
token being paid out — and would have reversed into an apparent loss
when the price fell back.

get_daily_earnings already takes the delta in the native currency and
prices it afterwards. That is the whole reason it is written that way,
and handing it USD defeated it for this one service.

It now reports native ANYONE, with the token mapped in CRYPTO_IDS so
the existing machinery prices it. The CoinGecko id really is
"airtor-protocol" — the project renamed from ATOR and the slug did not
follow — taken from the collector, which had been querying exactly that
id all along. Its private price fetch is now dead and removed.

Safe for existing installs: database.py only takes a delta when the
stored currency matches the previous reading, so the USD-to-ANYONE
changeover is skipped rather than counted as a 900-unit gain. Asserted,
because that is the property that makes this shippable rather than a
migration.

The contract fixture already declared currency ANYONE. It was right and
the code disagreed with it; the fixture only needed its now-stale "usd"
field dropped.

Two of my own tests needed correcting before they were worth anything.
They first read r["earnings"] keyed by ISO date while get_daily_earnings
returns {"date": "Aug 02", "amount": ...} — every lookup returned 0.0,
so three assertions expecting 0.0 passed without touching the code they
name. Then a regex fix swallowed a @pytest.mark.asyncio decorator into
an assert message, which stayed green only because an assert's message
is never evaluated while the assertion holds.

* fix: the shared fleet key could permanently delete payout records

require_role granted the fleet role writer on every writer check. The
comment justified it as being "for heartbeat/status endpoints" — but
the heartbeat never went through require_role at all.
api_worker_heartbeat authenticates with _authenticate_worker_heartbeat,
so the grant gave workers nothing they needed while unlocking five
user-facing routes: payout confirm, payout REJECT, container logs,
collection, and arbitrary worker commands.

Payout rejection is a hard DELETE FROM payouts. The route's own comment
says "this mutates financial records, and rejection is a hard DELETE. A
read-only account must not be able to destroy them."

CASHPILOT_API_KEY is handed to every worker host by design — docs/fleet.md
puts it in each worker's compose environment — so it is readable by any
machine in the fleet, by anyone who can `docker inspect
cashpilot-worker`, and by anyone who can read the shared /fleet volume.
docs/upgrade-v1.md already claims the shared key is "enrollment/bootstrap
only" and "stops being a fleet-wide credential once a worker is
enrolled". The code never implemented that.

Verified against a running server rather than argued. Before: a bare
`Authorization: Bearer <fleet key>` POST to
/api/earnings/payouts/1/reject returned 200 and deleted the row, with no
session and no account. After: 403. Worker commands likewise 403. The
heartbeat still returns 200, which is what made the removal safe.

Automation that genuinely needs write access already has
CASHPILOT_ADMIN_API_KEY, which resolves to owner and is not distributed
to workers.

tests/test_auth.py had a test asserting the vulnerable behaviour
(test_fleet_satisfies_writer). It is now inverted, with the reason
recorded, plus an end-to-end class that calls the real handlers through
the real guard — the unit test alone could not have caught this, since
require_role is a pure function and the damage only appears when you ask
what a request carrying that key can reach.

That end-to-end class needed a correction of its own: it first patched
app.main.auth_module, which does not exist, so the three control cases
errored while the two "fleet is refused" cases passed — passing because
everything failed, not because the guard worked. Patched at
app.deps._require_auth_api instead, so the real require_role runs.

* fix: ten verified audit findings (beads batch 1) (#182)

* fix: ten verified audit findings — honest defaults, working links, real gates

Batch 1 of the bead backlog. Every one was independently re-verified
against this branch before being touched: the audit's own adversarial
pass had already refuted 13 of 94 findings, so none of these was taken
on trust.

CashPilot-c1q / CashPilot-i21 (one defect, filed twice from two areas)
  The config.updated_at migration back-filled every existing row with
  datetime('now'), so an upgraded volume reported every credential as
  brand new — including a Bytelixir session cookie that expires after
  two hours and had in fact expired days earlier. Unknown age rendered
  as the most favourable known age. NULL is the honest value and both
  consumers already skip it.

CashPilot-153  bool() on a TEXT config value made "false", "0" and "no"
  all mean True. That flag decides whether the fleet page says "turning
  it off would save that" or "this machine would be on anyway" —
  opposite advice from the same setting.

CashPilot-a02  electricity_price_per_kwh was honoured by the fleet
  endpoint and ignored by the dashboard one, so an upgrading user saw
  running costs on one page and "cost unknown" on the other from a
  single stored value. Both now resolve through _tariff_price.

CashPilot-4ks  /docs, /redoc and /openapi.json were served
  unauthenticated. The schema is a complete map of the admin surface,
  including the worker and payout endpoints. Disabled.

CashPilot-0id  The Configured badge used `.some`, so one of two required
  credentials was enough to show it — while make_collectors skipped that
  service for the missing field and it silently never collected. Now
  requires every REQUIRED field (optional ones must not block it) and
  has a distinct Incomplete state.

CashPilot-0rw  The credential hint rendered only in the modal, though
  services/_schema.yml says it appears "next to the input in Settings" —
  the one screen a tracking-only user uses. Rendered through
  sanitizeHint, since the hints contain anchors.

CashPilot-x2r  entrypoint.sh is the ENTRYPOINT of both images and was in
  neither the release paths filter nor either detect regex, so changing
  the privilege-drop and Docker-socket-group script published nothing.

CashPilot-w3g  An unwritable key file marked the encryption key
  ephemeral even when it came from CASHPILOT_ENCRYPTION_KEY — a value
  identical on every restart. Startup then refused and told the user to
  supply a key via CASHPILOT_ENCRYPTION_KEY, which they already had.

CashPilot-ecc  The Salad credential hint pointed at app.salad.com, which
  does not resolve. The working host was already in the same file three
  lines above.

CashPilot-i5l  CLAUDE.md described a synthetic first-earnings baseline
  row. No such code exists anywhere in app/. The documented outcome is
  delivered by the no-predecessor rule instead, so the doc was corrected
  rather than the code.

39 regression tests, each naming its bead. Negative controls run:
reverting the docs and dedicated fixes turns four of them red.

* fix: make the key-persistence message match its actual consequence

Both findings from CodeRabbit on this PR, both correct.

The fix stopped treating an env-supplied key as ephemeral but left the
error text saying credentials 'will be unreadable after a restart'.
That is untrue while CASHPILOT_ENCRYPTION_KEY is set, and it is exactly
the kind of false alarm that teaches people to ignore the real one. A
minted key keeps the loud error; a supplied key gets a warning that says
what actually matters — keep the variable set, because without the
cached file it is the only copy.

The comment above the config.updated_at migration still described the
back-fill this PR removes, which would have invited someone to restore
it.

The batch's own count guard fired on the new test class. That is the
guard working: review-response classes are now excluded by name rather
than by raising the number, so a dropped bead fix still shows up.

* fix: a service nothing has read is not a service earning zero (#183)

Batch 2. Eight beads, two fixes — three audit areas each filed the same
two defects, and both are one mistake wearing different clothes: code
with no value supplying a confident one.

CashPilot-vp6 / -ikh / -7qk
  `balance_map.get(slug, 0.0)` turned "no reading" into $0.00 on both
  the container and external paths. Nothing else caught it:
  collector_disconnected needs an alert (none was raised, because no
  collector ran), collector_needs_setup only checks whether config keys
  are filled, and the flatline detector explicitly skips rows whose max
  balance is 0. So the dashboard showed a service Running with a balance
  of $0.00 while the bell said "All collectors healthy" — three
  independent signals all agreeing on something none of them had
  checked.

  The API now sends balance: null plus balance_known, and the row
  renders an em dash. The renderer's `|| 0` had to go with it: it would
  have coerced the new null straight back to a confident zero and undone
  the fix while every test still passed.

CashPilot-3oa / -jkd / -s2b
  The payout-progress card asserted a definite 0.00 lifetime and a
  0%-of-minimum bar for a service never read. It already got "Balance
  now" right via balance_known, which is what made the two fabricated
  figures beside it read as corroborated. The card now hides when there
  is neither a balance nor a confirmed payout, and lifetime_earned is
  null rather than a folded zero.

One correction to my own change: I also nulled `projection`, and an
existing test objected. It was right. payouts.project already returns an
explicit NOT_ENOUGH_DATA state, which says WHY there is no estimate —
strictly more useful than null, and nulling it pushed a null-check onto
every caller. The misleading part was never that field but the card
drawing a 0% bar from it, which is fixed where the card is drawn.

Verified with a negative control: restoring the 0.0 default turns the
API test red.

* fix: a provider changing its API shape is an error, not zero earnings (#184)

Batch 3. Seven beads.

CashPilot-y5t — an interrupted upgrade bricked startup permanently.
  The workers table rebuild ran through executescript, which COMMITS
  PER STATEMENT, with no DROP IF EXISTS and no transaction. An
  interruption between CREATE and RENAME left workers_new behind for
  good; the guard above it was still true on the next boot, so the
  rebuild re-ran, the CREATE failed with "table already exists",
  init_db raised, and the app never started again.

  Reproduced before the fix on a seeded pre-client_id volume:
  OperationalError: table workers_new already exists — on every
  subsequent restart. Now re-entrant, and the worker rows survive.
  The leftover is always disposable: it is only ever a partial copy,
  and the real table is intact until the DROP.

CashPilot-3yn / -rtb (Storj), -10d (Anyone), -fzw (Grass)
  All three read provider payloads with .get(key, default), so a
  renamed or missing field became a confident 0.00 with no error. That
  zero was written as a real balance: no alert (there was no error), no
  flatline warning (that detector explicitly skips rows whose max
  balance is 0), nothing on screen to distinguish it from a service
  that genuinely earned nothing.

  Storj is the worst of the three, because its zero is a DROP from the
  previous reading — so _detect_payout asked the user to confirm
  receiving their entire balance, money that was never paid, and a
  confirmed payout is permanent.

  Each now distinguishes an absent KEY from a genuine zero: "0" and
  "null" still mean zero for Anyone, an empty device list still means
  zero for Grass, and Storj still sums whichever payout fields are
  present. Only a shape it does not recognise raises, and the message
  names the service and what it actually saw.

CashPilot-0ja — /api/preferences spawned a full fleet collection for
  any authenticated user, while /api/collect gates the identical call
  behind _require_writer. Two doors, one side effect, different locks;
  a viewer could hit every provider API on demand. The preference still
  saves for a viewer — only the collection is gated, so completing
  setup is not blocked.

CashPilot-dbh — both Deploy buttons were enabled for writers, while
  api_deploy requires owner. A writer filled in provider credentials
  and only then got a 403. The buttons now match the server.

Negative controls run: reverting the migration guard fails three tests,
reverting the collection gate fails one.

* fix: totals that were wrong, not merely missing (beads batch 4) (#185)

* chore: drop an empty test file that slipped into batch 1

tests/test_security_headers.py was a 0-byte stray from an interrupted
heredoc earlier in this work. It was untracked for a long time and got
swept into the batch-1 commit by a broad 'git add tests/'. An empty test
file contributes nothing and implies coverage that does not exist.

* fix: three totals that were wrong rather than merely missing

Batch 4.

CashPilot-47t — the dashboard Total silently dropped whole holdings.
  exchange_rates.to_usd consults only the LIVE caches, so a crypto
  whose rate lookup was merely stale vanished from the headline figure
  with nothing on screen saying it was incomplete. In a seeded run, 110
  MYST disappeared entirely; it now reads 13.20 from the rate the
  reading was recorded at.

  This is not a new rule invented here. get_earnings_dashboard_summary
  and get_earned_by_platform already price rows exactly this way, and
  upsert_earnings stores fx_rate_usd on every row specifically so "the
  USD value of a historical non-USD reading cannot be reconstructed
  later" without it. The Total was the one figure applying a stricter
  rule than the cards beside it, which is why they disagreed. It now
  uses database._usd_rate, the same validator, so a 0, negative, inf or
  nan rate is still rejected rather than believed.

  The query had to select fx_rate_usd at all — the column never left
  SQLite for this path, so the value was unreachable.

  What still cannot be priced is now REPORTED as unpriced_platforms
  rather than logged and forgotten (CashPilot-oj4): a total that
  silently omits holdings is indistinguishable from a correct one.

CashPilot-7oj — month_change was a hardcoded 0.0 that nothing computed,
  rendered as "+0.0%" in the positive style forever. It is now null, and
  setChangeIndicator renders an unmeasured value as blank instead of
  throwing on it.

CashPilot-he1 — fleet net subtracted the cost of SOME machines from the
  gross of ALL of them, so the more machines you could not price, the
  healthier the fleet appeared. Net now compares like with like; gross
  still covers the whole fleet, and cost_known_for still says how many
  were measured.

The 47t helper is module-level rather than a closure: defined inside the
loop it captured `currency` late (ruff B023), so every conversion would
have used the last currency seen — silently worse than the bug.

Negative controls: reverting he1 and 7oj turns three tests red.

The batch-4 tests strip comments before scanning source. This is the
fifth time a text-matching assertion has matched the comment explaining
the thing it checks — here the comment mentions `pct.toFixed` while
warning it would throw, so an ordering assertion found the prose first
and failed on correct code.

* fix: an owner adding a user was logged into that user's account (#186)

Batch 5. Four beads.

CashPilot-5e6 — do_register minted a session for the account it had
  just created and set it as the CALLER's cookie. An owner adding a
  viewer was silently demoted into that viewer's session: their own
  access gone, with no sign anything had happened beyond landing on a
  page with fewer controls. Only the first-run owner is signed in here
  now, which is the case this code was written for; adding a user
  returns to /users with the caller's session untouched.

CashPilot-zcc — the UI signed outbound worker commands with a key the
  worker had never confirmed. get_worker_key discards the confirmed
  flag that get_worker_key_state returns, and the unconfirmed state is
  reachable whenever the worker fails to persist its key and logs
  "staying on shared key". The worker then read as ONLINE from its
  heartbeats while every deploy, start, stop, restart and logs call
  failed 401 — two symptoms with nothing on screen connecting them. It
  now falls back to the shared key until the worker confirms.

CashPilot-cxy — per-worker watts and dedicated settings were keyed on
  the AUTOINCREMENT row id. delete_worker removes the row and nothing
  else, so a host removed and re-enrolled came back under a new id,
  silently orphaning its settings while the old keys lingered forever.
  Now keyed on client_id, which is UNIQUE and survives re-enrolment,
  with the id-based key still honoured for existing volumes.

CashPilot-153 (this branch's half) — the same helper also fixes bool()
  on a TEXT config value, where "false", "0" and "no" were all truthy.

Two existing tests asserted the old worker-auth contract and were
updated rather than worked around; a third case was added for the
unconfirmed key, which is the state the bug lived in.

Negative controls run for both behaviour changes. The first attempt at
the 5e6 control PASSED — because my revert patch did not match and
silently did nothing. Re-done against the real text, it fails the two
tests that name the defect.

* fix: assert the salad host by parsing it, not by substring

CodeQL flagged two high-severity 'incomplete URL substring sanitization'
alerts on this branch, both in the test added for CashPilot-ecc. It was
right, and not only formally: "app.salad.io" in text also passes for
app.salad.io.evil.com and for evil.com/?next=app.salad.io. A test whose
job is to pin which host we send users to should not accept a host that
merely contains the right string.

Both assertions now parse each URL and compare hostname equality.
Verified by planting app.salad.io.evil.example in salad.yml: the old
assertion accepted it, the new one fails on it.

* test: assert the salad host with explicit equality, not membership

The last CodeQL alert on this branch. Membership on a list IS exact, but
the scanner cannot distinguish `x in hosts` from a substring test
against a string and flags the shape either way.

Rewritten as a count of hosts equal to the expected one. That is
unambiguous to both the reader and the scanner, and it asserts something
slightly stronger: that the host actually appears, rather than that
something merely matched. Still verified against a planted
app.salad.io.evil.example, which it rejects.
GeiserX added a commit that referenced this pull request Aug 5, 2026
…un token, orchestrator coverage) (#100)

* perf(db): bound health_events growth — split retention + weekly VACUUM

health_events is dominated by check_ok/check_down uptime samples (one per
service every 5 minutes), yet get_health_scores only reads a bounded window
(/api/health/scores caps it at 90 days) while retention kept everything 400
days. Trim samples to HEALTH_CHECK_RETENTION_DAYS=95 (just past the max query
window, so no allowed query can out-range its own samples) while keeping the
rare lifecycle events (start/stop/restart/crash) the full 400 days. Add a weekly
VACUUM job (commit-first, WAL-checkpoint TRUNCATE) since SQLite never shrinks the
file on DELETE alone — without it the DB keeps its high-water-mark size forever.

Uptime% is unchanged for every allowed query. Tests cover the split-retention
boundary + a clean VACUUM run.

* test: cover orchestrator/worker_api Docker paths; expect db_vacuum job

Add 61 tests (Docker SDK fully mocked) for the two lowest-covered, most
privileged modules: orchestrator.py 38%->73%, worker_api.py 44%->74%. Covers
_collect_stats (zero-delta CPU, missing keys, APIError), _find_container name/
label/ValueError fallback, get_status[_light] docker-unavailable + corrupted-
container-skip paths, _verify_api_key negative paths, _validate_deploy_spec
rejections (privileged, cap_add, network_mode, blocked volume roots incl.
subpaths; host/bridge + named volumes allowed), and every command endpoint's
success + ValueError->404 / RuntimeError->503 branches.

Also update the lifespan-scheduler test to expect the new db_vacuum interval
job (added in the previous commit).

* feat(security): setup-token gate for first-run + trusted-proxy client IP

The "private network only" guard on first-run /register trusts
request.client.host, which behind a reverse proxy is the proxy (a loopback/
private address) — so the check always passes and the first public visitor could
seize the owner account. Add a proxy-independent gate: while no users exist,
startup generates a one-time setup token, persists it (encrypted config), and
logs it; /register then requires it (query param, X-Setup-Token header, or form
field) and it is retired the moment the owner account is created.

Also resolve the real client IP behind a trusted proxy (opt-in
CASHPILOT_TRUSTED_PROXY, right-most X-Forwarded-For), used by both the first-run
network check and the login rate limiter — which previously collapsed every
proxied client into one bucket keyed by the proxy IP.

Tests: token module + client_ip (trusted/untrusted) + the first-run guard
(missing/wrong/query/header/form token, public-IP-still-blocked, network-only
fallback once retired) + register integration (403 without token, 303 + token
cleared with it); conftest resets the token global between tests.

* fix(db): restore health-retention split + VACUUM lost in rebase; cover _run_vacuum + first-run token

A messy multi-rebase during PR stacking silently dropped the database.py half of
the health_events change (HEALTH_CHECK_RETENTION_DAYS, the check-sample purge, and
vacuum_database) while keeping main.py's _run_vacuum + scheduler job — leaving
main calling a database function that no longer existed. Restore the database.py
logic and its tests, and add coverage for main._run_vacuum (success + error) and
the lifespan first-run setup-token generation (the codecov/patch gaps).

Full pytest 1081 passed; ruff check + format clean.

* fix(security): don't accept first-run setup token via query string (CodeRabbit)

A ?setup_token= URL leaks the secret into reverse-proxy access logs and browser
history. Drop query-param support in _require_first_run_access (form field +
X-Setup-Token header only); the GET /register page now renders behind just the
network check so the operator can reach it, the token is a visible form field
they type from the logs, and the startup log no longer embeds the token in a URL.
GeiserX added a commit that referenced this pull request Aug 5, 2026
…ard the next one (#181)

* feat: somewhere to answer "did you get paid?"

The backend has been detecting payouts for a release and had nowhere to
ask. A balance drop is recorded as a PROBABLE payout, and until someone
answers it never counts toward lifetime earnings — so a real payout
looked exactly like a loss, permanently. The endpoints to confirm or
reject one existed, were tested, and were called by nothing.

A dashboard card, hidden until there is something to answer, because an
empty card every day trains people to ignore the one day it matters.
Confirm is one click; reject asks first, since it is a hard DELETE with
no undo. A failed fetch leaves the card alone rather than hiding it —
unknown is not "nothing pending", and hiding it would silently drop a
question the user still owes an answer to.

Verified in a real browser over CDP, not by reading: the card renders,
both buttons carry data-action with zero inline handlers, CP resolves
the handlers, clicking Confirm actually calls the API and the row
disappears, and no CSP violation or JS error fires.

That browser check earned its keep immediately. A 24.90 USD payout
rendered as "£18.55", because the dashboard's display currency was
applied to it. Everywhere else that is correct; here it is a converted
approximation of one specific real transaction that the user is about
to check against the provider's own page. The native amount now leads
and the conversion follows, marked approximate and suppressed when the
two are identical.

The tests carry their own correction: the first version sliced a fixed
number of characters from app.js and read on into the next function, so
it reported that confirmPayout asks for confirmation when what it had
actually found was rejectPayout. Bounded on the next function instead.

* feat: show how far off the payout is, on the service that earns it

The second orphaned endpoint. payout-progress answers three questions
that used to be one number which went DOWN when the user got paid:
what is sitting there now, what this service has earned in total, and
when it can be cashed out. The backend has answered all three since
1.10.0 and nothing ever asked.

It nearly went into app/templates/service_detail.html — which turns out
to be a DEAD template. No route renders it and nothing references it;
the real detail view is a modal built by renderServiceDetail. A card
added there would have passed every string test and never once appeared
on screen. Found by driving the page rather than reading it. There is
now a test asserting that template stays unrendered, which should fail
and be deleted the day someone wires it up.

Two things the browser caught that no string test would have:

The card first rendered "BALANCE NOW £3.73" directly above "to the 20
minimum" — two halves of one comparison in different units, with a
progress bar agreeing with neither. Everything in the card is now
stated in the cashout currency the provider's own minimum is declared
in, which is the only unit that comparison makes sense in. This is the
opposite call to the payout queue, and for the same reason: there, the
figure is a specific transaction to check against a provider's page;
here, it is a value being compared to a threshold.

The bar is derived from `remaining`, because the API returns no
`threshold` key — checked against payouts.project() rather than
assumed, which is how the first version came to reference one. A
service with no documented minimum omits `remaining` entirely, and that
absence is what suppresses the bar: stuck at zero it would say the
wrong thing far more loudly than no bar at all.

Verified over CDP: card visible, bar at 25% for 5.00 of a 20 minimum,
"About 15 day(s) to the 20 minimum, at 1.0000 a day", no CSP violation,
no JS error, zero inline handlers.

* fix: never label an unconverted amount as the display currency

CodeRabbit flagged the payout queue printing a duplicate amount. Chasing
it in a browser turned up a larger defect underneath, in code every
figure on the dashboard goes through.

formatCurrency left the amount in USD when the display currency had no
rate, and then formatted it AS that currency — so $24.90 rendered as
"£24.90". Not a conversion: the same number wearing a different sign.
Rates are fetched asynchronously, so this hit EVERY figure on the page
until they arrived, and hit permanently for any currency with no rate.
The label now follows the conversion that actually happened; with no
rate the value stays in USD and says USD.

The duplicate itself was the reported symptom. It compared formatted
strings — "$24.90" against "24.90 USD" — which differ even when nothing
was converted. Comparing against _displayCurrency was still wrong for
the no-rate case, so the decision now asks which currency the formatter
will ACTUALLY use, including an unpriced token, which stays in its own
units.

My test for that was worse than the bug: it asserted the literal source
text `converted !== native`, so it passed while that very comparison
produced the duplication it claimed to prevent. Asserting the
implementation is not asserting the behaviour. Replaced, and the real
proof is scripts/currency_check.mjs, which runs the actual functions
against controlled rate state and covers five cases including both
no-rate paths.

Also from the same review: confirm and reject require writer access on
the backend, and the queue showed the buttons to everyone, so a viewer
could only ever click into a 403. Gated on _canWrite, following the
idiom the deploy button already uses, with a line saying why rather
than an unexplained empty row.

* feat: warn before a credential dies, not after collection stops

Third orphaned endpoint. Several providers issue session cookies
measured in hours — Bytelixir's is about two. When one dies, collection
stops and NOTHING says so: the dashboard keeps showing the last balance
it managed to read, which looks exactly like a service that is simply
not earning. The endpoint that computes this shipped in 1.10.x and was
called by nothing, so the warning existed and nobody ever saw it.

On Settings rather than the dashboard, and always visible there when
anything is configured. That is the opposite call to the payout card,
deliberately: the dashboard is glanceable and a permanent card becomes
furniture, while Settings is somewhere you go on purpose.

Worst first, because the only rows worth acting on are the bad ones.
Where the provider offers a longer-lived credential the row says so —
re-pasting a two-hour cookie restarts the cycle, setting the durable
one ends it, and that is the actual fix rather than a nag.

Verified over CDP against a credential aged past its lifetime: three
rows, "Likely expired" first, the reason and the durable alternative
both shown, Update offered only on the actionable row and only to an
owner, no CSP violation, no JS error — and the seeded credential value
never appears anywhere in the page, which the check asserts explicitly.

Also in this iteration: the daily-chart currency item from the 1.11.0
list is a FALSE ALARM, and re-verifying it rather than fixing it was
the right call. The dataset holds raw USD, but the y-axis ticks and the
tooltip both format through the same converter, so bars and ticks share
one space and the reading is consistent. That consistency is one line
from breaking in a way that would silently misplace every bar against
its own axis, so it now has a test.

* refactor: move service-specific knowledge out of app/ and into the catalog

The repo's own rule is "never hardcode service-specific logic in app/",
and it had drifted twice.

Thirteen credential hints — prose telling a user where to find a token
in a provider's own UI — lived in a dict inside api_collectors_meta,
out of reach of anyone editing the service they describe. They now live
in collector.credential_hint in each service's YAML. Extracted by AST
and verified byte-identical through the endpoint afterwards: 13 of 13,
none altered, none invented.

Given a home rather than reusing collector.notes, which the schema
documents as implementation hints for developers. That is a different
audience: notes explain the API to whoever writes the collector, the
hint is what someone reads while looking at a provider dashboard they
have never seen before. Conflating them would have made both worse.

api_per_node_earnings branched on `slug == "mysterium"` and imported
that collector class by name — two pieces of service knowledge in a
route handler, so a second service reporting per-node figures meant
editing the handler. It now reads collector.per_node_earnings from the
catalog and builds through the existing factory. A service that
declares the capability without implementing it logs and returns empty
rather than 500ing.

The rule now has a test, because a rule without one is a preference: an
AST scan for catalogued slugs appearing as literals anywhere in app/
outside app/collectors, which is exempt by design. The one remaining
match is allowlisted with its reason — "mysterium" in exchange_rates is
a CoinGecko coin id, not a service reference — and a further test fails
if that exemption ever goes stale.

The guard checks CODE, not text. The first version failed on the
comment explaining the branch it had removed, which is the third time
this session a text-matching check has fired on its own explanation. A
guard that reads comments punishes documenting the fix.

* fix: connect the guard that was never wired, delete the constant that was

Two half-alive things in machine_economics, decided rather than left.

per_service_is_meaningful() was called only by its own tests. That
looked like dead code, but the one path that needed it was doing
without: api_earnings_net attributes electricity per service, and a
bandwidth container adds roughly 1-3 W to a host that is already on —
below what a consumer smart plug can resolve. The module has held that
rule since it was written and nothing had ever asked it. So this is a
guard that was never connected, not code to remove.

Connected as a per-row `cost_attributable` flag rather than by dropping
the figure. The host really does draw the power; what is unreliable is
which service to blame. Nulling the cost would have understated the
fleet total, trading a precision problem for an accuracy one — so the
cost still reports and still counts, and only the attribution is
doubted. Verified: 2 W flags false, 120 W flags true, and the total
still counts both. The flag defaults to True, so a caller that does not
pass it is unaffected.

DEFAULT_IDLE_WATTS genuinely was dead, and worse than dead: 65.0 with
the same meaning as power.DEFAULT_HOST_TDP_WATTS, which is the same
number and has a real consumer. Two constants for one quantity in two
modules drift apart silently and force the next reader to guess which
is authoritative. Deleted, with the reason left where it stood so the
next person does not re-add it.

* feat: show what the fleet costs to run, without inventing the parts we cannot know

Fourth and fifth orphaned endpoints. fleet/economics answers "is this
machine worth leaving on" and earnings/net answers it per service; both
shipped in 1.10.x and were called by nothing.

Rendered on the fleet page, hidden until there is a tariff to answer
with, because "unknown" repeated on every visit is noise rather than
information.

The endpoints already return a finished sentence for every branch —
"not billed by the kilowatt-hour", "without this machine's power draw
CashPilot cannot say whether that covers the electricity, and guessing
would be worse than not saying" — so those are rendered rather than
reworded into something cheerier. The verdicts exist because the answer
is often that a machine is not worth its electricity, and softening
that would defeat the point of computing it.

An unknown cost renders as a dash, never as zero. A zero would turn
gross into apparent profit, which is precisely what app/power.py exists
to prevent, and it must not creep back in at the presentation layer
just because the API got it right.

Per-service rows appear only where the cost can honestly be attributed,
using the cost_attributable flag wired up in the previous commit. What
is withheld is COUNTED OUT LOUD — "1 service(s) draw too little on a
machine that is already on ... their cost is still counted in the
machine totals above" — because dropping the rows silently would read
as those services being free.

Verified over CDP against a metered worker with no declared wattage:
card visible, the machine's unknown-cost sentence intact, cost and net
both rendered as dashes, zero fabricated per-service rows, the
withholding explained, no CSP violation and no JS error.

* feat: say what a deploy will do, and ask before it earns nothing

The last two orphaned endpoints, and the worst place for them to have
been silent: the backend knew that a second instance behind one IP can
make a provider forfeit the account balance, and the deploy button
never mentioned it.

deploy-risk renders in the service modal — what running this does with
the connection, and whether it can be kept off the LAN. "Nobody has
documented this" is rendered as prominently as a known risk, because
undocumented is not safe; collapsing that distinction would undo the
design of lan_isolation.py, which goes out of its way to preserve it.

preflight now runs at the deploy step, which is where the backend's own
comments say it belongs rather than buried in a FAQ. It WARNS and never
blocks: the assessment reports blocking=false and says "you can deploy
it anyway", and the person at the keyboard knows things CashPilot does
not. Only findings that say the service will earn nothing interrupt —
advisory notes on every deploy would train people to click through the
one that matters. An unreachable preflight proceeds, because failing to
fetch advice is not grounds to refuse a deploy the user asked for.

Verified over CDP in three cases, which is what it takes to show a gate
works rather than merely exists:

  objection, declined : asked, confirmed, NOT deployed
  objection, accepted : asked, confirmed, deployed
  no objection        : not asked, deployed

A gate that always fires is a broken button; one that never fires is
decoration. Both halves are asserted.

The check needed two corrections of its own. It first called the deploy
function cold, which hit the "select a worker" guard and proved
nothing — so it now drives the real modal, ticking the checkbox the
user would tick. Then the accepted case deployed to the only free node
and the next case found every checkbox disabled: the test consumed its
own fixture. The third case uses a different service for that reason.

* test: cover the catalog-driven branches that were only ever proven by hand

codecov/patch was red, and it was right. Four lines this branch added
had no test: the three per-node-earnings branches and the credential
hint assignment. I had verified all of them in a scratch script when
writing them and never committed the check — which is the same as not
having one, because a path proven once and never again silently rots.

Covered now: a service that declares per_node_earnings gets its rows; a
service that does not declare it gets nothing; an unknown slug gets
nothing; a service that declares the capability without implementing it
warns and returns empty rather than 500ing; missing credentials return
empty rather than calling a collector with empty auth and surfacing a
provider error as if the feature were broken; and the 13 credential
hints are still served from YAML with their text intact.

Fixed by writing the tests rather than by telling codecov to ignore
frontend paths. The frontend genuinely is uncovered by pytest — that is
what the browser harnesses are for — but these four were Python, in
reach, and simply untested.

* fix: degrade on a provider failure, and put the rel where it survives

Two findings from review, one of which needed fixing somewhere other
than where it was reported.

api_per_node_earnings awaited the collector without catching. That call
reaches a THIRD-PARTY API which times out, rate-limits and returns HTML
instead of JSON on a bad day — none of it a fault in CashPilot, all of
it arriving as a raw 500. Every other collector call in this file
degrades; this one did not. An empty per-node breakdown alongside the
account total is a smaller loss than a broken page. The collector is
still closed on that path, so a flaky provider cannot leak a session
per request.

The review also asked for rel="noopener noreferrer" on the credential
hint anchors, listing five service YAMLs. Both parts needed correcting.

It is 13 files, not 5 — every migrated hint carries a target='_blank'
anchor. And writing the rel there would have done NOTHING: sanitizeHint
strips every attribute it does not explicitly keep, so the rel would
have been removed on its way to the DOM. The fix would have looked
applied, passed review, and had no effect.

So the rel is set by the sanitiser, after the stripping rather than
before it, which covers all 13 hints and every future one and cannot be
forgotten by whoever writes the next. Verified in a real DOM against
the real shipped hint: the anchor comes out with the rel; a rel written
in the YAML is overwritten rather than honoured; a link with no target
gains nothing; and a javascript: href, an onclick and a script tag are
all still stripped. That harness is committed, because the property it
checks is invisible in the source.

Worth being accurate about the severity: modern browsers imply noopener
for target=_blank, so this was defence in depth rather than a live
hole. noreferrer is the part still worth having — these links point at
a provider dashboard and the referrer would otherwise name the user's
CashPilot host.

* fix: a brand-new user could not create an account at all

The onboarding page — where a fresh install redirects you — had no
field for the one-time setup token, and never sent one. /register
requires it on first run, so every new installation answered the form
with a 403 and the message "Registration failed. Please try again."
Trying again did the same thing. Forever.

This is not a regression from this branch. v1.10.1 ships it: `git show
v1.10.1:app/templates/onboarding.html` contains no setup_token at all.
The gate landed in PR #100 and the page it gates was never updated. It
survived because existing installs were unaffected — the people who
could have noticed already had accounts, and every test in the suite
creates its owner by calling database.create_user directly, so the
actual install flow had never once been exercised.

Found by the user-story audit asking the obvious question nobody had
asked: can a new user actually install this?

Fixed: the field, the value in the POST, and the message. A 403 here
has exactly one cause, so it now says so and points at
`docker logs cashpilot-ui` and the log line to look for, instead of
suggesting a retry that cannot work.

Verified by driving a genuinely fresh instance in a browser — land on
/, follow the redirect, fill the form, press the button — and then
asking the SERVER whether the account exists rather than trusting the
page. Removing the token from the POST turns that check red with a 403.
The harness is committed, since the suite could not see this and would
not have.

The pytest additions carry their own correction: the first version of
the gate test passed a wrong token and was told "allowed", which looked
like a security hole. It was an unarmed fixture — conftest clears the
token global before every test and verify() returns True when no token
is active. Now armed, with a positive case so the two negative ones
cannot pass with the gate welded shut.

* fix: net earnings charged no electricity at all, and called it known

/api/earnings/net filtered running containers on c["service"], while
_get_all_worker_containers emits "slug". It therefore matched NOTHING
in production, every time.

The consequence was not a missing panel. Every service was charged 0 W,
so cost came out 0.00 and net was reported EQUAL TO GROSS with
cost_known: true. The endpoint's docstring promises it reports net
alongside gross and never presents earnings as profit; it did exactly
that, and said the cost was known while doing it.

Measured on a seeded instance before and after:

  before   gross 4.00   cost 0.00   net 4.00
  after    gross 4.00   cost 3.97   net 0.03

The electricity was consuming almost the entire gross. A user reading
this would have concluded a service was worth running when it was
barely breaking even.

This is the SECOND time this key has bitten the project.
egress.container_slug() exists because of the first time and accepts
both spellings; its docstring says code reading "service" matched
nothing in production while its tests passed. That is precisely what
happened again, in a function written after the helper existed. Both
readers in this endpoint now use it, so the legacy shape still works.

The regression tests carry their own correction. The first negative
control reported PASS with the fix reverted, which would have meant the
tests were worthless — in fact `ruff format` had collapsed the
comprehension onto one line, so the revert patch never matched and the
control was a no-op. Re-run against the real text, it fails exactly the
two tests that name the bug. A control that cannot fail proves nothing,
and one that silently does not apply is worse than none.

* test: nothing parsed the JavaScript, so a dead dashboard shipped green

Demonstrated rather than argued: appending `function broken( {` to
app/static/js/app.js leaves the entire suite green — 2306 passed — and
CI green with it, because nothing in test.yml so much as looks at a .js
file.

What ships in that state is a completely dead dashboard. app.js is one
IIFE assigned to CP, so a parse failure anywhere in its 2900 lines
means CP is never defined; delegate.js falls through to
`typeof CP !== 'undefined' ? CP : {}` and every data-action button logs
"No handler named X" and does nothing. No error page, no failed
request, no alert — a UI where nothing works and nothing says so.

That risk grew with this release rather than shrinking: 1.11.0 moved
the payout queue, payout progress, credential health, running costs and
the deploy-risk gate into that file, and every one of them is reachable
only through CP. The audit was right to call this the highest-leverage
gap it found.

Now gated in both places. pytest runs `node --check` over every JS file
so it fails locally, skipping only where node is absent, since a missing
toolchain is not a code defect. CI installs node and runs the same
check as a hard step, because a skip is not a check.

CI also runs scripts/currency_check.mjs, which needs no browser and
pins behaviour pytest cannot see — it exists because formatCurrency
once labelled an unconverted USD amount with the viewer's currency
symbol, rendering $24.90 as "£24.90".

The three committed browser harnesses are asserted to exist and to be
capable of failing, so deleting one is a red test rather than a silent
loss of the only check on that behaviour.

* fix: the wizard selected services invisibly, and the chart invented zeros

Two audit findings, the same mistake in different costumes: the code
had no value, so it made one up.

The wizard's service cards passed `data-a2="this"` — a leftover from
the inline-handler migration, where `this` really was the element.
delegate.js reads arguments out of `dataset`, and dataset values are
always strings, so the handler received the literal string "this" and
threw on `.classList`.

The throw was the harmless half. The selection is mutated BEFORE the
line that throws, so a click added the service and then died before
highlighting anything. Nothing appeared to happen; clicking again — the
natural reaction — silently removed it; and the user then pressed Next
and was told to select a service. Demonstrated by running delegate.js's
own argument extraction: args are ["honeygain", "this"], the call
raises TypeError, and selectedServices already contains the slug.

The handler now resolves its own element, because an element cannot
travel through a data-* attribute and pretending otherwise is what
caused this.

The earnings chart, on a failed fetch, drew one bar per day at exactly
0.00 with real-money axes and tooltips. The user read "I earned nothing
every day for a month" when the browser could not reach the server. Of
everywhere in this app that could render unknown as a number, this was
the biggest on screen. It now leaves an existing chart alone — stale
real data beats invented data — and otherwise says the figures could
not be fetched and that this is not a reading of zero.

The guard against the data-a2 class needed fixing too, and this is the
FOURTH time in this session that a text-matching check has fired on the
comment explaining the thing it forbids. Rewording the comment each
time makes the code worse to read in order to keep a weak test green,
so the scan now strips comments first. Reintroducing the real markup
still fails it; the explanation no longer does.

* fix: a live credential was written to the container log, twice over

Several providers authenticate with a bare header value — Salad's auth
cookie, Grass's access token, ProxyRack's API key, EarnApp's OAuth
token, PacketStream's JWT. When httpx rejects one, the exception TEXT
is the credential. Two independent sites printed it in plaintext.

app/main.py logged result.error raw, one line ABOVE the comment
explaining that collector errors must be redacted because "an httpx
exception embeds the offending header or URL — which for several
providers is a live credential". The alert built from that same string
was carefully sanitised; the log line above it was not, which is what
made this easy to miss for so long.

All sixteen collectors then did it again, independently:
logger.error("X collection failed: %s", exc, exc_info=True) — the
message leaking it once and the chained httpcore/httpx traceback
repeating it. Fixing main.py alone would have left that fully intact.

The result went to stdout, so into `docker logs cashpilot-ui` and into
whatever ships those logs off the box — Loki, journald, the Unraid
syslog. Anyone in the docker group could read a live provider
credential.

notify.redact() already existed and already worked; it was simply
applied one step too late. It is now applied before both log calls, and
the collectors route through a single base.log_failure() helper so a
new collector cannot reintroduce this by copying the idiom from its
neighbours — which is exactly how it reached all sixteen.

Tested with a distinctive fake JWT asserted to appear nowhere in
captured log output, which is the only formulation that cannot be
satisfied by redacting in the wrong place. exc_info is checked with an
AST walk rather than a grep, because the helper's own docstring quotes
the idiom it forbids and a text scan would flag the explanation.

Also fixes MystNodes' per-node fetch, which had the same shape and was
not in the audit's list.

* style: apply ruff format to the redacted log call

ruff check was green and ruff format --check was not. CI runs both, and
my final gate only ran the first — the exact gap I have hit on this repo
before.

* fix: a token price move invented earnings for anyone-protocol

The collector reported balance in USD, converted from the token count
at collection time. balance is CUMULATIVE lifetime rewards and earnings
are the delta between two readings, so pricing before the subtraction
turned an exchange-rate movement into income: the same 1000 ANYONE read
at $0.10 and then at $0.12 produced $20 "earned today" without a single
token being paid out — and would have reversed into an apparent loss
when the price fell back.

get_daily_earnings already takes the delta in the native currency and
prices it afterwards. That is the whole reason it is written that way,
and handing it USD defeated it for this one service.

It now reports native ANYONE, with the token mapped in CRYPTO_IDS so
the existing machinery prices it. The CoinGecko id really is
"airtor-protocol" — the project renamed from ATOR and the slug did not
follow — taken from the collector, which had been querying exactly that
id all along. Its private price fetch is now dead and removed.

Safe for existing installs: database.py only takes a delta when the
stored currency matches the previous reading, so the USD-to-ANYONE
changeover is skipped rather than counted as a 900-unit gain. Asserted,
because that is the property that makes this shippable rather than a
migration.

The contract fixture already declared currency ANYONE. It was right and
the code disagreed with it; the fixture only needed its now-stale "usd"
field dropped.

Two of my own tests needed correcting before they were worth anything.
They first read r["earnings"] keyed by ISO date while get_daily_earnings
returns {"date": "Aug 02", "amount": ...} — every lookup returned 0.0,
so three assertions expecting 0.0 passed without touching the code they
name. Then a regex fix swallowed a @pytest.mark.asyncio decorator into
an assert message, which stayed green only because an assert's message
is never evaluated while the assertion holds.

* fix: the shared fleet key could permanently delete payout records

require_role granted the fleet role writer on every writer check. The
comment justified it as being "for heartbeat/status endpoints" — but
the heartbeat never went through require_role at all.
api_worker_heartbeat authenticates with _authenticate_worker_heartbeat,
so the grant gave workers nothing they needed while unlocking five
user-facing routes: payout confirm, payout REJECT, container logs,
collection, and arbitrary worker commands.

Payout rejection is a hard DELETE FROM payouts. The route's own comment
says "this mutates financial records, and rejection is a hard DELETE. A
read-only account must not be able to destroy them."

CASHPILOT_API_KEY is handed to every worker host by design — docs/fleet.md
puts it in each worker's compose environment — so it is readable by any
machine in the fleet, by anyone who can `docker inspect
cashpilot-worker`, and by anyone who can read the shared /fleet volume.
docs/upgrade-v1.md already claims the shared key is "enrollment/bootstrap
only" and "stops being a fleet-wide credential once a worker is
enrolled". The code never implemented that.

Verified against a running server rather than argued. Before: a bare
`Authorization: Bearer <fleet key>` POST to
/api/earnings/payouts/1/reject returned 200 and deleted the row, with no
session and no account. After: 403. Worker commands likewise 403. The
heartbeat still returns 200, which is what made the removal safe.

Automation that genuinely needs write access already has
CASHPILOT_ADMIN_API_KEY, which resolves to owner and is not distributed
to workers.

tests/test_auth.py had a test asserting the vulnerable behaviour
(test_fleet_satisfies_writer). It is now inverted, with the reason
recorded, plus an end-to-end class that calls the real handlers through
the real guard — the unit test alone could not have caught this, since
require_role is a pure function and the damage only appears when you ask
what a request carrying that key can reach.

That end-to-end class needed a correction of its own: it first patched
app.main.auth_module, which does not exist, so the three control cases
errored while the two "fleet is refused" cases passed — passing because
everything failed, not because the guard worked. Patched at
app.deps._require_auth_api instead, so the real require_role runs.

* fix: ten verified audit findings (beads batch 1) (#182)

* fix: ten verified audit findings — honest defaults, working links, real gates

Batch 1 of the bead backlog. Every one was independently re-verified
against this branch before being touched: the audit's own adversarial
pass had already refuted 13 of 94 findings, so none of these was taken
on trust.

CashPilot-c1q / CashPilot-i21 (one defect, filed twice from two areas)
  The config.updated_at migration back-filled every existing row with
  datetime('now'), so an upgraded volume reported every credential as
  brand new — including a Bytelixir session cookie that expires after
  two hours and had in fact expired days earlier. Unknown age rendered
  as the most favourable known age. NULL is the honest value and both
  consumers already skip it.

CashPilot-153  bool() on a TEXT config value made "false", "0" and "no"
  all mean True. That flag decides whether the fleet page says "turning
  it off would save that" or "this machine would be on anyway" —
  opposite advice from the same setting.

CashPilot-a02  electricity_price_per_kwh was honoured by the fleet
  endpoint and ignored by the dashboard one, so an upgrading user saw
  running costs on one page and "cost unknown" on the other from a
  single stored value. Both now resolve through _tariff_price.

CashPilot-4ks  /docs, /redoc and /openapi.json were served
  unauthenticated. The schema is a complete map of the admin surface,
  including the worker and payout endpoints. Disabled.

CashPilot-0id  The Configured badge used `.some`, so one of two required
  credentials was enough to show it — while make_collectors skipped that
  service for the missing field and it silently never collected. Now
  requires every REQUIRED field (optional ones must not block it) and
  has a distinct Incomplete state.

CashPilot-0rw  The credential hint rendered only in the modal, though
  services/_schema.yml says it appears "next to the input in Settings" —
  the one screen a tracking-only user uses. Rendered through
  sanitizeHint, since the hints contain anchors.

CashPilot-x2r  entrypoint.sh is the ENTRYPOINT of both images and was in
  neither the release paths filter nor either detect regex, so changing
  the privilege-drop and Docker-socket-group script published nothing.

CashPilot-w3g  An unwritable key file marked the encryption key
  ephemeral even when it came from CASHPILOT_ENCRYPTION_KEY — a value
  identical on every restart. Startup then refused and told the user to
  supply a key via CASHPILOT_ENCRYPTION_KEY, which they already had.

CashPilot-ecc  The Salad credential hint pointed at app.salad.com, which
  does not resolve. The working host was already in the same file three
  lines above.

CashPilot-i5l  CLAUDE.md described a synthetic first-earnings baseline
  row. No such code exists anywhere in app/. The documented outcome is
  delivered by the no-predecessor rule instead, so the doc was corrected
  rather than the code.

39 regression tests, each naming its bead. Negative controls run:
reverting the docs and dedicated fixes turns four of them red.

* fix: make the key-persistence message match its actual consequence

Both findings from CodeRabbit on this PR, both correct.

The fix stopped treating an env-supplied key as ephemeral but left the
error text saying credentials 'will be unreadable after a restart'.
That is untrue while CASHPILOT_ENCRYPTION_KEY is set, and it is exactly
the kind of false alarm that teaches people to ignore the real one. A
minted key keeps the loud error; a supplied key gets a warning that says
what actually matters — keep the variable set, because without the
cached file it is the only copy.

The comment above the config.updated_at migration still described the
back-fill this PR removes, which would have invited someone to restore
it.

The batch's own count guard fired on the new test class. That is the
guard working: review-response classes are now excluded by name rather
than by raising the number, so a dropped bead fix still shows up.

* fix: a service nothing has read is not a service earning zero (#183)

Batch 2. Eight beads, two fixes — three audit areas each filed the same
two defects, and both are one mistake wearing different clothes: code
with no value supplying a confident one.

CashPilot-vp6 / -ikh / -7qk
  `balance_map.get(slug, 0.0)` turned "no reading" into $0.00 on both
  the container and external paths. Nothing else caught it:
  collector_disconnected needs an alert (none was raised, because no
  collector ran), collector_needs_setup only checks whether config keys
  are filled, and the flatline detector explicitly skips rows whose max
  balance is 0. So the dashboard showed a service Running with a balance
  of $0.00 while the bell said "All collectors healthy" — three
  independent signals all agreeing on something none of them had
  checked.

  The API now sends balance: null plus balance_known, and the row
  renders an em dash. The renderer's `|| 0` had to go with it: it would
  have coerced the new null straight back to a confident zero and undone
  the fix while every test still passed.

CashPilot-3oa / -jkd / -s2b
  The payout-progress card asserted a definite 0.00 lifetime and a
  0%-of-minimum bar for a service never read. It already got "Balance
  now" right via balance_known, which is what made the two fabricated
  figures beside it read as corroborated. The card now hides when there
  is neither a balance nor a confirmed payout, and lifetime_earned is
  null rather than a folded zero.

One correction to my own change: I also nulled `projection`, and an
existing test objected. It was right. payouts.project already returns an
explicit NOT_ENOUGH_DATA state, which says WHY there is no estimate —
strictly more useful than null, and nulling it pushed a null-check onto
every caller. The misleading part was never that field but the card
drawing a 0% bar from it, which is fixed where the card is drawn.

Verified with a negative control: restoring the 0.0 default turns the
API test red.

* fix: a provider changing its API shape is an error, not zero earnings (#184)

Batch 3. Seven beads.

CashPilot-y5t — an interrupted upgrade bricked startup permanently.
  The workers table rebuild ran through executescript, which COMMITS
  PER STATEMENT, with no DROP IF EXISTS and no transaction. An
  interruption between CREATE and RENAME left workers_new behind for
  good; the guard above it was still true on the next boot, so the
  rebuild re-ran, the CREATE failed with "table already exists",
  init_db raised, and the app never started again.

  Reproduced before the fix on a seeded pre-client_id volume:
  OperationalError: table workers_new already exists — on every
  subsequent restart. Now re-entrant, and the worker rows survive.
  The leftover is always disposable: it is only ever a partial copy,
  and the real table is intact until the DROP.

CashPilot-3yn / -rtb (Storj), -10d (Anyone), -fzw (Grass)
  All three read provider payloads with .get(key, default), so a
  renamed or missing field became a confident 0.00 with no error. That
  zero was written as a real balance: no alert (there was no error), no
  flatline warning (that detector explicitly skips rows whose max
  balance is 0), nothing on screen to distinguish it from a service
  that genuinely earned nothing.

  Storj is the worst of the three, because its zero is a DROP from the
  previous reading — so _detect_payout asked the user to confirm
  receiving their entire balance, money that was never paid, and a
  confirmed payout is permanent.

  Each now distinguishes an absent KEY from a genuine zero: "0" and
  "null" still mean zero for Anyone, an empty device list still means
  zero for Grass, and Storj still sums whichever payout fields are
  present. Only a shape it does not recognise raises, and the message
  names the service and what it actually saw.

CashPilot-0ja — /api/preferences spawned a full fleet collection for
  any authenticated user, while /api/collect gates the identical call
  behind _require_writer. Two doors, one side effect, different locks;
  a viewer could hit every provider API on demand. The preference still
  saves for a viewer — only the collection is gated, so completing
  setup is not blocked.

CashPilot-dbh — both Deploy buttons were enabled for writers, while
  api_deploy requires owner. A writer filled in provider credentials
  and only then got a 403. The buttons now match the server.

Negative controls run: reverting the migration guard fails three tests,
reverting the collection gate fails one.

* fix: totals that were wrong, not merely missing (beads batch 4) (#185)

* chore: drop an empty test file that slipped into batch 1

tests/test_security_headers.py was a 0-byte stray from an interrupted
heredoc earlier in this work. It was untracked for a long time and got
swept into the batch-1 commit by a broad 'git add tests/'. An empty test
file contributes nothing and implies coverage that does not exist.

* fix: three totals that were wrong rather than merely missing

Batch 4.

CashPilot-47t — the dashboard Total silently dropped whole holdings.
  exchange_rates.to_usd consults only the LIVE caches, so a crypto
  whose rate lookup was merely stale vanished from the headline figure
  with nothing on screen saying it was incomplete. In a seeded run, 110
  MYST disappeared entirely; it now reads 13.20 from the rate the
  reading was recorded at.

  This is not a new rule invented here. get_earnings_dashboard_summary
  and get_earned_by_platform already price rows exactly this way, and
  upsert_earnings stores fx_rate_usd on every row specifically so "the
  USD value of a historical non-USD reading cannot be reconstructed
  later" without it. The Total was the one figure applying a stricter
  rule than the cards beside it, which is why they disagreed. It now
  uses database._usd_rate, the same validator, so a 0, negative, inf or
  nan rate is still rejected rather than believed.

  The query had to select fx_rate_usd at all — the column never left
  SQLite for this path, so the value was unreachable.

  What still cannot be priced is now REPORTED as unpriced_platforms
  rather than logged and forgotten (CashPilot-oj4): a total that
  silently omits holdings is indistinguishable from a correct one.

CashPilot-7oj — month_change was a hardcoded 0.0 that nothing computed,
  rendered as "+0.0%" in the positive style forever. It is now null, and
  setChangeIndicator renders an unmeasured value as blank instead of
  throwing on it.

CashPilot-he1 — fleet net subtracted the cost of SOME machines from the
  gross of ALL of them, so the more machines you could not price, the
  healthier the fleet appeared. Net now compares like with like; gross
  still covers the whole fleet, and cost_known_for still says how many
  were measured.

The 47t helper is module-level rather than a closure: defined inside the
loop it captured `currency` late (ruff B023), so every conversion would
have used the last currency seen — silently worse than the bug.

Negative controls: reverting he1 and 7oj turns three tests red.

The batch-4 tests strip comments before scanning source. This is the
fifth time a text-matching assertion has matched the comment explaining
the thing it checks — here the comment mentions `pct.toFixed` while
warning it would throw, so an ordering assertion found the prose first
and failed on correct code.

* fix: an owner adding a user was logged into that user's account (#186)

Batch 5. Four beads.

CashPilot-5e6 — do_register minted a session for the account it had
  just created and set it as the CALLER's cookie. An owner adding a
  viewer was silently demoted into that viewer's session: their own
  access gone, with no sign anything had happened beyond landing on a
  page with fewer controls. Only the first-run owner is signed in here
  now, which is the case this code was written for; adding a user
  returns to /users with the caller's session untouched.

CashPilot-zcc — the UI signed outbound worker commands with a key the
  worker had never confirmed. get_worker_key discards the confirmed
  flag that get_worker_key_state returns, and the unconfirmed state is
  reachable whenever the worker fails to persist its key and logs
  "staying on shared key". The worker then read as ONLINE from its
  heartbeats while every deploy, start, stop, restart and logs call
  failed 401 — two symptoms with nothing on screen connecting them. It
  now falls back to the shared key until the worker confirms.

CashPilot-cxy — per-worker watts and dedicated settings were keyed on
  the AUTOINCREMENT row id. delete_worker removes the row and nothing
  else, so a host removed and re-enrolled came back under a new id,
  silently orphaning its settings while the old keys lingered forever.
  Now keyed on client_id, which is UNIQUE and survives re-enrolment,
  with the id-based key still honoured for existing volumes.

CashPilot-153 (this branch's half) — the same helper also fixes bool()
  on a TEXT config value, where "false", "0" and "no" were all truthy.

Two existing tests asserted the old worker-auth contract and were
updated rather than worked around; a third case was added for the
unconfirmed key, which is the state the bug lived in.

Negative controls run for both behaviour changes. The first attempt at
the 5e6 control PASSED — because my revert patch did not match and
silently did nothing. Re-done against the real text, it fails the two
tests that name the defect.

* fix: assert the salad host by parsing it, not by substring

CodeQL flagged two high-severity 'incomplete URL substring sanitization'
alerts on this branch, both in the test added for CashPilot-ecc. It was
right, and not only formally: "app.salad.io" in text also passes for
app.salad.io.evil.com and for evil.com/?next=app.salad.io. A test whose
job is to pin which host we send users to should not accept a host that
merely contains the right string.

Both assertions now parse each URL and compare hostname equality.
Verified by planting app.salad.io.evil.example in salad.yml: the old
assertion accepted it, the new one fails on it.

* test: assert the salad host with explicit equality, not membership

The last CodeQL alert on this branch. Membership on a list IS exact, but
the scanner cannot distinguish `x in hosts` from a substring test
against a string and flags the shape either way.

Rewritten as a count of hosts equal to the expected one. That is
unambiguous to both the reader and the scanner, and it asserts something
slightly stronger: that the host actually appears, rather than that
something merely matched. Still verified against a planted
app.salad.io.evil.example, which it rejects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant