From d27ab6a8aaf4146f37044d618f935cf3d76119a6 Mon Sep 17 00:00:00 2001 From: John Osumi <931193+sumitake@users.noreply.github.com> Date: Tue, 4 Aug 2026 12:18:40 -0700 Subject: [PATCH 1/2] client: add cloud transport fallback to BusyBarClient (v1.6) Adds an optional cloud relay fallback for BusyBarClient, mirroring busylib-py's single-class transport-flag design, so an integration keeps working when the local device (USB/Wi-Fi) becomes unreachable, as long as the operator has configured a BUSY cloud API token. New [device] config: cloud_token = "" (empty disables cloud fallback entirely -- out-of-the-box behavior is byte-for-byte unchanged from pre-v1.6), cloud_base_url (default https://api.busy.app/busybar), transport = "auto" | "local" | "cloud". Key names were chosen to match BusyBarClient's constructor kwargs exactly, so both integration call sites simplify to BusyBarClient(**cfg["device"]). In "auto" mode (the default), _request tries local first with the existing (3, 5) timeout; on a requests.RequestException with a non-empty cloud_token, the same request is retried against cloud_base_url with a (5, 15) timeout and an Authorization: Bearer header. A per-client active_transport attribute tracks which transport last succeeded; while degraded, subsequent calls skip the local attempt entirely and go straight to cloud until LOCAL_RETRY_SECONDS (60) have elapsed since the last local failure, at which point local is retried first again as a recovery probe -- cheap to do inline (no background thread) since a down local device fails fast. DrawResult.UNREACHABLE now means both transports failed when cloud is configured; unchanged (local-only) semantics when it isn't. Local paths ("/api/...") map to cloud paths by stripping the "/api" prefix and letting cloud_base_url (which already carries "/busybar") supply the rest. DrawResult's members, all six public BusyBarClient methods, and both integrations' logic are unchanged -- the fallback lives entirely inside the private _request/_try_local/_try_cloud layer. Per the coordinator's explicit security requirement, cloud_token is never logged at any level including DEBUG -- only transport transitions are logged (INFO, static strings, no interpolation). Verified both by a manual grep of every log call site in client.py and by a dedicated caplog-based regression test. No live cloud verification was performed this round -- the operator hadn't provisioned a real token yet, per explicit instruction. Tests use a literal placeholder token string only; no network call was made to any busy.app host. README's new "Cloud transport" section documents the token creation/rotation walkthrough and the post-merge live-probe checklist (forced-cloud draw probe, cadence headroom check at 10s redraws, and resolving the api.busy.app/busybar vs. busylib-py's proxy.busy.app base-URL discrepancy) for the controller pass that runs once a token exists. 323 -> 337 tests passing (14 net-new: 12 in test_client.py covering fallback/no-fallback/forced-mode/request-shape/recovery-probe-timing/ token-never-logged, 2 in test_config.py for config.example.toml/DEFAULTS parity on the new [device] keys). Co-Authored-By: Claude Fable 5 --- README.md | 78 ++++++++ config.example.toml | 21 +++ ...6-08-03-calendar-ci-integrations-design.md | 151 +++++++++++++++ integrations/calendar_countdown/main.py | 2 +- integrations/ci_status/main.py | 2 +- src/busybar/client.py | 117 +++++++++++- src/busybar/config.py | 12 +- tests/test_client.py | 175 ++++++++++++++++++ tests/test_config.py | 30 ++- 9 files changed, 581 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index ced8401..fe4cd54 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,84 @@ To add a new integration: - The repo ships only `config.example.toml`, documenting all fields and defaults. - CI/CD can inject secrets via environment-variable expansion in config parsing if needed. +## Cloud transport + +By default, `BusyBarClient` talks to your BUSY Bar directly over the LAN +(`[device].host`). As of v1.6, it can automatically fall back to BUSY's +cloud relay if the local device becomes unreachable — USB unplugged, +Wi-Fi drop, the device off — and recover back to local on its own once +it's reachable again. This is entirely optional and off by default. + +### Setting it up + +1. Create a token at [cloud.busy.app](https://cloud.busy.app) → **API + tokens** tab → create a new token with the **"BUSY Bar"** scope. This + scope grants full control of exactly one linked device — there's no + separate device ID to configure; the token itself identifies which + device it talks to. +2. Add it to your `config.toml` (**never** `config.example.toml`, and + never anything committed to the repo — see "Configuration" above): + ```toml + [device] + host = "10.0.4.20" + cloud_token = "paste-your-real-token-here" + ``` +3. Optionally set `transport` (default `"auto"`): + - `"auto"` — local first, cloud fallback when the local device is + unreachable and `cloud_token` is set. Recovers back to local + automatically. + - `"local"` — local only, never falls back (identical to pre-v1.6 + behavior; the default if you never set `cloud_token`). + - `"cloud"` — forced cloud only, never attempts local. Mainly useful + for deliberately exercising/debugging the cloud path. +4. `cloud_base_url` defaults to `https://api.busy.app/busybar` and + normally doesn't need to change — see the base-URL note below. + +### Rotating or revoking a token + +Manage tokens from the same **API tokens** tab on cloud.busy.app. +Revoking a token takes effect **immediately and cannot be undone** — if +you're rotating, create and deploy the replacement token first, then +revoke the old one, rather than revoking first. + +### What cloud fallback does NOT cover + +Continuous status streaming (`/api/status/ws`) is local-only by design — +the cloud API has no equivalent, so a caller relying on the status +WebSocket will not get a cloud fallback for it. Everything else this +client uses (`draw`, `clear`, `status`, `get_busy`, `set_busy_simple`, +`play_audio`) is a synchronous request/response call and mirrors 1:1 +over cloud. + +### Post-merge live-probe checklist + +This round's tests are entirely mocked — no cloud request has been made +against a real token, since the operator hadn't provisioned one yet. +**Before relying on cloud fallback in practice**, run this checklist +once a real `cloud_token` is in `config.toml`: + +1. **Forced-cloud draw probe.** Set `transport = "cloud"` temporarily + and run a `client.draw(...)` (e.g. via either integration's + `--once --dry-run=false` path, or a one-off script) to confirm the + token is valid and a real device draw round-trips over the cloud + relay end to end. +2. **Cadence headroom check.** No rate limit or cadence guidance is + documented anywhere for the cloud API (see + `scratchpad/busy-cloud-api-research.md`'s "Open items"). Run + `calendar_countdown` (10s ambient redraw cadence) forced onto cloud + transport for a few minutes and confirm no throttling/errors show up + before trusting cloud fallback to hold up under sustained polling. +3. **Base-URL ambiguity.** This codebase defaults `cloud_base_url` to + `https://api.busy.app/busybar`, but busylib-py's own hardcoded default + is the differently-hosted `https://proxy.busy.app` — an unresolved + discrepancy in the source research, not something this round's mocked + tests can settle. Confirm which base actually works against a live + token (or whether both do) and update the default/docs here if + `api.busy.app/busybar` turns out to be wrong or non-canonical. + +Set `transport` back to `"auto"` (or leave it, since `"auto"` is the +default) once the checklist above passes. + ## What's inside | Integration | Description | diff --git a/config.example.toml b/config.example.toml index cdd69b2..6bb5e19 100644 --- a/config.example.toml +++ b/config.example.toml @@ -3,6 +3,27 @@ [device] host = "10.0.4.20" # USB-Ethernet default; set your LAN IP for Wi-Fi +# Cloud transport fallback (v1.6, optional). When cloud_token is set, +# BusyBarClient automatically falls back to BUSY's cloud relay if the +# local device becomes unreachable (USB unplugged, Wi-Fi drop, etc.), and +# recovers back to local automatically once the device is reachable +# again. Leave cloud_token empty (the default) to disable cloud fallback +# entirely -- behavior is then identical to pre-v1.6. +# +# Mint a token at https://cloud.busy.app -> "API tokens" tab -> create a +# token with the "BUSY Bar" scope (full device control, tied to exactly +# one linked device -- no separate device id needed here). Revoking a +# token from the dashboard takes effect immediately and cannot be undone; +# create the replacement token first if you're rotating. +# +# The real token belongs ONLY in your git-ignored config.toml -- never +# here, and never committed anywhere. See README.md's "Cloud transport" +# section for the full walkthrough. +cloud_token = "" +cloud_base_url = "https://api.busy.app/busybar" +transport = "auto" # "auto" (local, fall back to cloud) | "local" | "cloud" (forced -- + # mainly for deliberately testing the cloud path) + [calendar_countdown] poll_seconds = 10 # ambient-tier redraw cadence (default: 10) -- matches the running-CI # overlay's 10s dwell gap so this app's redraws reliably land inside diff --git a/docs/superpowers/specs/2026-08-03-calendar-ci-integrations-design.md b/docs/superpowers/specs/2026-08-03-calendar-ci-integrations-design.md index a4b8a95..870e04b 100644 --- a/docs/superpowers/specs/2026-08-03-calendar-ci-integrations-design.md +++ b/docs/superpowers/specs/2026-08-03-calendar-ci-integrations-design.md @@ -1460,3 +1460,154 @@ operator directly ear-verified the corrected `.snd` call as part of reporting the original bug, which is stronger evidence than a fresh scripted probe could add (a scripted probe can only confirm HTTP `200` again, exactly the signal already shown not to prove audibility). + +## 2026-08-04 — v1.6 cloud transport fallback + +**Status:** Implemented, branch `dev/claude/cloud-transport-v1.6` off +`main` (top of `main` at branch time: PR #13, the chirp-`.snd` fix). Not +pushed. **No live cloud verification performed this round** — the +operator hadn't provisioned a real `cloud_token` yet; see the README's +"Post-merge live-probe checklist" for what runs once one exists. + +Operator-approved feature, based on `scratchpad/busy-cloud-api-research.md` +(source citations: `busy-app/busylib-py` on GitHub, docs.busy.app, and the +live cloud OpenAPI spec at `api.busy.app/busybar/openapi.yaml`). BUSY +exposes a cloud relay in addition to the device's local HTTP API — same +endpoint surface, mounted under `/busybar/...` instead of `/api/...`, bearer- +token authenticated, synchronous (the relay bridges the HTTP call over MQTT +to the device and holds the connection until it replies, so callers see the +same 200/409 semantics as local — no async/poll pattern to add). One +confirmed gap: `/api/status/ws` (continuous status streaming) has no cloud +equivalent — local-only by construction, not something this round could +paper over. + +### `BusyBarClient` transport layer (`src/busybar/client.py`) + +Single-class transport-flag design, mirroring busylib-py's own pattern +(one class, a mode flag branching request construction) rather than a +class hierarchy — matches this codebase's existing single-`BusyBarClient` +shape and needed no changes to `DrawResult`'s enum members or to any +consumer's call to `draw`/`clear`/`status`/`get_busy`/`set_busy_simple`/ +`play_audio`; the fallback lives entirely inside `_request`. + +New constructor kwargs: `cloud_token` (default `""`, disables cloud +fallback entirely when empty), `cloud_base_url` (default +`"https://api.busy.app/busybar"`), `transport` (`"auto"` | `"local"` | +`"cloud"`, default `"auto"`), `cloud_timeout` (fixed `(5, 15)`, not +config-exposed — longer than local's `(3, 5)` since a cloud round-trip +crosses the public internet and bridges over MQTT to the device rather +than a direct LAN hop). + +- **`"local"`**: unchanged pre-v1.6 behavior exactly — `_request` calls + local only, never falls back, regardless of `cloud_token`. +- **`"cloud"`**: forced — `_request` calls cloud only, never attempts + local. For deliberately exercising/debugging the cloud path (e.g. the + live-probe checklist below). +- **`"auto"`** (default): local-first-with-cloud-fallback. + `active_transport` (`"local"` | `"cloud"`) tracks which transport last + succeeded. On a fresh/healthy client, every call tries local first with + the existing `(3, 5)` timeout; on a `requests.RequestException` AND a + non-empty `cloud_token`, the SAME request (same method, path, body) is + retried against `cloud_base_url` with `cloud_timeout` and an + `Authorization: Bearer ` header. A successful cloud call + transitions `active_transport` to `"cloud"` (logged once at INFO, + transition only — not on every request while already degraded). + - **Local-recovery probe** (`LOCAL_RETRY_SECONDS = 60`): while + degraded (`active_transport == "cloud"`), `_request` skips the local + attempt entirely and goes straight to cloud until + `LOCAL_RETRY_SECONDS` have elapsed since the last local failure, at + which point the next call tries local first again as a recovery + probe. Doing this inline per-request (no background prober thread) + is cheap specifically because a down local device fails fast + (connection refused/timeout, well under even the `(3, 5)` local + timeout) — the occasional 60s-interval probe costs little even if + local is still down, and if the probe itself fails, the client falls + through to cloud for that same request and resets the degraded timer + to the probe's own failure time (so the next probe is another full + window out, not immediately retried). + - **Path mapping**: local paths are `/api/`; per the research + doc, cloud mirrors them 1:1 under `/busybar/` relative to + the cloud host. Since `cloud_base_url`'s documented default already + carries that `/busybar` segment, `_cloud_path` simply strips the + local `/api` prefix and lets `cloud_base_url` supply the rest (e.g. + local `/api/display/draw` → cloud tail `/display/draw` → full URL + `https://api.busy.app/busybar/display/draw`). + - **`DrawResult.UNREACHABLE` redefinition**: now means both local AND + cloud (when configured) failed for this call — previously it only + ever meant local failed, since there was no other transport. When + `cloud_token` is empty, behavior is unchanged from pre-v1.6: a local + failure alone is `UNREACHABLE`, no cloud attempt is made at all. + +**Base-URL discrepancy, deliberately left unresolved this round.** +busylib-py's own hardcoded default is `https://proxy.busy.app` — a +different host entirely from this codebase's `https://api.busy.app/busybar` +default. The research doc flags this as unresolved; this round follows the +operator's explicit instruction to default to `api.busy.app` here and defer +resolution to the post-merge live probe (README checklist item 3) rather +than guessing which is authoritative without a real token to test against. + +**Security: `cloud_token` is never logged, at any level including DEBUG.** +Only transport *transitions* are logged (INFO), and those log lines are +static strings with no header/token interpolation — grep confirms no +f-string, `%s`, or `.format()` call anywhere in `client.py` ever +interpolates `cloud_token` or a header dict into a log call. A dedicated +`caplog`-based test (`test_cloud_token_never_appears_in_log_output`) +exercises both the degrade and the both-transports-fail paths and asserts +the placeholder token string never appears in any captured log record's +formatted message or args. + +### Config (`src/busybar/config.py`, `config.example.toml`) + +New `[device]` keys, added to `DEFAULTS["device"]` and mirrored in +`config.example.toml`: `cloud_token = ""`, `cloud_base_url = +"https://api.busy.app/busybar"`, `transport = "auto"`. Key names were +chosen to match `BusyBarClient`'s constructor kwargs exactly, so both +integration call sites (`calendar_countdown/main.py`, +`ci_status/main.py`) simplify from `BusyBarClient(host=cfg["device"] +["host"])` to `BusyBarClient(**cfg["device"])` — a single point of +plumbing rather than adding three more explicit keyword arguments at each +call site, and automatically future-proof against a v1.7 adding a fourth +`[device]` key. + +### Verification + +`TZ=UTC uv run pytest -v`: 337 passed (323 at the start of this branch's +work, after the chirp-`.snd` fix line). New coverage (`tests/test_client.py`): +auto-mode fallback on local failure (asserts both the local call and the +subsequent cloud call's exact URL), no fallback when `cloud_token` is +empty, `UNREACHABLE` requiring both transports to fail, forced `"local"` +never attempting cloud even on failure, forced `"cloud"` always going +straight to cloud, the cloud request's exact shape (Bearer header, +`cloud_timeout`, base URL), the `/api` → `/busybar` path mapping, three +recovery-probe timing tests (skip-local within the window, probe-and- +recover once the window elapses, probe-fails-so-stays-cloud-and-resets- +timer), a fresh-client sanity check that the skip logic never fires before +any degradation, and the token-never-logged `caplog` test. New coverage +(`tests/test_config.py`): the three new `[device]` defaults, and two +config.example.toml parity tests (device section value-for-value matches +`DEFAULTS["device"]`, and `cloud_token` in the shipped example is +literally `""` — not a placeholder that merely looks non-empty). + +### Docs + +README gained a "Cloud transport" section: token creation walkthrough +(cloud.busy.app → API tokens tab → "BUSY Bar" scope), config placement +(explicitly: the real token lives only in git-ignored `config.toml`, +never `config.example.toml`, never committed), rotation guidance +(revocation is immediate and irreversible — create the replacement before +revoking the old one), the status-WS cloud gap, and the post-merge +live-probe checklist (forced-cloud draw probe, cadence headroom check at +the 10s ambient-redraw cadence, base-URL ambiguity resolution) — +duplicated in condensed form above so both the report and the operator- +facing docs carry it. + +### Deferred to the operator (explicit — not an oversight) + +No live cloud verification was performed or attempted this round, per +the coordinator's explicit instruction: the operator had not yet inserted +a real `cloud_token`. All 12 new `test_client.py` tests and both new +`test_config.py` tests use `requests` mocks and a literal +placeholder-string token (`"test-placeholder-token-do-not-use"`) — no +network call was made to any `busy.app` host during this round's work. +The three-item live-probe checklist above is the explicit handoff for +the controller/operator pass that runs once a token exists. diff --git a/integrations/calendar_countdown/main.py b/integrations/calendar_countdown/main.py index eee6406..b5e1847 100644 --- a/integrations/calendar_countdown/main.py +++ b/integrations/calendar_countdown/main.py @@ -215,7 +215,7 @@ def main() -> int: ordering_warning = check_threshold_ordering(cfg["calendar_countdown"]) if ordering_warning is not None: log.warning(ordering_warning) - client = BusyBarClient(host=cfg["device"]["host"]) + client = BusyBarClient(**cfg["device"]) # Drop any stale elements from a previous process. This also protects a # restart onto this version against every id change made across the # v1.3 -> v1.3.1 -> v1.4 line: v1.3.1 replaced the native "countdown" diff --git a/integrations/ci_status/main.py b/integrations/ci_status/main.py index 1206656..73fcd10 100644 --- a/integrations/ci_status/main.py +++ b/integrations/ci_status/main.py @@ -366,7 +366,7 @@ def main() -> int: except RuntimeError as exc: log.error(str(exc)) return 1 - client = BusyBarClient(host=cfg["device"]["host"]) + client = BusyBarClient(**cfg["device"]) client.clear(APP) # drop any stale elements from a previous process (type collisions 400) state_cache: dict[str, RepoState] = {} diff --git a/src/busybar/client.py b/src/busybar/client.py index a335e76..1eccede 100644 --- a/src/busybar/client.py +++ b/src/busybar/client.py @@ -8,26 +8,137 @@ NULL_CARD_ID = "00000000-0000-0000-0000-000000000000" +# v1.6 cloud transport fallback -- while degraded to cloud, `_request` skips +# the (known-failing) local attempt and goes straight to cloud until this +# many seconds have elapsed since the last local failure, then tries local +# first again as a recovery probe. Doing this inline (no background thread) +# is cheap because a down local device fails fast (connection refused/ +# timeout well under `timeout`), so the occasional probe costs little. +LOCAL_RETRY_SECONDS = 60 + class DrawResult(Enum): DRAWN = "drawn" REJECTED = "rejected" # 409: higher-priority app on screen — expected - UNREACHABLE = "unreachable" # device off / USB unplugged — caller backs off + UNREACHABLE = "unreachable" # local AND cloud (if configured) both failed — caller backs off ERROR = "error" # non-200/409 from a live device — no backoff; retried next poll class BusyBarClient: - def __init__(self, host: str = "10.0.4.20", timeout: tuple = (3, 5)): + """Talks to a single BUSY Bar device, over the LAN (local transport, + the default and preferred path) and, when configured, via BUSY's cloud + relay as an automatic fallback when the local device is unreachable. + + Transport selection (`transport`, mirroring busylib-py's single-class + transport-flag pattern rather than a class hierarchy -- see + scratchpad/busy-cloud-api-research.md for the source citations): + + - `"auto"` (default): every call tries local first with `timeout`. On + a `requests.RequestException` AND a non-empty `cloud_token`, the + SAME request is retried against `cloud_base_url` with + `cloud_timeout` and an `Authorization: Bearer` header. ` + active_transport` tracks which transport last succeeded. While + degraded (`active_transport == "cloud"`), subsequent calls skip the + local attempt and go straight to cloud until `LOCAL_RETRY_SECONDS` + have elapsed since the last local failure, at which point local is + retried first again as a recovery probe (see module docstring + constant above). `cloud_token = ""` (the shipped default) disables + cloud fallback entirely regardless of `transport="auto"` -- calls + behave exactly as they did before v1.6. + - `"local"`: local only, never falls back. Pre-v1.6 behavior exactly. + - `"cloud"`: cloud only, forced -- never attempts local. For + deliberately testing/debugging the cloud path. + + Local endpoints are mounted at `/api/...`; the cloud API mirrors them + 1:1 under `/busybar/...` relative to the cloud host. `cloud_base_url`'s + documented default (`https://api.busy.app/busybar`) already carries + that `/busybar` segment, so cloud requests are built by stripping the + local `/api` prefix and appending the remainder to `cloud_base_url`. + + SECURITY: `cloud_token` is never logged or included in any log + statement, at any level including DEBUG -- only transport + *transitions* (local->cloud degradation, cloud->local recovery) are + logged, at INFO, and those log lines never include header values. + """ + + def __init__(self, host: str = "10.0.4.20", timeout: tuple = (3, 5), *, + cloud_token: str = "", cloud_base_url: str = "https://api.busy.app/busybar", + transport: str = "auto", cloud_timeout: tuple = (5, 15)): + if transport not in ("auto", "local", "cloud"): + raise ValueError(f"transport must be 'auto', 'local', or 'cloud', got {transport!r}") self.base = f"http://{host}" self.timeout = timeout + self.cloud_token = cloud_token + self.cloud_base = cloud_base_url.rstrip("/") + self.cloud_timeout = cloud_timeout + self.transport = transport + # Cloud fallback is only "configured" with a non-empty token; an + # empty string (the shipped default) disables it in "auto" mode + # regardless of anything else. Forced transport="cloud" is exempt + # from this gate deliberately -- it's the caller's explicit, + # non-"auto" choice, not a fallback decision this client makes. + self._cloud_configured = bool(cloud_token) + self.active_transport = "cloud" if transport == "cloud" else "local" + self._degraded_since: float | None = None # time.monotonic() of the last local + # failure while in "auto" mode; None + # whenever active_transport == "local" - def _request(self, method: str, path: str, **kwargs) -> requests.Response | None: + def _mark_degraded(self) -> None: + if self.active_transport != "cloud": + log.info("busybar transport: local -> cloud (local device unreachable; falling back)") + self.active_transport = "cloud" + self._degraded_since = time.monotonic() + + def _mark_recovered(self) -> None: + if self.active_transport != "local": + log.info("busybar transport: cloud -> local (local device reachable again)") + self.active_transport = "local" + self._degraded_since = None + + def _should_probe_local(self) -> bool: + return (self._degraded_since is not None + and (time.monotonic() - self._degraded_since) >= LOCAL_RETRY_SECONDS) + + def _cloud_path(self, path: str) -> str: + return path[len("/api"):] if path.startswith("/api") else path + + def _try_local(self, method: str, path: str, **kwargs) -> requests.Response | None: try: return requests.request(method, f"{self.base}{path}", timeout=self.timeout, **kwargs) except requests.RequestException as exc: log.debug("device unreachable: %s", exc) return None + def _try_cloud(self, method: str, path: str, **kwargs) -> requests.Response | None: + headers = {**(kwargs.pop("headers", None) or {}), "Authorization": f"Bearer {self.cloud_token}"} + try: + return requests.request(method, f"{self.cloud_base}{self._cloud_path(path)}", + timeout=self.cloud_timeout, headers=headers, **kwargs) + except requests.RequestException as exc: + log.debug("cloud unreachable: %s", exc) + return None + + def _request(self, method: str, path: str, **kwargs) -> requests.Response | None: + if self.transport == "local": + return self._try_local(method, path, **kwargs) + + if self.transport == "cloud": + return self._try_cloud(method, path, **kwargs) + + # transport == "auto": local-first-with-cloud-fallback, with the + # LOCAL_RETRY_SECONDS recovery probe described in the class + # docstring. + if self.active_transport == "local" or self._should_probe_local(): + resp = self._try_local(method, path, **kwargs) + if resp is not None: + self._mark_recovered() + return resp + if not self._cloud_configured: + return None + self._mark_degraded() + + return self._try_cloud(method, path, **kwargs) + def draw(self, application_name: str, elements: list[dict], priority: int = 50, led_notification_color: str | None = None) -> DrawResult: body: dict = {"application_name": application_name, "priority": priority, diff --git a/src/busybar/config.py b/src/busybar/config.py index 9390588..e81d27c 100644 --- a/src/busybar/config.py +++ b/src/busybar/config.py @@ -4,7 +4,17 @@ from pathlib import Path DEFAULTS: dict = { - "device": {"host": "10.0.4.20"}, + "device": { + "host": "10.0.4.20", + # Cloud transport fallback (v1.6) -- see README's "Cloud + # transport" section before setting these. An empty cloud_token + # (the shipped default) disables cloud fallback entirely in + # "auto" mode; keys here MUST match BusyBarClient's constructor + # kwarg names exactly since call sites pass **cfg["device"]. + "cloud_token": "", + "cloud_base_url": "https://api.busy.app/busybar", + "transport": "auto", # "auto" | "local" | "cloud" (forced) + }, "calendar_countdown": { # 10s matches busybar.display.AMBIENT_REDRAW_SECONDS -- the ambient # tier's redraw contract, tuned (after on-device re-measurement diff --git a/tests/test_client.py b/tests/test_client.py index 998899b..c69e598 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -150,3 +150,178 @@ def test_play_audio_never_touches_volume_endpoint(mock_request): BusyBarClient().play_audio("app", stock_path="shared/x.wav") for call in mock_request.call_args_list: assert "/api/audio/volume" not in call.args[1] + + +# --- cloud transport fallback (v1.6) ---------------------------------------- +# Placeholder tokens only -- never anything resembling a real credential. +FAKE_TOKEN = "test-placeholder-token-do-not-use" + + +def _cloud_client(host="192.0.2.1", **kwargs): + return BusyBarClient(host=host, cloud_token=FAKE_TOKEN, + cloud_base_url="https://cloud.example.test/busybar", **kwargs) + + +@patch("busybar.client.requests.request") +def test_auto_falls_back_to_cloud_on_local_failure(mock_request): + mock_request.side_effect = [requests.ConnectionError(), _response(200)] + client = _cloud_client() + assert client.draw("app", ELEMENTS) == DrawResult.DRAWN + assert mock_request.call_count == 2 + local_call, cloud_call = mock_request.call_args_list + assert local_call.args == ("POST", "http://192.0.2.1/api/display/draw") + assert cloud_call.args == ("POST", "https://cloud.example.test/busybar/display/draw") + assert client.active_transport == "cloud" + + +@patch("busybar.client.requests.request") +def test_no_fallback_when_cloud_unconfigured(mock_request): + mock_request.side_effect = requests.ConnectionError() + client = BusyBarClient(host="192.0.2.1") # cloud_token defaults to "" + assert client.draw("app", ELEMENTS) == DrawResult.UNREACHABLE + assert mock_request.call_count == 1 # only local was ever attempted + assert client.active_transport == "local" # never transitions without cloud configured + + +@patch("busybar.client.requests.request") +def test_unreachable_when_both_transports_fail(mock_request): + mock_request.side_effect = requests.ConnectionError() + client = _cloud_client() + assert client.draw("app", ELEMENTS) == DrawResult.UNREACHABLE + assert mock_request.call_count == 2 # local AND cloud were both attempted + assert client.active_transport == "cloud" + + +@patch("busybar.client.requests.request") +def test_forced_local_never_attempts_cloud_even_on_failure(mock_request): + mock_request.side_effect = requests.ConnectionError() + client = _cloud_client(transport="local") + assert client.draw("app", ELEMENTS) == DrawResult.UNREACHABLE + assert mock_request.call_count == 1 + assert mock_request.call_args.args[1].startswith("http://192.0.2.1") + + +@patch("busybar.client.requests.request") +def test_forced_cloud_always_goes_straight_to_cloud(mock_request): + mock_request.return_value = _response(200) + client = _cloud_client(transport="cloud") + assert client.draw("app", ELEMENTS) == DrawResult.DRAWN + assert mock_request.call_count == 1 + method, url = mock_request.call_args.args + assert method == "POST" and url == "https://cloud.example.test/busybar/display/draw" + assert client.active_transport == "cloud" + + +@patch("busybar.client.requests.request") +def test_cloud_request_shape_bearer_header_and_base_url(mock_request): + mock_request.side_effect = [requests.ConnectionError(), _response(200)] + client = _cloud_client() + client.draw("app", ELEMENTS, priority=30) + cloud_call = mock_request.call_args_list[1] + assert cloud_call.args == ("POST", "https://cloud.example.test/busybar/display/draw") + assert cloud_call.kwargs["headers"] == {"Authorization": f"Bearer {FAKE_TOKEN}"} + assert cloud_call.kwargs["timeout"] == (5, 15) + assert cloud_call.kwargs["json"]["application_name"] == "app" + + +@patch("busybar.client.requests.request") +def test_cloud_path_mapping_strips_api_prefix(mock_request): + mock_request.side_effect = [requests.ConnectionError(), _response(200, {})] + client = _cloud_client() + client.status() + cloud_call = mock_request.call_args_list[1] + assert cloud_call.args == ("GET", "https://cloud.example.test/busybar/status") + + +@patch("busybar.client.time.monotonic") +@patch("busybar.client.requests.request") +def test_degraded_client_skips_local_within_retry_window(mock_request, mock_time): + # First call degrades to cloud at t=0. A second call at t=30 (well + # inside LOCAL_RETRY_SECONDS=60) must skip the local attempt entirely + # and go straight to cloud -- only one requests.request call for the + # second draw, and it must be the cloud URL. + mock_time.return_value = 0.0 + mock_request.side_effect = [requests.ConnectionError(), _response(200)] + client = _cloud_client() + assert client.draw("app", ELEMENTS) == DrawResult.DRAWN + assert mock_request.call_count == 2 # local (failed) + cloud (succeeded) + + mock_time.return_value = 30.0 + mock_request.reset_mock() + mock_request.side_effect = None + mock_request.return_value = _response(200) + assert client.draw("app", ELEMENTS) == DrawResult.DRAWN + assert mock_request.call_count == 1 # local skipped -- straight to cloud + method, url = mock_request.call_args.args + assert url == "https://cloud.example.test/busybar/display/draw" + + +@patch("busybar.client.time.monotonic") +@patch("busybar.client.requests.request") +def test_degraded_client_probes_local_after_retry_window_elapses(mock_request, mock_time): + # Degrade at t=0, then let LOCAL_RETRY_SECONDS (60) elapse: the next + # call must try local FIRST again (the recovery probe), and recover + # to active_transport == "local" when it succeeds. + mock_time.return_value = 0.0 + mock_request.side_effect = [requests.ConnectionError(), _response(200)] + client = _cloud_client() + client.draw("app", ELEMENTS) + assert client.active_transport == "cloud" + + mock_time.return_value = 61.0 # LOCAL_RETRY_SECONDS elapsed + mock_request.reset_mock() + mock_request.side_effect = None + mock_request.return_value = _response(200) # local now recovered + assert client.draw("app", ELEMENTS) == DrawResult.DRAWN + assert mock_request.call_count == 1 # local probe succeeded, no cloud needed + method, url = mock_request.call_args.args + assert url == "http://192.0.2.1/api/display/draw" + assert client.active_transport == "local" + + +@patch("busybar.client.time.monotonic") +@patch("busybar.client.requests.request") +def test_degraded_client_reprobes_local_and_stays_cloud_if_still_down(mock_request, mock_time): + # Recovery probe fires after the window elapses but local is STILL + # down: client must fall back to cloud again for that same request + # (not return UNREACHABLE just because the probe failed) and reset + # the degraded timer so the next probe is another window out. + mock_time.return_value = 0.0 + mock_request.side_effect = [requests.ConnectionError(), _response(200)] + client = _cloud_client() + client.draw("app", ELEMENTS) + + mock_time.return_value = 61.0 + mock_request.side_effect = [requests.ConnectionError(), _response(200)] # local still down, cloud up + assert client.draw("app", ELEMENTS) == DrawResult.DRAWN + assert client.active_transport == "cloud" + assert client._degraded_since == 61.0 # timer reset to the failed probe's time + + +@patch("busybar.client.requests.request") +def test_no_recovery_probe_before_window_when_never_degraded(mock_request): + # A freshly-constructed "auto" client (active_transport == "local") + # always tries local first, regardless of LOCAL_RETRY_SECONDS -- the + # probe-skip logic only applies once actually degraded. + mock_request.return_value = _response(200) + client = _cloud_client() + assert client.active_transport == "local" + assert client.draw("app", ELEMENTS) == DrawResult.DRAWN + assert mock_request.call_count == 1 + assert mock_request.call_args.args[1] == "http://192.0.2.1/api/display/draw" + + +# --- token never logged (v1.6 security requirement) ------------------------- + +@patch("busybar.client.requests.request") +def test_cloud_token_never_appears_in_log_output(mock_request, caplog): + import logging + caplog.set_level(logging.DEBUG, logger="busybar.client") + mock_request.side_effect = [requests.ConnectionError(), _response(200), + requests.ConnectionError(), requests.ConnectionError()] + client = _cloud_client() + client.draw("app", ELEMENTS) # local fails, cloud succeeds -- degrades + client.draw("app", ELEMENTS) # local fails again, cloud fails too -- UNREACHABLE + for record in caplog.records: + assert FAKE_TOKEN not in record.getMessage() + assert FAKE_TOKEN not in str(record.args) diff --git a/tests/test_config.py b/tests/test_config.py index 3694799..701d5e1 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,9 +1,16 @@ +import tomllib from pathlib import Path -from busybar.config import load_config +from busybar.config import DEFAULTS, load_config + +REPO_ROOT = Path(__file__).resolve().parents[1] def test_defaults_when_no_file(tmp_path): cfg = load_config(tmp_path / "missing.toml") assert cfg["device"]["host"] == "10.0.4.20" + # v1.6 cloud transport fallback -- disabled out of the box. + assert cfg["device"]["cloud_token"] == "" + assert cfg["device"]["cloud_base_url"] == "https://api.busy.app/busybar" + assert cfg["device"]["transport"] == "auto" assert cfg["calendar_countdown"]["poll_seconds"] == 10 # v1.5 ambient-tier default assert cfg["ci_status"]["repos"] == [] assert cfg["ci_status"]["show_running"] is True @@ -39,3 +46,24 @@ def test_returned_config_mutation_does_not_corrupt_defaults(tmp_path): cfg2 = load_config(tmp_path / "missing.toml") assert cfg2["ci_status"]["repos"] == [] assert cfg2["calendar_countdown"]["poll_seconds"] == 10 + + +# --- config.example.toml parity (v1.6) -------------------------------------- +# config.example.toml's own header claims "All keys optional; defaults +# shown" -- these tests hold that claim to account for the [device] table +# specifically, since it's the one this round touches and the one where a +# drifted example (e.g. a non-empty placeholder token) would be a real +# hygiene problem, not just documentation staleness. + +def test_example_toml_device_section_matches_defaults(): + with open(REPO_ROOT / "config.example.toml", "rb") as fh: + example = tomllib.load(fh) + assert example["device"] == DEFAULTS["device"] + +def test_example_toml_cloud_token_is_empty_placeholder_not_a_real_looking_token(): + with open(REPO_ROOT / "config.example.toml", "rb") as fh: + example = tomllib.load(fh) + # Guards against ever accidentally shipping a real-looking credential + # in the committed example file -- the real token belongs only in the + # git-ignored config.toml. + assert example["device"]["cloud_token"] == "" From 628e3dded9b1519877e5de216e509118890a47c3 Mon Sep 17 00:00:00 2001 From: John Osumi <931193+sumitake@users.noreply.github.com> Date: Tue, 4 Aug 2026 12:28:31 -0700 Subject: [PATCH 2/2] client: guard config splat against unknown keys; lock transport-fallback contracts in tests Final-gate review fixes for the v1.6 cloud transport fallback feature, same branch (dev/claude/cloud-transport-v1.6), addressing two Important and three Minor findings. Important #1: BusyBarClient(**cfg["device"]) crashed with a cryptic TypeError on any unknown/typo'd [device] key -- a regression from pre-v1.6, where only host= was ever passed explicitly and an unrelated key was silently ignored. Worst-timed failure mode: an operator typo'ing cloud_token during first-time setup would hit this. Fixed with a new busybar.config.device_kwargs(cfg) helper that filters cfg["device"] down to BusyBarClient's actual constructor kwargs (introspected via inspect.signature so it can't drift out of sync with the constructor) and logs one WARNING per dropped key, restoring "ignored, not fatal" while adding observability the pre-v1.6 code never had. Both calendar_countdown/main.py and ci_status/main.py now construct BusyBarClient(**device_kwargs(cfg)). Important #2: added the single most important untested behavioral contract of the whole feature -- a client with cloud_token configured, where local returns HTTP 500 (a real response, no exception), must result in DrawResult.ERROR with cloud never attempted. Locks in "fallback triggers strictly on requests.RequestException, never on any HTTP response the device actually returned." Minor #1: fixed an inaccurate comment in test_cloud_token_never_appears_in_log_output (claimed the second call re-attempts local; it actually skips local within the recovery window) and added time.monotonic mocking for determinism instead of relying on real wall-clock time staying under LOCAL_RETRY_SECONDS between two adjacent statements. Minor #2: added cloud-409 -> REJECTED coverage for both forced-cloud and degraded-auto transport, mirroring the existing local-409 test. Minor #3: added a direct test for the transport ValueError guard. 337 -> 346 tests passing (9 net-new: 5 in test_config.py for device_kwargs, 4 in test_client.py for the HTTP-500/cloud-409/ValueError contracts). Report appended to .superpowers/sdd/display-v1.6-report.md (gitignored); spec doc gained a "Final-gate review fixes" subsection under the v1.6 section. Co-Authored-By: Claude Fable 5 --- ...6-08-03-calendar-ci-integrations-design.md | 56 +++++++++++++++ integrations/calendar_countdown/main.py | 4 +- integrations/ci_status/main.py | 4 +- src/busybar/config.py | 36 ++++++++++ tests/test_client.py | 68 +++++++++++++++++-- tests/test_config.py | 47 ++++++++++++- 6 files changed, 204 insertions(+), 11 deletions(-) diff --git a/docs/superpowers/specs/2026-08-03-calendar-ci-integrations-design.md b/docs/superpowers/specs/2026-08-03-calendar-ci-integrations-design.md index 870e04b..491f868 100644 --- a/docs/superpowers/specs/2026-08-03-calendar-ci-integrations-design.md +++ b/docs/superpowers/specs/2026-08-03-calendar-ci-integrations-design.md @@ -1611,3 +1611,59 @@ placeholder-string token (`"test-placeholder-token-do-not-use"`) — no network call was made to any `busy.app` host during this round's work. The three-item live-probe checklist above is the explicit handoff for the controller/operator pass that runs once a token exists. + +### Final-gate review fixes (same branch) + +The coordinator's final-gate review found two Important issues and three +Minor issues before merge: + +1. **Important — `**cfg["device"]` crash surface.** Both call sites + splatting the raw `[device]` dict meant an unknown/typo'd key (e.g. + `coud_token` for `cloud_token`) went from silently-ignored (pre-v1.6, + when only `host=` was ever passed explicitly) to a `TypeError` + crashing startup — exactly the wrong failure mode for a typo made + while first configuring `cloud_token`. Fixed with a new + `busybar.config.device_kwargs(cfg)` helper: filters `cfg["device"]` to + `BusyBarClient`'s actual constructor kwargs (introspected via + `inspect.signature`, so it can't drift out of sync with the + constructor) and logs a `WARNING` naming each dropped key, restoring + "ignored, not fatal" while adding the observability the pre-v1.6 code + never had. Both call sites now read + `BusyBarClient(**device_kwargs(cfg))`. Five new tests in + `test_config.py`: known keys pass through unchanged, an unknown key is + dropped without crashing (`caplog` confirms the `WARNING` names it), + constructing `BusyBarClient` from the filtered result never raises, + one `WARNING` fires per unknown key (not one combined message), and no + warnings fire when every key is known. +2. **Important — untested highest-risk semantic.** Added + `test_local_http_500_is_error_and_does_not_trigger_cloud_fallback`: + with `cloud_token` configured, a local HTTP `500` (a real response, no + exception) must return `DrawResult.ERROR` and must NOT attempt cloud + at all — locks in that fallback triggers strictly on + `requests.RequestException`, never on a non-2xx/409 response the + device actually returned. This was previously only implied by the + `_request` implementation, not directly asserted. +3. **Minor — inaccurate comment fixed.** + `test_cloud_token_never_appears_in_log_output`'s second `draw()` call + was commented "local fails again, cloud fails too" — wrong: per the + recovery-probe design, a call made well within `LOCAL_RETRY_SECONDS` + of degrading skips the local attempt entirely and goes straight to + cloud, so only one `requests.request` call happens on that second + draw. Corrected the comment and, per the reviewer's suggestion, added + `@patch("busybar.client.time.monotonic")` to control elapsed time + explicitly rather than relying on real wall-clock time staying under + 60s between two adjacent test statements. +4. **Minor — cloud-409 coverage added.** Two new tests: + `test_cloud_409_is_rejected_not_error_forced_cloud` (forced + `transport="cloud"`) and `test_cloud_409_is_rejected_not_error_while_degraded` + (auto mode, already degraded to cloud) — both assert a cloud `409` + maps to `DrawResult.REJECTED`, mirroring the existing local-409 + coverage. +5. **Minor — transport `ValueError` guard tested.** + `test_invalid_transport_value_raises_value_error` asserts + `BusyBarClient(transport="carrier-pigeon")` raises `ValueError` naming + the bad value — previously implemented but unverified by any test. + +`TZ=UTC uv run pytest -v`: 346 passed (337 before this review round's 9 +net-new tests: 5 in `test_config.py` for `device_kwargs`, 4 in +`test_client.py` for the 500/409/ValueError contracts). diff --git a/integrations/calendar_countdown/main.py b/integrations/calendar_countdown/main.py index b5e1847..b83a7fb 100644 --- a/integrations/calendar_countdown/main.py +++ b/integrations/calendar_countdown/main.py @@ -12,7 +12,7 @@ from datetime import datetime, timezone from busybar.client import BusyBarClient, DrawResult -from busybar.config import load_config +from busybar.config import device_kwargs, load_config from busybar.display import PRIORITY_AMBIENT, ambient_timeout from .logic import (ascii_safe, build_elements, select_active_event, @@ -215,7 +215,7 @@ def main() -> int: ordering_warning = check_threshold_ordering(cfg["calendar_countdown"]) if ordering_warning is not None: log.warning(ordering_warning) - client = BusyBarClient(**cfg["device"]) + client = BusyBarClient(**device_kwargs(cfg)) # Drop any stale elements from a previous process. This also protects a # restart onto this version against every id change made across the # v1.3 -> v1.3.1 -> v1.4 line: v1.3.1 replaced the native "countdown" diff --git a/integrations/ci_status/main.py b/integrations/ci_status/main.py index 73fcd10..c06504e 100644 --- a/integrations/ci_status/main.py +++ b/integrations/ci_status/main.py @@ -12,7 +12,7 @@ from datetime import datetime, timedelta, timezone from busybar.client import BusyBarClient, DrawResult -from busybar.config import load_config +from busybar.config import device_kwargs, load_config from busybar.display import OVERLAY_DWELL_SECONDS, overlay_gap_elapsed from .logic import ( @@ -366,7 +366,7 @@ def main() -> int: except RuntimeError as exc: log.error(str(exc)) return 1 - client = BusyBarClient(**cfg["device"]) + client = BusyBarClient(**device_kwargs(cfg)) client.clear(APP) # drop any stale elements from a previous process (type collisions 400) state_cache: dict[str, RepoState] = {} diff --git a/src/busybar/config.py b/src/busybar/config.py index e81d27c..f8fb690 100644 --- a/src/busybar/config.py +++ b/src/busybar/config.py @@ -1,8 +1,14 @@ import copy +import inspect +import logging import os import tomllib from pathlib import Path +from busybar.client import BusyBarClient + +log = logging.getLogger(__name__) + DEFAULTS: dict = { "device": { "host": "10.0.4.20", @@ -79,6 +85,36 @@ def _merge(base: dict, override: dict) -> dict: return out +def device_kwargs(cfg: dict) -> dict: + """Filter `cfg["device"]` down to BusyBarClient's known constructor + kwargs, for use as `BusyBarClient(**device_kwargs(cfg))`. + + Before v1.6 both integration call sites only ever passed a single + explicit keyword (`host=cfg["device"]["host"]`), so an unknown/typo'd + [device] key in config.toml (e.g. `coud_token` for `cloud_token`) was + silently ignored -- it just never got read. v1.6 switched both call + sites to splat the whole [device] table so the three new cloud- + transport keys wouldn't need updating twice; splatting an *unfiltered* + dict, though, means that same typo now raises TypeError("unexpected + keyword argument") at startup instead -- a cryptic crash, and the + worst possible failure mode for exactly the moment an operator is + most likely to typo a key: first-time cloud_token setup. This + restores "unknown key doesn't crash startup" while adding + observability the pre-v1.6 code never had: each ignored key is + logged at WARNING (not silently dropped) so a genuine typo is still + visible, just not fatal. + + The known-kwargs set is derived from BusyBarClient's own signature + (rather than hardcoded here) so it can't drift out of sync with the + constructor as transport options evolve. + """ + known = set(inspect.signature(BusyBarClient.__init__).parameters) - {"self"} + device = cfg.get("device", {}) + for key in sorted(set(device) - known): + log.warning("config: ignoring unknown [device] key %r (not a BusyBarClient parameter)", key) + return {k: v for k, v in device.items() if k in known} + + def find_config() -> Path | None: candidate = Path(__file__).resolve().parents[2] / "config.toml" return candidate if candidate.exists() else None diff --git a/tests/test_client.py b/tests/test_client.py index c69e598..58d57aa 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1,3 +1,4 @@ +import logging from unittest.mock import Mock, patch import requests from busybar.client import BusyBarClient, DrawResult @@ -313,15 +314,70 @@ def test_no_recovery_probe_before_window_when_never_degraded(mock_request): # --- token never logged (v1.6 security requirement) ------------------------- +@patch("busybar.client.time.monotonic") @patch("busybar.client.requests.request") -def test_cloud_token_never_appears_in_log_output(mock_request, caplog): - import logging +def test_cloud_token_never_appears_in_log_output(mock_request, mock_time, caplog): caplog.set_level(logging.DEBUG, logger="busybar.client") - mock_request.side_effect = [requests.ConnectionError(), _response(200), - requests.ConnectionError(), requests.ConnectionError()] + mock_time.return_value = 0.0 + mock_request.side_effect = [requests.ConnectionError(), _response(200)] client = _cloud_client() - client.draw("app", ELEMENTS) # local fails, cloud succeeds -- degrades - client.draw("app", ELEMENTS) # local fails again, cloud fails too -- UNREACHABLE + client.draw("app", ELEMENTS) # local fails, cloud succeeds -- degrades to cloud + + # Still well within LOCAL_RETRY_SECONDS (60): per the recovery-probe + # design (see test_degraded_client_skips_local_within_retry_window), + # this second call skips the local attempt entirely and goes straight + # to cloud -- only ONE requests.request call happens here, not two. + mock_time.return_value = 30.0 + mock_request.side_effect = [requests.ConnectionError()] # the sole (cloud) attempt fails + assert client.draw("app", ELEMENTS) == DrawResult.UNREACHABLE + for record in caplog.records: assert FAKE_TOKEN not in record.getMessage() assert FAKE_TOKEN not in str(record.args) + + +# --- fallback-only-on-RequestException contract (final-gate review) -------- + +@patch("busybar.client.requests.request") +def test_local_http_500_is_error_and_does_not_trigger_cloud_fallback(mock_request): + # Highest-risk semantic in the whole feature: a local HTTP error + # response (no exception -- the device is reachable, it just returned + # a bad status) must NOT be treated as "local is down." Locks in that + # fallback triggers ONLY on requests.RequestException, never on a + # non-2xx/409 response the device actually returned. + local_500 = _response(500) + mock_request.return_value = local_500 + client = _cloud_client() + assert client.draw("app", ELEMENTS) == DrawResult.ERROR + assert mock_request.call_count == 1 # cloud was never attempted + assert mock_request.call_args.args == ("POST", "http://192.0.2.1/api/display/draw") + assert client.active_transport == "local" # never degraded + + +# --- cloud 409 -> REJECTED (final-gate review) ------------------------------- + +@patch("busybar.client.requests.request") +def test_cloud_409_is_rejected_not_error_forced_cloud(mock_request): + mock_request.return_value = _response(409) + client = _cloud_client(transport="cloud") + assert client.draw("app", ELEMENTS) == DrawResult.REJECTED + +@patch("busybar.client.requests.request") +def test_cloud_409_is_rejected_not_error_while_degraded(mock_request): + mock_request.side_effect = [requests.ConnectionError(), _response(200)] + client = _cloud_client() + client.draw("app", ELEMENTS) # degrades to cloud + assert client.active_transport == "cloud" + + mock_request.side_effect = [_response(409)] # degraded -- skips local, straight to cloud + assert client.draw("app", ELEMENTS) == DrawResult.REJECTED + + +# --- transport ValueError guard (final-gate review) -------------------------- + +def test_invalid_transport_value_raises_value_error(): + try: + BusyBarClient(transport="carrier-pigeon") + raise AssertionError("expected ValueError") + except ValueError as exc: + assert "carrier-pigeon" in str(exc) diff --git a/tests/test_config.py b/tests/test_config.py index 701d5e1..835b961 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,6 +1,7 @@ +import logging import tomllib from pathlib import Path -from busybar.config import DEFAULTS, load_config +from busybar.config import DEFAULTS, device_kwargs, load_config REPO_ROOT = Path(__file__).resolve().parents[1] @@ -67,3 +68,47 @@ def test_example_toml_cloud_token_is_empty_placeholder_not_a_real_looking_token( # in the committed example file -- the real token belongs only in the # git-ignored config.toml. assert example["device"]["cloud_token"] == "" + + +# --- device_kwargs() unknown-key guard (final-gate review, v1.6) ----------- +# Both integration call sites splat cfg["device"] into BusyBarClient's +# constructor. Splatting the raw dict would TypeError on any unknown/typo'd +# [device] key (a silent-ignore -> crash regression from pre-v1.6, where +# only host= was ever passed explicitly) -- device_kwargs() must filter to +# known kwargs and warn, not crash. + +def test_device_kwargs_passes_through_all_known_keys(): + cfg = {"device": {"host": "192.0.2.1", "cloud_token": "x", + "cloud_base_url": "https://cloud.example.test", + "transport": "local"}} + assert device_kwargs(cfg) == cfg["device"] + +def test_device_kwargs_drops_unknown_key_without_crashing(caplog): + cfg = {"device": {"host": "192.0.2.1", "coud_token": "typo'd-key"}} + caplog.set_level(logging.WARNING, logger="busybar.config") + result = device_kwargs(cfg) + assert result == {"host": "192.0.2.1"} # unknown key silently dropped, not crashed + assert "coud_token" in caplog.text + assert any(record.levelname == "WARNING" for record in caplog.records) + +def test_device_kwargs_result_never_crashes_busybarclient_construction(): + from busybar.client import BusyBarClient + cfg = {"device": {"host": "192.0.2.1", "cloud_token": "x", "bogus_extra_key": 123}} + # This is the actual regression this guard exists for: a typo'd or + # unrecognized [device] key must not raise TypeError when splatted. + client = BusyBarClient(**device_kwargs(cfg)) + assert client.base == "http://192.0.2.1" + +def test_device_kwargs_logs_one_warning_per_unknown_key(caplog): + cfg = {"device": {"host": "192.0.2.1", "bogus_one": 1, "bogus_two": 2}} + caplog.set_level(logging.WARNING, logger="busybar.config") + device_kwargs(cfg) + warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warnings) == 2 + assert any("bogus_one" in r.getMessage() for r in warnings) + assert any("bogus_two" in r.getMessage() for r in warnings) + +def test_device_kwargs_no_warnings_when_all_keys_known(caplog): + caplog.set_level(logging.WARNING, logger="busybar.config") + device_kwargs({"device": dict(DEFAULTS["device"])}) + assert not [r for r in caplog.records if r.levelname == "WARNING"]