Skip to content

Commit 1ca5618

Browse files
anandgupta42claude
andcommitted
fix: BigQuery finops UX — surface bq_region, actionable errors, warehouse_add warning
Partial resolution of #754, focused on the four BigQuery UX/visibility items that naturally cluster. Three behavioural deferrals (#5 throw-on-invalid, #6 options-object refactor, #7 e2e silent-skip) remain open in #754. ## Changes 1. `bq_region` surfaced in every BigQuery finops tool response. `QueryHistoryResult`, `CreditAnalysisResult`, `ExpensiveQueriesResult`, `WarehouseAdvisorResult`, `UnusedResourcesResult`, and `RoleGrantsResult` gain an optional `bq_region?: string` field. All five finops modules compute `sanitisedBqRegion = sanitizeBqRegion(bqRegion)` once at entry and thread it through success and error returns. Non-BigQuery warehouses omit the field. Lets the agent (and humans inspecting tool output) see which region was actually queried after the silent `us` fallback shipped in v0.6.1 (#739). 2. `augmentBqError(error, region)` helper in `bq-utils.ts`. Appends a region hint to BigQuery errors that look region-related, anchored to `region-<x>.INFORMATION_SCHEMA` references and "Not found" lines so unrelated text (`region-based routing`, `multi-region-aware`) does not trigger. Idempotent — safe under nested catch wrapping. Wired into all five finops `catch` blocks. 3. `isBqPermissionError(error)` helper in `bq-utils.ts`. Detects the `INFORMATION_SCHEMA.TABLE_STORAGE` 403 pattern that `finops_unused_resources` hits routinely on project-scoped service accounts. Word-boundary `\b403\b` check so `4031`, `40322`, `port 4030` do not false-positive. `unused-resources.ts` produces an actionable message naming `bigquery.resourceAdmin` (org-level requirement) when this fires. 4. `warehouse_add` non-fatal warning when a BigQuery connection is registered without a `location` field. Catches whitespace-only values too — `sanitizeBqRegion` trims at query time and falls back to "us", so `location: " "` would otherwise pass a truthy-only guard but silently query the wrong region. ## Review notes Reviewed via `/consensus:code-review` with eight external models (GPT 5.4, Gemini 3.1 Pro, Kimi K2.5, MiniMax M2.7, GLM-5, Qwen 3.6 Plus, DeepSeek V3.2, MiMo-V2-Pro) plus Claude. 7 of 8 converged in round 1; Qwen's CLI did not return in time. Convergence corrected three accuracy issues from the initial draft: - Original draft claimed `includes("accessdenied")` was redundant after `.toLowerCase()`. MiniMax retracted this on second look — Google's BigQuery SDKs emit `AccessDenied` (camelCase), which lowercases to `accessdenied` (no space) and would not be caught by the `access denied` substring check. Both checks are kept. - Original draft attributed dead `?? "us"` to two locations. Verified by grep — only `unused-resources.ts:212` had it. Single fix applied. - Original draft listed several BigQuery `location` aliases (`Location`, `projectLocation`, `locationId`) as "auto-normalized" but `BIGQUERY_ALIASES` in `packages/drivers/src/normalize.ts` does not include any `location` alias. The check on `args.config.location` is correct for current aliases. The remaining real concern (whitespace trim) was downgraded and addressed. ## Tests 8 new tests in `finops-bq-visibility.test.ts` for the convergence-driven guards: `region-based routing`, `multi-region-aware`, bare `region-`, canonical INFORMATION_SCHEMA shapes, numeric-prefix `403` negative cases, canonical 403 surfaces, whitespace-only and null `location` warnings. 30/30 in this file pass; full altimate + skill suite 3236/3236. Verified: - `bun turbo typecheck`: 5/5 green - `bun run script/upstream/analyze.ts --markers --base main --strict`: clean - 3236 pass / 491 skipped (env-gated) / 0 fail across altimate + skill Closes #755 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 941c8ca commit 1ca5618

9 files changed

Lines changed: 563 additions & 13 deletions

File tree

