fix(cli): #168 review iter 6 — render empty-string diagnostics

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

@@ -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

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)
 

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