From 1ca5618cbc7abd035fbdcaf5a78199f6632a66f3 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 25 Apr 2026 11:51:57 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20BigQuery=20finops=20UX=20=E2=80=94=20sur?= =?UTF-8?q?face=20bq=5Fregion,=20actionable=20errors,=20warehouse=5Fadd=20?= =?UTF-8?q?warning?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-.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) --- .../src/altimate/native/finops/bq-utils.ts | 56 +++ .../altimate/native/finops/credit-analyzer.ts | 14 +- .../altimate/native/finops/query-history.ts | 8 +- .../src/altimate/native/finops/role-access.ts | 8 +- .../native/finops/unused-resources.ts | 29 +- .../native/finops/warehouse-advisor.ts | 8 +- .../opencode/src/altimate/native/types.ts | 12 + .../src/altimate/tools/warehouse-add.ts | 22 + .../altimate/finops-bq-visibility.test.ts | 419 ++++++++++++++++++ 9 files changed, 563 insertions(+), 13 deletions(-) create mode 100644 packages/opencode/test/altimate/finops-bq-visibility.test.ts diff --git a/packages/opencode/src/altimate/native/finops/bq-utils.ts b/packages/opencode/src/altimate/native/finops/bq-utils.ts index a61ab1afb..cc8589bdd 100644 --- a/packages/opencode/src/altimate/native/finops/bq-utils.ts +++ b/packages/opencode/src/altimate/native/finops/bq-utils.ts @@ -55,3 +55,59 @@ export function interpolateBqRegion(sqlTemplate: string, bqRegion?: unknown): st export function bqRegionFor(warehouse: string): unknown { return Registry.getConfig(warehouse)?.location } + +/** + * Append a region-hint to a BigQuery error message when the error looks + * region-related (missing dataset, region-not-found, unknown INFORMATION_SCHEMA + * view). The hint tells the user which region the query ran against and how to + * change it — the root cause on non-US projects is almost always an unset or + * mis-typed `location` on the warehouse config. + * + * Returns the original error string unchanged when no region signal is present, + * so non-region failures (bad syntax, auth, quota) stay legible. + */ +export function augmentBqError(error: unknown, sanitizedRegion: string): string { + const msg = String(error) + // Signals we treat as region-related: + // 1. A backticked region-qualified INFORMATION_SCHEMA reference, which is + // what BigQuery emits when the dataset can't be resolved in the queried + // region: `region-eu.INFORMATION_SCHEMA.JOBS not found`. + // 2. A "Not found: ... INFORMATION_SCHEMA" line — covers cases where BQ + // mentions the view name but not the region prefix. + // 3. The literal "Not found: Dataset region-..." form some BQ surfaces use. + // Anchored to these specific shapes so unrelated text containing "region-" + // (e.g. "region-based routing denied", "multi-region-aware feature disabled") + // does NOT trigger the BigQuery-connection hint. + const hasRegionSignal = + /region-[a-z0-9]+[a-z0-9-]*\.INFORMATION_SCHEMA/i.test(msg) || + /Not\s+found:.*INFORMATION_SCHEMA/i.test(msg) || + /Not\s+found:\s*Dataset\s+region-[a-z0-9]+/i.test(msg) + if (!hasRegionSignal) return msg + // Idempotent — don't append twice if this helper runs in nested catches + if (msg.includes('set "location" on the BigQuery connection')) return msg + return `${msg} (queried region-${sanitizedRegion}; set "location" on the BigQuery connection to change this)` +} + +/** + * Detect errors raised by BigQuery when the caller lacks the org-level + * permissions required by views such as `INFORMATION_SCHEMA.TABLE_STORAGE` + * (used by `finops_unused_resources`). Most project-scoped service accounts + * lack `bigquery.resourceAdmin` at the org and hit this path first. + */ +export function isBqPermissionError(error: unknown): boolean { + const msg = String(error).toLowerCase() + // Word-boundary regex on `403` so `4031`, `40322`, `port 4030`, and similar + // numeric prefixes don't false-positive into the permission-error branch. + // The two `accessdenied` / `access denied` checks are intentional and not + // redundant — Google's BigQuery SDKs emit `AccessDenied` (camelCase), + // which `.toLowerCase()` collapses to `accessdenied` (no space) and would + // not be caught by the `access denied` substring check. + return ( + msg.includes("permission denied") || + msg.includes("access denied") || + msg.includes("accessdenied") || + msg.includes("bigquery.resourceadmin") || + msg.includes("iam permission") || + /\b403\b/.test(msg) + ) +} diff --git a/packages/opencode/src/altimate/native/finops/credit-analyzer.ts b/packages/opencode/src/altimate/native/finops/credit-analyzer.ts index 368ee95dd..9f7cbbb61 100644 --- a/packages/opencode/src/altimate/native/finops/credit-analyzer.ts +++ b/packages/opencode/src/altimate/native/finops/credit-analyzer.ts @@ -5,7 +5,7 @@ */ import * as Registry from "../connections/registry" -import { bqRegionFor, interpolateBqRegion } from "./bq-utils" +import { augmentBqError, bqRegionFor, interpolateBqRegion, sanitizeBqRegion } from "./bq-utils" import type { CreditAnalysisParams, CreditAnalysisResult, @@ -306,6 +306,7 @@ export async function analyzeCredits(params: CreditAnalysisParams): Promise 0 ? errors.join("; ") : undefined, + ...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }), } } catch (e) { return { @@ -228,7 +248,8 @@ export async function findUnusedResources(params: UnusedResourcesParams): Promis idle_warehouses: [], summary: {}, days_analyzed: days, - error: String(e), + error: sanitisedBqRegion ? augmentBqError(e, sanitisedBqRegion) : String(e), + ...(sanitisedBqRegion && { bq_region: sanitisedBqRegion }), } } } diff --git a/packages/opencode/src/altimate/native/finops/warehouse-advisor.ts b/packages/opencode/src/altimate/native/finops/warehouse-advisor.ts index 5352f48f7..727dfc44a 100644 --- a/packages/opencode/src/altimate/native/finops/warehouse-advisor.ts +++ b/packages/opencode/src/altimate/native/finops/warehouse-advisor.ts @@ -5,7 +5,7 @@ */ import * as Registry from "../connections/registry" -import { bqRegionFor, interpolateBqRegion } from "./bq-utils" +import { augmentBqError, bqRegionFor, interpolateBqRegion, sanitizeBqRegion } from "./bq-utils" import type { WarehouseAdvisorParams, WarehouseAdvisorResult, @@ -223,6 +223,7 @@ export async function adviseWarehouse(params: WarehouseAdvisorParams): Promise[] summary: Record warehouse_type?: string + /** For BigQuery warehouses: the sanitised region the query was executed against. */ + bq_region?: string error?: string } @@ -553,6 +555,8 @@ export interface CreditAnalysisResult { total_credits: number days_analyzed: number recommendations: Record[] + /** For BigQuery warehouses: the sanitised region the query was executed against. */ + bq_region?: string error?: string } @@ -569,6 +573,8 @@ export interface ExpensiveQueriesResult { queries: Record[] query_count: number days_analyzed: number + /** For BigQuery warehouses: the sanitised region the query was executed against. */ + bq_region?: string error?: string } @@ -585,6 +591,8 @@ export interface WarehouseAdvisorResult { warehouse_performance: Record[] recommendations: Record[] days_analyzed: number + /** For BigQuery warehouses: the sanitised region the query was executed against. */ + bq_region?: string error?: string } @@ -602,6 +610,8 @@ export interface UnusedResourcesResult { idle_warehouses: Record[] summary: Record days_analyzed: number + /** For BigQuery warehouses: the sanitised region the query was executed against. */ + bq_region?: string error?: string } @@ -619,6 +629,8 @@ export interface RoleGrantsResult { grants: Record[] grant_count: number privilege_summary: Record + /** For BigQuery warehouses: the sanitised region the query was executed against. */ + bq_region?: string error?: string } diff --git a/packages/opencode/src/altimate/tools/warehouse-add.ts b/packages/opencode/src/altimate/tools/warehouse-add.ts index 3758afaf1..f5c7d3b09 100644 --- a/packages/opencode/src/altimate/tools/warehouse-add.ts +++ b/packages/opencode/src/altimate/tools/warehouse-add.ts @@ -47,6 +47,28 @@ IMPORTANT: For private key file paths, always use "private_key_path" (not "priva // altimate_change start — append post-connect feature suggestions (async, non-blocking) let output = `Successfully added warehouse '${result.name}' (type: ${result.type}).\n\nUse warehouse_test to verify connectivity.` + // BigQuery connections without a `location` field silently fall back to + // the `us` region in finops tools (query-history / credit-analyzer / + // warehouse-advisor / unused-resources / role-grants all key off + // region-qualified `INFORMATION_SCHEMA.*` views). For non-US projects + // this produces empty results rather than a clear error. Warn once at + // add time so users notice before they hit an empty-looking dashboard. + // Also catches whitespace-only values — `sanitizeBqRegion` trims at + // query time and falls back to "us", so `location: " "` would + // otherwise pass this guard but query the wrong region. + const rawLocation = (args.config as Record).location + if ( + result.type === "bigquery" && + !String(rawLocation ?? "").trim() + ) { + output += + `\n\nWarning: no "location" set on this BigQuery connection. ` + + `Finops tools (query-history, credit-analyzer, warehouse-advisor, ` + + `unused-resources, role-grants) will query the \`us\` region by default. ` + + `If this project lives in \`eu\`, \`asia-northeast1\`, \`us-central1\`, ` + + `or any non-US region, re-add the connection with \`"location": ""\`.` + } + // Run suggestion gathering concurrently with a timeout to avoid // adding noticeable latency to the warehouse add response. try { diff --git a/packages/opencode/test/altimate/finops-bq-visibility.test.ts b/packages/opencode/test/altimate/finops-bq-visibility.test.ts new file mode 100644 index 000000000..5d7ac2818 --- /dev/null +++ b/packages/opencode/test/altimate/finops-bq-visibility.test.ts @@ -0,0 +1,419 @@ +/** + * Tests for the BigQuery finops UX + visibility improvements. + * + * Covers the items from #754 shipped in this PR: + * 1. `augmentBqError` — region-hint appended to BQ errors that look + * region-related (and only those), idempotent under double-wrapping. + * 2. `isBqPermissionError` — detects the TABLE_STORAGE 403 pattern that + * `finops_unused_resources` routinely hits on project-scoped SAs. + * 3. `warehouse_add` emits a non-fatal warning when a BigQuery connection + * is registered without a `location` field. + * 4. `getQueryHistory` + friends surface `bq_region` in their responses + * on the BigQuery branch (only). + */ + +import { describe, test, expect, mock, beforeEach, afterEach, afterAll, spyOn } from "bun:test" +import { SessionID, MessageID } from "../../src/session/schema" +import { Telemetry } from "../../src/telemetry" +import * as Dispatcher from "../../src/altimate/native/dispatcher" +import * as Registry from "../../src/altimate/native/connections/registry" +import { WarehouseAddTool } from "../../src/altimate/tools/warehouse-add" +import { augmentBqError, isBqPermissionError } from "../../src/altimate/native/finops/bq-utils" +import { getQueryHistory } from "../../src/altimate/native/finops/query-history" + +// --------------------------------------------------------------------------- +// 1. augmentBqError +// --------------------------------------------------------------------------- + +describe("augmentBqError", () => { + test("appends a region hint when the error mentions region-", () => { + const out = augmentBqError("Not found: Dataset region-eu:INFORMATION_SCHEMA", "eu") + expect(out).toContain("Not found: Dataset region-eu:INFORMATION_SCHEMA") + expect(out).toContain('set "location" on the BigQuery connection') + expect(out).toContain("region-eu") + }) + + test("appends a region hint on generic 'Not found: ... INFORMATION_SCHEMA' errors", () => { + const out = augmentBqError( + "Not found: Table my_project.INFORMATION_SCHEMA.JOBS", + "asia-northeast1", + ) + expect(out).toContain("region-asia-northeast1") + expect(out).toContain('set "location"') + }) + + test("passes non-region errors through unchanged", () => { + // Syntax errors, auth errors, quota errors — nothing region-related in the message + expect(augmentBqError("Syntax error at line 5: unexpected token", "us")).toBe( + "Syntax error at line 5: unexpected token", + ) + expect(augmentBqError("Quota exceeded: too many queries", "us")).toBe( + "Quota exceeded: too many queries", + ) + expect(augmentBqError("Invalid credentials", "us")).toBe("Invalid credentials") + }) + + test("is idempotent — never double-wraps the hint", () => { + const once = augmentBqError("Not found: Dataset region-eu:INFORMATION_SCHEMA", "eu") + const twice = augmentBqError(once, "eu") + expect(twice).toBe(once) + // Only one hint in the final message + const hintCount = (twice.match(/set "location"/g) ?? []).length + expect(hintCount).toBe(1) + }) + + test("accepts non-string errors (Error object, null, undefined, number)", () => { + const err = new Error("Not found: Dataset region-us:INFORMATION_SCHEMA.JOBS") + expect(augmentBqError(err, "us")).toContain('set "location"') + // Non-region/non-string inputs just round-trip through String() + expect(augmentBqError(null, "us")).toBe("null") + expect(augmentBqError(undefined, "us")).toBe("undefined") + expect(augmentBqError(42, "us")).toBe("42") + }) + + test("does not leak the sanitised region into unrelated error text", () => { + // The word "region" appears in unrelated contexts — don't trigger on those + expect(augmentBqError("Target region parameter missing for backup", "eu")).not.toContain( + 'set "location"', + ) + }) + + test("does not trigger on `region-` text that is not a BQ INFORMATION_SCHEMA reference", () => { + // Tightened in convergence: the regex used to match any "region-" + // and would falsely tag these as BQ region errors. After the fix the hint + // is appended only when the message contains a region-qualified + // INFORMATION_SCHEMA reference or a "Not found" line. + expect(augmentBqError("region-based routing policy denied", "us")).not.toContain( + 'set "location"', + ) + expect(augmentBqError("multi-region-aware feature disabled", "us")).not.toContain( + 'set "location"', + ) + expect(augmentBqError("Invalid region-europe parameter in config", "eu")).not.toContain( + 'set "location"', + ) + expect(augmentBqError("backup policy target region-eu is invalid", "us")).not.toContain( + 'set "location"', + ) + }) + + test("bare `region-` (zero chars after the hyphen) does not trigger the hint", () => { + // Pre-fix the regex used `*` which matched zero chars — `region-` alone + // would be tagged. After the fix this only matches with at least one + // [a-z0-9] char before the dot. + expect(augmentBqError("Configuration key region- is reserved", "us")).not.toContain( + 'set "location"', + ) + }) + + test("does trigger on canonical BQ region-qualified INFORMATION_SCHEMA references", () => { + expect( + augmentBqError( + "Not found: Table region-eu.INFORMATION_SCHEMA.JOBS", + "eu", + ), + ).toContain('set "location"') + expect( + augmentBqError( + "Could not resolve `region-asia-northeast1.INFORMATION_SCHEMA.JOBS`", + "asia-northeast1", + ), + ).toContain('set "location"') + }) + + test("triggers on `Not found: Dataset region-` shape", () => { + expect( + augmentBqError("Not found: Dataset region-eu was not found", "eu"), + ).toContain('set "location"') + }) +}) + +// --------------------------------------------------------------------------- +// 2. isBqPermissionError +// --------------------------------------------------------------------------- + +describe("isBqPermissionError", () => { + test("detects 'Permission denied' (case-insensitive)", () => { + expect(isBqPermissionError("Permission denied on resource project X")).toBe(true) + expect(isBqPermissionError("permission denied")).toBe(true) + expect(isBqPermissionError("PERMISSION DENIED")).toBe(true) + }) + + test("detects explicit bigquery.resourceAdmin mentions", () => { + expect( + isBqPermissionError( + "User does not have bigquery.resourceAdmin permission on organization", + ), + ).toBe(true) + }) + + test("detects 403 status code text", () => { + expect(isBqPermissionError("HTTP 403: forbidden")).toBe(true) + }) + + test("detects 'Access denied' variant used by Google APIs", () => { + expect(isBqPermissionError("Access Denied: BigQuery BigQuery")).toBe(true) + expect(isBqPermissionError("accessDenied")).toBe(true) + }) + + test("detects 'IAM permission' framing", () => { + expect(isBqPermissionError("IAM permission denied on foo")).toBe(true) + }) + + test("returns false for region/syntax/quota errors", () => { + expect(isBqPermissionError("Not found: Dataset region-eu")).toBe(false) + expect(isBqPermissionError("Syntax error at line 5")).toBe(false) + expect(isBqPermissionError("Quota exceeded")).toBe(false) + expect(isBqPermissionError("Invalid region: bogus")).toBe(false) + }) + + test("returns false for numeric prefixes containing `403` (word-boundary check)", () => { + // Pre-fix the substring match `includes("403")` would match all of these + // and falsely route non-permission errors to the TABLE_STORAGE/IAM hint. + // After the fix `\b403\b` is anchored on word boundaries. + expect(isBqPermissionError("Error code 4031 - rate limit exceeded")).toBe(false) + expect(isBqPermissionError("Request id 40322 failed")).toBe(false) + expect(isBqPermissionError("Connection failed on port 4030")).toBe(false) + expect(isBqPermissionError("Error code 1403 - timeout")).toBe(false) + expect(isBqPermissionError("Row 40314 has invalid data")).toBe(false) + }) + + test("returns true for canonical 403 surfaces (word-bounded)", () => { + expect(isBqPermissionError("HTTP 403 Forbidden")).toBe(true) + expect(isBqPermissionError("Status: 403 — denied")).toBe(true) + expect(isBqPermissionError("403 Forbidden: insufficient privileges")).toBe(true) + }) + + test("accepts non-string inputs via String() coercion", () => { + expect(isBqPermissionError(new Error("Permission denied"))).toBe(true) + expect(isBqPermissionError(null)).toBe(false) + expect(isBqPermissionError(undefined)).toBe(false) + // `String(403)` yields "403" with implicit boundaries at start/end of string, + // which `\b403\b` matches. This documents the helper's coercion contract. + expect(isBqPermissionError(403)).toBe(true) + }) +}) + +// --------------------------------------------------------------------------- +// 3. warehouse_add: BQ missing-location warning +// --------------------------------------------------------------------------- + +const ctx = { + sessionID: SessionID.make("ses_test"), + messageID: MessageID.make("msg_test"), + callID: "call_test", + agent: "build", + abort: AbortSignal.any([]), + messages: [], + metadata: () => {}, + ask: async () => {}, +} + +let dispatcherSpy: ReturnType + +function mockDispatcherCall(handler: (method: string, params: any) => Promise) { + dispatcherSpy?.mockRestore() + dispatcherSpy = spyOn(Dispatcher, "call").mockImplementation(handler as any) +} + +describe("warehouse_add — BigQuery missing-location warning", () => { + beforeEach(() => { + process.env.ALTIMATE_TELEMETRY_DISABLED = "true" + spyOn(Telemetry, "track").mockImplementation(() => {}) + spyOn(Telemetry, "getContext").mockReturnValue({ + sessionId: "test-session", + projectId: "", + } as any) + }) + + afterEach(() => { + dispatcherSpy?.mockRestore() + mock.restore() + }) + + afterAll(() => { + dispatcherSpy?.mockRestore() + delete process.env.ALTIMATE_TELEMETRY_DISABLED + }) + + test("warns when BigQuery connection is added without location", async () => { + mockDispatcherCall(async (method: string) => { + if (method === "warehouse.add") return { success: true, name: "bq-eu", type: "bigquery" } + if (method === "schema.cache_status") return { total_tables: 0 } + if (method === "warehouse.list") return { warehouses: [{ name: "bq-eu" }] } + return {} + }) + + const tool = await WarehouseAddTool.init() + const result = await tool.execute( + { name: "bq-eu", config: { type: "bigquery", project: "my-project" } }, + ctx as any, + ) + + expect(result.output).toContain("Successfully added warehouse") + expect(result.output).toContain('no "location" set') + expect(result.output).toContain("us") + expect(result.output).toContain("re-add") + // The suggestion block still runs + expect(result.metadata).toMatchObject({ success: true, type: "bigquery" }) + }) + + test("does NOT warn when BigQuery connection has location set", async () => { + mockDispatcherCall(async (method: string) => { + if (method === "warehouse.add") return { success: true, name: "bq-eu", type: "bigquery" } + if (method === "schema.cache_status") return { total_tables: 0 } + if (method === "warehouse.list") return { warehouses: [{ name: "bq-eu" }] } + return {} + }) + + const tool = await WarehouseAddTool.init() + const result = await tool.execute( + { name: "bq-eu", config: { type: "bigquery", project: "my-project", location: "eu" } }, + ctx as any, + ) + + expect(result.output).toContain("Successfully added warehouse") + expect(result.output).not.toContain('no "location" set') + }) + + test("does NOT warn for non-BigQuery warehouses (Snowflake, Postgres, etc.)", async () => { + mockDispatcherCall(async (method: string) => { + if (method === "warehouse.add") return { success: true, name: "sf", type: "snowflake" } + if (method === "schema.cache_status") return { total_tables: 0 } + if (method === "warehouse.list") return { warehouses: [{ name: "sf" }] } + return {} + }) + + const tool = await WarehouseAddTool.init() + const result = await tool.execute( + { + name: "sf", + config: { type: "snowflake", account: "xy12345", user: "admin", password: "pw" }, + }, + ctx as any, + ) + + expect(result.output).not.toContain('no "location" set') + }) + + test("empty-string location is treated as missing (falsy)", async () => { + mockDispatcherCall(async (method: string) => { + if (method === "warehouse.add") return { success: true, name: "bq", type: "bigquery" } + if (method === "schema.cache_status") return { total_tables: 0 } + if (method === "warehouse.list") return { warehouses: [{ name: "bq" }] } + return {} + }) + + const tool = await WarehouseAddTool.init() + const result = await tool.execute( + { name: "bq", config: { type: "bigquery", project: "p", location: "" } }, + ctx as any, + ) + + expect(result.output).toContain('no "location" set') + }) + + test("whitespace-only location triggers the warning (sanitizer would strip and fall back to us)", async () => { + // Convergence-driven: the original guard used !cfg.location (truthy-check), + // which would have passed for " ". sanitizeBqRegion trims and falls back + // to "us" at query time, so the user thinks they configured a region but + // silently queries US. Now caught at warehouse_add time. + mockDispatcherCall(async (method: string) => { + if (method === "warehouse.add") return { success: true, name: "bq", type: "bigquery" } + if (method === "schema.cache_status") return { total_tables: 0 } + if (method === "warehouse.list") return { warehouses: [{ name: "bq" }] } + return {} + }) + + const tool = await WarehouseAddTool.init() + const result = await tool.execute( + { name: "bq", config: { type: "bigquery", project: "p", location: " " } }, + ctx as any, + ) + + expect(result.output).toContain('no "location" set') + }) + + test("null location triggers the warning", async () => { + mockDispatcherCall(async (method: string) => { + if (method === "warehouse.add") return { success: true, name: "bq", type: "bigquery" } + if (method === "schema.cache_status") return { total_tables: 0 } + if (method === "warehouse.list") return { warehouses: [{ name: "bq" }] } + return {} + }) + + const tool = await WarehouseAddTool.init() + const result = await tool.execute( + { name: "bq", config: { type: "bigquery", project: "p", location: null as any } }, + ctx as any, + ) + + expect(result.output).toContain('no "location" set') + }) +}) + +// --------------------------------------------------------------------------- +// 4. finops getQueryHistory surfaces bq_region +// --------------------------------------------------------------------------- + +describe("getQueryHistory — bq_region in response", () => { + beforeEach(() => { + Registry.reset() + process.env.ALTIMATE_TELEMETRY_DISABLED = "true" + }) + + afterEach(() => { + Registry.reset() + delete process.env.ALTIMATE_TELEMETRY_DISABLED + }) + + test("bq_region appears on the error result when a BQ connector fails", async () => { + // No connector registered → Registry.get() will throw → we exercise + // the catch branch of getQueryHistory. The key invariant: the response + // still carries bq_region so the agent can tell which region was queried. + Registry.setConfigs({ + "bq-eu": { type: "bigquery", project: "p", location: "eu" } as any, + }) + + const result = await getQueryHistory({ warehouse: "bq-eu" }) + expect(result.success).toBe(false) + expect(result.bq_region).toBe("eu") + }) + + test("bq_region defaults to 'us' when BQ warehouse has no location set", async () => { + Registry.setConfigs({ + "bq-default": { type: "bigquery", project: "p" } as any, + }) + + const result = await getQueryHistory({ warehouse: "bq-default" }) + expect(result.success).toBe(false) + expect(result.bq_region).toBe("us") + }) + + test("bq_region sanitises malicious location values before exposing them", async () => { + Registry.setConfigs({ + "bq-evil": { type: "bigquery", project: "p", location: "us`; DROP TABLE X" } as any, + }) + + const result = await getQueryHistory({ warehouse: "bq-evil" }) + expect(result.bq_region).toBe("usdroptablex") + expect(result.bq_region).not.toContain("`") + expect(result.bq_region).not.toContain(";") + }) + + test("bq_region is absent for non-BigQuery warehouses", async () => { + Registry.setConfigs({ + "sf": { type: "snowflake", account: "xy", user: "u", password: "p" } as any, + }) + + const result = await getQueryHistory({ warehouse: "sf" }) + expect(result.bq_region).toBeUndefined() + }) + + test("bq_region is absent when no warehouse is registered (unknown type)", async () => { + Registry.setConfigs({}) + const result = await getQueryHistory({ warehouse: "nonexistent" }) + expect(result.success).toBe(false) + expect(result.bq_region).toBeUndefined() + }) +})