packages/opencode/src/altimate/native/finops/bq-utils.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,3 +55,59 @@ export function interpolateBqRegion(sqlTemplate: string, bqRegion?: unknown): st
5555
export function bqRegionFor(warehouse: string): unknown {
5656
return Registry.getConfig(warehouse)?.location
5757
}
58+
59+
/**
60+
* Append a region-hint to a BigQuery error message when the error looks
61+
* region-related (missing dataset, region-not-found, unknown INFORMATION_SCHEMA
62+
* view). The hint tells the user which region the query ran against and how to
63+
* change it — the root cause on non-US projects is almost always an unset or
64+
* mis-typed `location` on the warehouse config.
65+
*
66+
* Returns the original error string unchanged when no region signal is present,
67+
* so non-region failures (bad syntax, auth, quota) stay legible.
68+
*/
69+
export function augmentBqError(error: unknown, sanitizedRegion: string): string {
70+
const msg = String(error)
71+
// Signals we treat as region-related:
72+
// 1. A backticked region-qualified INFORMATION_SCHEMA reference, which is
73+
// what BigQuery emits when the dataset can't be resolved in the queried
74+
// region: `region-eu.INFORMATION_SCHEMA.JOBS not found`.
75+
// 2. A "Not found: ... INFORMATION_SCHEMA" line — covers cases where BQ
76+
// mentions the view name but not the region prefix.
77+
// 3. The literal "Not found: Dataset region-..." form some BQ surfaces use.
78+
// Anchored to these specific shapes so unrelated text containing "region-"
79+
// (e.g. "region-based routing denied", "multi-region-aware feature disabled")
80+
// does NOT trigger the BigQuery-connection hint.
81+
const hasRegionSignal =
82+
/region-[a-z0-9]+[a-z0-9-]*\.INFORMATION_SCHEMA/i.test(msg) ||
83+
/Not\s+found:.*INFORMATION_SCHEMA/i.test(msg) ||
84+
/Not\s+found:\s*Dataset\s+region-[a-z0-9]+/i.test(msg)
85+
if (!hasRegionSignal) return msg
86+
// Idempotent — don't append twice if this helper runs in nested catches
87+
if (msg.includes('set "location" on the BigQuery connection')) return msg
88+
return `${msg} (queried region-${sanitizedRegion}; set "location" on the BigQuery connection to change this)`
89+
}
90+
91+
/**
92+
* Detect errors raised by BigQuery when the caller lacks the org-level
93+
* permissions required by views such as `INFORMATION_SCHEMA.TABLE_STORAGE`
94+
* (used by `finops_unused_resources`). Most project-scoped service accounts
95+
* lack `bigquery.resourceAdmin` at the org and hit this path first.
96+
*/
97+
export function isBqPermissionError(error: unknown): boolean {
98+
const msg = String(error).toLowerCase()
99+
// Word-boundary regex on `403` so `4031`, `40322`, `port 4030`, and similar
100+
// numeric prefixes don't false-positive into the permission-error branch.
101+
// The two `accessdenied` / `access denied` checks are intentional and not
102+
// redundant — Google's BigQuery SDKs emit `AccessDenied` (camelCase),
103+
// which `.toLowerCase()` collapses to `accessdenied` (no space) and would
104+
// not be caught by the `access denied` substring check.
105+
return (
106+
msg.includes("permission denied") ||
107+
msg.includes("access denied") ||
108+
msg.includes("accessdenied") ||
109+
msg.includes("bigquery.resourceadmin") ||
110+
msg.includes("iam permission") ||
111+
/\b403\b/.test(msg)
112+
)
113+
}

packages/opencode/src/altimate/native/finops/credit-analyzer.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66

77
import * as Registry from "../connections/registry"
8-
import { bqRegionFor, interpolateBqRegion } from "./bq-utils"
8+
import { augmentBqError, bqRegionFor, interpolateBqRegion, sanitizeBqRegion } from "./bq-utils"
99
import type {
1010
CreditAnalysisParams,
1111
CreditAnalysisResult,
@@ -306,6 +306,7 @@ export async function analyzeCredits(params: CreditAnalysisParams): Promise<Cred
306306
const days = params.days ?? 30
307307
const limit = params.limit ?? 50
308308
const bqRegion = whType === "bigquery" ? bqRegionFor(params.warehouse) : undefined
309+
const sanitisedBqRegion = whType === "bigquery" ? sanitizeBqRegion(bqRegion) : undefined
309310

310311
const dailyBuilt = buildCreditUsageSql(whType, days, limit, params.warehouse_filter, bqRegion)
311312
const summaryBuilt = buildCreditSummarySql(whType, days, bqRegion)
@@ -319,6 +320,7 @@ export async function analyzeCredits(params: CreditAnalysisParams): Promise<Cred
319320
days_analyzed: days,
320321
recommendations: [],
321322
error: `Credit analysis is not available for ${whType} warehouses.`,
323+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
322324
}
323325
}
324326

