AltimateAI / AltimateAI/altimate-code

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

Aperta
#754 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
TypeScript
Stelle
811
Fork
134
Merge medio
3g 2h
PR unite (30g)
50

Descrizione

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.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.