From 7f743d0392fc9629d878013b28e0b951d54e4432 Mon Sep 17 00:00:00 2001 From: ZdenekSrotyr Date: Mon, 4 May 2026 14:30:43 +0200 Subject: [PATCH] =?UTF-8?q?fix(cli):=20#168=20review=20iter=206=20?= =?UTF-8?q?=E2=80=94=20render=20empty-string=20diagnostics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Devin Review iter #6 found 2 issues. 🟡 BUG: cli/error_render.py filtered out empty-string values via `detail[key] not in (None, "")` and `value not in (None, "")` before they could reach `_kv_line`. But `_kv_line` was specifically designed to render empty strings as `(empty)` — the filter shadowed that branch. The hidden field happens to be the most operator-actionable one in `cross_project_forbidden`: `billing_project: ""` is the exact diagnostic confirming WHY USER_PROJECT_DENIED fires. Change filter to `is not None`. Empty strings now flow through `_kv_line` and render as `billing_project: (empty)`. 📝 ANALYSIS: CHANGELOG wording for the test-connection endpoint said "the saved data_source.bigquery config", which Devin flagged as slightly misleading because `get_bq_access` is `@functools.cache`d — "Test connection" tests the config in the running process, not the just-saved YAML overlay. The save flow already returns `restart_required: True` and the UI shows a banner, so the behavior is documented; only the CHANGELOG wording was loose. Tightened to "the **process-cached** BqAccess... Tests the config active in the running process — after a save the response includes restart_required; click Test AFTER restart to validate the freshly-saved values." New test: test_renders_empty_string_as_empty_marker locks in the empty-string-as-(empty) rendering for the cross_project_forbidden case so a future filter change won't silently drop the diagnostic again. 9 affected render tests pass. --- CHANGELOG.md | 10 +++++++--- cli/error_render.py | 10 +++++++--- tests/test_cli_error_render.py | 16 ++++++++++++++++ 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a75489..6270ef3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,9 +13,13 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C ### Added - **`/admin/server-config` BQ test connection**: admin-only `POST /api/admin/bigquery/test-connection` runs a 10s-timeout `SELECT 1` - against BigQuery via the saved `data_source.bigquery` config and - returns typed structured feedback (`200 ok` / `400 not_configured` / - `502 cross_project_forbidden` / `504 timeout`). The + against BigQuery via the **process-cached** `BqAccess` + (`@functools.cache` on `get_bq_access`) and returns typed structured + feedback (`200 ok` / `400 not_configured` / `502 cross_project_forbidden` + / `504 timeout`). Tests the config active in the running process — + after a `data_source.bigquery` save the response shape includes + `restart_required: True`; click "Test connection" AFTER restart to + validate the freshly-saved values. The /admin/server-config UI gets a "Test BigQuery connection" button next to the data_source Save button; on failure the inline result uses the same structured shape as the CLI renderer so operators see the same diff --git a/cli/error_render.py b/cli/error_render.py index fe4951c..3859266 100644 --- a/cli/error_render.py +++ b/cli/error_render.py @@ -77,11 +77,15 @@ def _format_dict(status_code: int, detail: dict) -> str: # Only the key actually used in the label is hidden from the kv block. seen: set[str] = {label_key} if label_key else set() - # Priority keys first + # Priority keys first. Filter only None — `_kv_line` already renders + # empty strings as `(empty)`, which is the key diagnostic for + # `billing_project: ""` in cross_project_forbidden errors. Earlier + # `not in (None, "")` filter dropped exactly the field the operator + # needs to see (Devin Review iter #6 on PR #168). for key in _PRIORITY_KEYS: if key in seen: continue - if key in detail and detail[key] not in (None, ""): + if key in detail and detail[key] is not None: lines.append(_kv_line(key, detail[key])) seen.add(key) @@ -89,7 +93,7 @@ def _format_dict(status_code: int, detail: dict) -> str: for key, value in detail.items(): if key in seen or key in _WRAP_KEYS: continue - if value not in (None, ""): + if value is not None: lines.append(_kv_line(key, value)) seen.add(key) diff --git a/tests/test_cli_error_render.py b/tests/test_cli_error_render.py index b519750..99eaadc 100644 --- a/tests/test_cli_error_render.py +++ b/tests/test_cli_error_render.py @@ -100,3 +100,19 @@ def test_falls_back_when_detail_is_string(render_error): body = {"detail": "Only single SELECT queries are allowed"} out = render_error(400, body) assert "Only single SELECT" in out + + +def test_renders_empty_string_as_empty_marker(render_error): + """Devin Review iter #6: `billing_project: ""` in cross_project_forbidden + is the key diagnostic showing WHY the operator hits USER_PROJECT_DENIED. + The renderer must show empty strings as `(empty)`, not silently drop them.""" + body = {"detail": { + "kind": "cross_project_forbidden", + "billing_project": "", # the key diagnostic + "data_project": "prj-example", + "hint": "Set data_source.bigquery.billing_project", + }} + out = render_error(502, body) + assert "billing_project: (empty)" in out, \ + f"empty billing_project must render as (empty); got:\n{out}" + assert "data_project: prj-example" in out