@@ -339,6 +341,7 @@ export async function analyzeCredits(params: CreditAnalysisParams): Promise<Cred
339341
total_credits: Math.round(totalCredits * 10000) / 10000,
340342
days_analyzed: days,
341343
recommendations,
344+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
342345
}
343346
} catch (e) {
344347
return {
@@ -348,7 +351,8 @@ export async function analyzeCredits(params: CreditAnalysisParams): Promise<Cred
348351
total_credits: 0,
349352
days_analyzed: days,
350353
recommendations: [],
351-
error: String(e),
354+
error: sanitisedBqRegion ? augmentBqError(e, sanitisedBqRegion) : String(e),
355+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
352356
}
353357
}
354358
}
@@ -358,6 +362,7 @@ export async function getExpensiveQueries(params: ExpensiveQueriesParams): Promi
358362
const days = params.days ?? 7
359363
const limit = params.limit ?? 20
360364
const bqRegion = whType === "bigquery" ? bqRegionFor(params.warehouse) : undefined
365+
const sanitisedBqRegion = whType === "bigquery" ? sanitizeBqRegion(bqRegion) : undefined
361366

362367
const built = buildExpensiveSql(whType, days, limit, bqRegion)
363368
if (!built) {
@@ -367,6 +372,7 @@ export async function getExpensiveQueries(params: ExpensiveQueriesParams): Promi
367372
query_count: 0,
368373
days_analyzed: days,
369374
error: `Expensive query analysis is not available for ${whType} warehouses.`,
375+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
370376
}
371377
}
372378

@@ -380,14 +386,16 @@ export async function getExpensiveQueries(params: ExpensiveQueriesParams): Promi
380386
queries,
381387
query_count: queries.length,
382388
days_analyzed: days,
389+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
383390
}
384391
} catch (e) {
385392
return {
386393
success: false,
387394
queries: [],
388395
query_count: 0,
389396
days_analyzed: days,
390-
error: String(e),
397+
error: sanitisedBqRegion ? augmentBqError(e, sanitisedBqRegion) : String(e),
398+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
391399
}
392400
}
393401
}

packages/opencode/src/altimate/native/finops/query-history.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66

77
import * as Registry from "../connections/registry"
8-
import { bqRegionFor, interpolateBqRegion, sanitizeBqRegion } from "./bq-utils"
8+
import { augmentBqError, bqRegionFor, interpolateBqRegion, sanitizeBqRegion } from "./bq-utils"
99
import type { QueryHistoryParams, QueryHistoryResult } from "../types"
1010

1111
// ---------------------------------------------------------------------------
@@ -214,6 +214,7 @@ export async function getQueryHistory(params: QueryHistoryParams): Promise<Query
214214
const days = params.days ?? 7
215215
const limit = params.limit ?? 100
216216
const bqRegion = whType === "bigquery" ? bqRegionFor(params.warehouse) : undefined
217+
const sanitisedBqRegion = whType === "bigquery" ? sanitizeBqRegion(bqRegion) : undefined
217218

218219
const built = buildHistoryQuery(whType, days, limit, params.user, params.warehouse_filter, bqRegion)
219220
if (!built) {
@@ -222,6 +223,7 @@ export async function getQueryHistory(params: QueryHistoryParams): Promise<Query
222223
queries: [],
223224
summary: {},
224225
error: `Query history is not available for ${whType} warehouses.`,
226+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
225227
}
226228
}
227229

@@ -255,13 +257,15 @@ export async function getQueryHistory(params: QueryHistoryParams): Promise<Query
255257
queries,
256258
summary,
257259
warehouse_type: whType,
260+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
258261
}
259262
} catch (e) {
260263
return {
261264
success: false,
262265
queries: [],
263266
summary: {},
264-
error: String(e),
267+
error: sanitisedBqRegion ? augmentBqError(e, sanitisedBqRegion) : String(e),
268+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
265269
}
266270
}
267271
}

packages/opencode/src/altimate/native/finops/role-access.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66

