AltimateAI / AltimateAI/altimate-code

BigQuery finops: UX + robustness follow-ups from v0.6.1 review

Ouverte
#754 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub
Langage dominant
TypeScript
Étoiles
811
Forks
134
Merge moyen
3 j 2 h
PR mergées (30 j)
50

Description

Deferred items surfaced during the v0.6.1 release review. All relate to the BigQuery finops multi-region work shipped in #739 — the fix is correct and shipped, these are UX and robustness follow-ups that were out of scope for a patch release.

## UX / visibility

**1. Surface queried region in BQ finops tool responses.** `sanitizeBqRegion` silently falls back to `us` when `location` is missing on the warehouse config. The agent has no way to see which region was actually queried, so an EU user with an unset `location` sees empty results and can't tell whether that's "no activity" or "wrong region." Proposal: add a `bq_region` / `region_used` field to the top-level response of `finops_query_history`, `finops_analyze_credits`, `finops_warehouse_advisor`, `finops_role_access`, and `finops_unused_resources` on the BigQuery branch.

**2. Warn at `warehouse_add` time when a BigQuery connection is registered without `location`.** Non-fatal warning, not an error. Single log line pointing at where `location` is documented.

**3. Friendlier error for `finops_unused_resources` 403 on BigQuery.** `INFORMATION_SCHEMA.TABLE_STORAGE` is org-level and most project-scoped service accounts 403 on it. Currently users get a raw exec error; detect the permission class and point them at `bigquery.resourceAdmin`.

**4. Region-hint on BQ error messages.** In the three `catch (e)` blocks across `query-history.ts`, `credit-analyzer.ts`, `warehouse-advisor.ts`, if the error mentions `region-` or `Not found`, append `(queried region-${region}; set "location" on the warehouse config to change this)`.

## Robustness

**5. Distinguish "no location provided" vs "location provided but sanitized to empty".** `sanitizeBqRegion` currently returns `"us"` in both cases. Compliance concern for EU/APAC tenants where an invalid `location` silently downgrades to US. Proposal: return `"us"` when input is `undefined`/`null`/`""`, but throw with an actionable message when a non-empty input sanitizes to empty (e.g. someone sets `location: "💥"`).

## Code structure

**6. Refactor BQ finops builders to options-object.** `buildGrantsSql("bigquery", undefined, undefined, 100, "us-central1")` now has 5–6 positional args across several builders. Not blocking, but getting hard to read.

## Test quality

**7. E2E test silent-skip warning.** `finops-bigquery-e2e.test.ts` wraps assertions in `if (result.success)` — if the service account lacks perms or region, the test "passes" with zero assertions run. Emit a warning when `result.success` is false but no rows came back, so hidden no-ops are visible in test output.

## Related

- #740 — PINEAPPLE blocked-term / TEAM_MEMBERS carve-out in anti-slop (already tracked).

Flagged by the five-persona review (CTO, PM, Data Engineer, Tech Lead, Chaos Gremlin) on the v0.6.1 release.

Guide de contribution

Ouvrir le guide de contribution

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.