77
import * as Registry from "../connections/registry"
8-
import { bqRegionFor, interpolateBqRegion } from "./bq-utils"
8+
import { augmentBqError, bqRegionFor, interpolateBqRegion, sanitizeBqRegion } from "./bq-utils"
99
import type {
1010
RoleGrantsParams,
1111
RoleGrantsResult,
@@ -167,6 +167,7 @@ export async function queryGrants(params: RoleGrantsParams): Promise<RoleGrantsR
167167
const whType = getWhType(params.warehouse)
168168
const limit = params.limit ?? 100
169169
const bqRegion = whType === "bigquery" ? bqRegionFor(params.warehouse) : undefined
170+
const sanitisedBqRegion = whType === "bigquery" ? sanitizeBqRegion(bqRegion) : undefined
170171

171172
const built = buildGrantsSql(whType, params.role, params.object_name, limit, bqRegion)
172173
if (!built) {
@@ -176,6 +177,7 @@ export async function queryGrants(params: RoleGrantsParams): Promise<RoleGrantsR
176177
grant_count: 0,
177178
privilege_summary: {},
178179
error: `Role/access queries are not available for ${whType} warehouses.`,
180+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
179181
}
180182
}
181183

@@ -195,14 +197,16 @@ export async function queryGrants(params: RoleGrantsParams): Promise<RoleGrantsR
195197
grants,
196198
grant_count: grants.length,
197199
privilege_summary: privilegeSummary,
200+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
198201
}
199202
} catch (e) {
200203
return {
201204
success: false,
202205
grants: [],
203206
grant_count: 0,
204207
privilege_summary: {},
205-
error: String(e),
208+
error: sanitisedBqRegion ? augmentBqError(e, sanitisedBqRegion) : String(e),
209+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
206210
}
207211
}
208212
}

packages/opencode/src/altimate/native/finops/unused-resources.ts

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,13 @@
55
*/
66

77
import * as Registry from "../connections/registry"
8-
import { bqRegionFor, interpolateBqRegion } from "./bq-utils"
8+
import {
9+
augmentBqError,
10+
bqRegionFor,
11+
interpolateBqRegion,
12+
isBqPermissionError,
13+
sanitizeBqRegion,
14+
} from "./bq-utils"
915
import type {
1016
UnusedResourcesParams,
1117
UnusedResourcesResult,
@@ -145,6 +151,8 @@ export async function findUnusedResources(params: UnusedResourcesParams): Promis
145151
const whType = getWhType(params.warehouse)
146152
const days = params.days ?? 30
147153
const limit = params.limit ?? 50
154+
const bqRegion = whType === "bigquery" ? bqRegionFor(params.warehouse) : undefined
155+
const sanitisedBqRegion = whType === "bigquery" ? sanitizeBqRegion(bqRegion) : undefined
148156

149157
if (!["snowflake", "bigquery", "databricks"].includes(whType)) {
150158
return {
@@ -187,11 +195,22 @@ export async function findUnusedResources(params: UnusedResourcesParams): Promis
187195
}
188196
} else if (whType === "bigquery") {
189197
try {
190-
const sql = interpolateBqRegion(BIGQUERY_UNUSED_TABLES_SQL, bqRegionFor(params.warehouse))
198+
const sql = interpolateBqRegion(BIGQUERY_UNUSED_TABLES_SQL, bqRegion)
191199
const result = await connector.execute(sql, limit, [days, limit])
192200
unusedTables = rowsToRecords(result)
193201
} catch (e) {
194-
errors.push(`Could not query unused tables: ${e}`)
202+
// TABLE_STORAGE is an org-level view. Most project-scoped service
203+
// accounts lack bigquery.resourceAdmin at the org and hit 403 here —
204+
// point them at the specific permission rather than a raw SQL error.
205+
if (isBqPermissionError(e)) {
206+
errors.push(
207+
`Could not query unused tables: BigQuery returned a permission error for INFORMATION_SCHEMA.TABLE_STORAGE. ` +
208+
`This view is org-level and requires bigquery.resourceAdmin (or equivalent) at the organisation. ` +
209+
`Project-scoped service accounts typically can't read it. Raw error: ${e}`,
210+
)
211+
} else {
212+
errors.push(`Could not query unused tables: ${augmentBqError(e, sanitisedBqRegion!)}`)
213+
}
195214
}
196215
} else if (whType === "databricks") {
197216
try {
@@ -220,6 +239,7 @@ export async function findUnusedResources(params: UnusedResourcesParams): Promis
220239
},
221240
days_analyzed: days,
222241
error: errors.length > 0 ? errors.join("; ") : undefined,
242+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
223243
}
224244
} catch (e) {
225245
return {
@@ -228,7 +248,8 @@ export async function findUnusedResources(params: UnusedResourcesParams): Promis
228248
idle_warehouses: [],
229249
summary: {},
230250
days_analyzed: days,
231-
error: String(e),
251+
error: sanitisedBqRegion ? augmentBqError(e, sanitisedBqRegion) : String(e),
252+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
232253
}
233254
}
234255
}

packages/opencode/src/altimate/native/finops/warehouse-advisor.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66

77
import * as Registry from "../connections/registry"
8-
import { bqRegionFor, interpolateBqRegion } from "./bq-utils"
8+
import { augmentBqError, bqRegionFor, interpolateBqRegion, sanitizeBqRegion } from "./bq-utils"
99
import type {
1010
WarehouseAdvisorParams,
1111
WarehouseAdvisorResult,
@@ -223,6 +223,7 @@ export async function adviseWarehouse(params: WarehouseAdvisorParams): Promise<W
223223
const whType = getWhType(params.warehouse)
224224
const days = params.days ?? 14
225225
const bqRegion = whType === "bigquery" ? bqRegionFor(params.warehouse) : undefined
226+
const sanitisedBqRegion = whType === "bigquery" ? sanitizeBqRegion(bqRegion) : undefined
226227

227228
const loadSql = buildLoadSql(whType, days, bqRegion)
228229
const sizingSql = buildSizingSql(whType, days, bqRegion)
@@ -235,6 +236,7 @@ export async function adviseWarehouse(params: WarehouseAdvisorParams): Promise<W
235236
recommendations: [],
236237
days_analyzed: days,
237238
error: `Warehouse sizing advice is not available for ${whType} warehouses.`,
239+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
238240
}
239241
}
240242

@@ -276,6 +278,7 @@ export async function adviseWarehouse(params: WarehouseAdvisorParams): Promise<W
276278
warehouse_performance: sizingData,
277279
recommendations,
278280
days_analyzed: days,
281+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
279282
}
280283
} catch (e) {
281284
return {
@@ -284,7 +287,8 @@ export async function adviseWarehouse(params: WarehouseAdvisorParams): Promise<W
284287
warehouse_performance: [],
285288
recommendations: [],
286289
days_analyzed: days,
287-
error: String(e),
290+
error: sanitisedBqRegion ? augmentBqError(e, sanitisedBqRegion) : String(e),
291+
...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }),
288292
}
289293
}
290294
}

packages/opencode/src/altimate/native/types.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,8 @@ export interface QueryHistoryResult {
534534
queries: Record<string, unknown>[]
535535
summary: Record<string, unknown>
536536
warehouse_type?: string
537+
/** For BigQuery warehouses: the sanitised region the query was executed against. */
538+
bq_region?: string
537539
error?: string
538540
}
539541

@@ -553,6 +555,8 @@ export interface CreditAnalysisResult {
553555
total_credits: number
554556
days_analyzed: number
555557
recommendations: Record<string, unknown>[]
558+
/** For BigQuery warehouses: the sanitised region the query was executed against. */
559+
bq_region?: string
556560
error?: string
557561
}
558562

@@ -569,6 +573,8 @@ export interface ExpensiveQueriesResult {
569573
queries: Record<string, unknown>[]
570574
query_count: number
571575
days_analyzed: number
576+
/** For BigQuery warehouses: the sanitised region the query was executed against. */
577+
bq_region?: string
572578
error?: string
573579
}
574580

@@ -585,6 +591,8 @@ export interface WarehouseAdvisorResult {
585591
warehouse_performance: Record<string, unknown>[]
586592
recommendations: Record<string, unknown>[]
587593
days_analyzed: number
594+
/** For BigQuery warehouses: the sanitised region the query was executed against. */
595+
bq_region?: string
588596
error?: string
589597
}
590598

@@ -602,6 +610,8 @@ export interface UnusedResourcesResult {
602610
idle_warehouses: Record<string, unknown>[]
603611
summary: Record<string, unknown>
604612
days_analyzed: number
613+
/** For BigQuery warehouses: the sanitised region the query was executed against. */
614+
bq_region?: string
605615
error?: string
606616
}
607617

@@ -619,6 +629,8 @@ export interface RoleGrantsResult {
619629
grants: Record<string, unknown>[]
620630
grant_count: number
621631
privilege_summary: Record<string, number>
632+
/** For BigQuery warehouses: the sanitised region the query was executed against. */
633+
bq_region?: string
622634
error?: string
623635
}
624636

0 commit comments

Comments
 (0)