fix: BigQuery finops UX — surface bq_region, actionable errors, warehouse_add warning#756
Open
anandgupta42 wants to merge 1 commit intomainfrom
Open
fix: BigQuery finops UX — surface bq_region, actionable errors, warehouse_add warning#756anandgupta42 wants to merge 1 commit intomainfrom
anandgupta42 wants to merge 1 commit intomainfrom
Conversation
…ouse_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>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThis PR adds BigQuery-specific error diagnostics and region visibility across finops tools. It introduces two utility functions for analyzing region-related errors and detecting permission failures, propagates the queried Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/src/altimate/native/finops/unused-resources.ts (1)
196-213: Minor: drop the non-null assertion onsanitisedBqRegion.Inside the
whType === "bigquery"branch,sanitisedBqRegionis guaranteed to be astringby construction (line 155). Rather than relying on a!assertion, narrow it locally so the type system enforces the invariant:♻️ Suggested narrowing
} else if (whType === "bigquery") { + const region = sanitisedBqRegion ?? "us" try { const sql = interpolateBqRegion(BIGQUERY_UNUSED_TABLES_SQL, bqRegion) const result = await connector.execute(sql, limit, [days, limit]) unusedTables = rowsToRecords(result) } catch (e) { // TABLE_STORAGE is an org-level view. Most project-scoped service // accounts lack bigquery.resourceAdmin at the org and hit 403 here — // point them at the specific permission rather than a raw SQL error. if (isBqPermissionError(e)) { errors.push( `Could not query unused tables: BigQuery returned a permission error for INFORMATION_SCHEMA.TABLE_STORAGE. ` + `This view is org-level and requires bigquery.resourceAdmin (or equivalent) at the organisation. ` + `Project-scoped service accounts typically can't read it. Raw error: ${e}`, ) } else { - errors.push(`Could not query unused tables: ${augmentBqError(e, sanitisedBqRegion!)}`) + errors.push(`Could not query unused tables: ${augmentBqError(e, region)}`) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/native/finops/unused-resources.ts` around lines 196 - 213, Remove the non-null assertion on sanitisedBqRegion in the bigquery error branch: since sanitisedBqRegion is already guaranteed to be a string earlier, narrow it locally (e.g., const region = sanitisedBqRegion) inside the whType === "bigquery" branch and pass that region variable to augmentBqError(e, region) instead of augmentBqError(e, sanitisedBqRegion!); update the error push that uses augmentBqError accordingly so no "!" is needed.packages/opencode/test/altimate/finops-bq-visibility.test.ts (1)
1-419: LGTM — well-scoped coverage of the four UX/visibility items.The test file does a careful job exercising each item from
#755:
augmentBqError: regex tightening tests (lines 81–107) explicitly lock in the post-convergence anchoring (region-<x>.INFORMATION_SCHEMAandNot found:shapes only) and idempotency (lines 56–63) — these will catch any future regression where the regex relaxes to plainregion-<word>.isBqPermissionError: the\b403\bword-boundary tests (lines 170–185) and the documentedString(403) → "403"coercion (lines 191–194) clearly capture the contract.warehouse_add: positive/negative coverage for empty, whitespace, null, present, and non-BQ types (lines 239–352); useful that the whitespace case is asserted explicitly with the convergence rationale inline.getQueryHistory: the success/error/sanitisation/non-BQ/unknown-warehouse matrix (lines 370–418) pins down the response invariant cleanly, andRegistry.reset()in bothbeforeEach/afterEachkeeps the suite isolated.A few small, optional observations (none blocking):
- Lines 229–232:
mock.restore()already restores every activespyOn, sodispatcherSpy?.mockRestore()in the sameafterEachis redundant. Harmless, but you could drop the explicit call (and the module-levellet dispatcherSpy) and just rely onmock.restore().- Lines 220–237:
ALTIMATE_TELEMETRY_DISABLEDis only deleted inafterAllfor thisdescribe. Per-test cleanup (mirroring thegetQueryHistoryblock at lines 365–368) would make the suite slightly more hermetic if a test in this block ever throws before reachingafterAll.- Line 399:
expect(result.bq_region).toBe("usdroptablex")is a very tight coupling to the current sanitiser implementation (lower-case + strip non-[a-z0-9]). If you ever switch the sanitiser to keep hyphens or use a different replacement char, this test will break for reasons unrelated to the security property under test. The twonot.toContainassertions on the next lines already capture the security invariant; consider keeping those as the primary contract and treating the exact-match as documentation only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/finops-bq-visibility.test.ts` around lines 1 - 419, Remove the redundant module-level dispatcherSpy and its explicit dispatcherSpy?.mockRestore() calls in the warehouse_add suite and rely solely on mock.restore(); in practice delete the let dispatcherSpy declaration and any spyRestore lines (references: dispatcherSpy, mock.restore()). Ensure ALTIMATE_TELEMETRY_DISABLED is cleaned up per-test by moving the env deletion into the warehouse_add afterEach (references: process.env.ALTIMATE_TELEMETRY_DISABLED, beforeEach/afterEach in the "warehouse_add" describe). Relax the brittle exact-match assertion in the getQueryHistory sanitisation test (reference: expect(result.bq_region).toBe("usdroptablex")) so it only asserts the security contract (e.g., no backticks/semicolons and normalized alphanumeric output) rather than a specific sanitized string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/src/altimate/native/finops/unused-resources.ts`:
- Around line 196-213: Remove the non-null assertion on sanitisedBqRegion in the
bigquery error branch: since sanitisedBqRegion is already guaranteed to be a
string earlier, narrow it locally (e.g., const region = sanitisedBqRegion)
inside the whType === "bigquery" branch and pass that region variable to
augmentBqError(e, region) instead of augmentBqError(e, sanitisedBqRegion!);
update the error push that uses augmentBqError accordingly so no "!" is needed.
In `@packages/opencode/test/altimate/finops-bq-visibility.test.ts`:
- Around line 1-419: Remove the redundant module-level dispatcherSpy and its
explicit dispatcherSpy?.mockRestore() calls in the warehouse_add suite and rely
solely on mock.restore(); in practice delete the let dispatcherSpy declaration
and any spyRestore lines (references: dispatcherSpy, mock.restore()). Ensure
ALTIMATE_TELEMETRY_DISABLED is cleaned up per-test by moving the env deletion
into the warehouse_add afterEach (references:
process.env.ALTIMATE_TELEMETRY_DISABLED, beforeEach/afterEach in the
"warehouse_add" describe). Relax the brittle exact-match assertion in the
getQueryHistory sanitisation test (reference:
expect(result.bq_region).toBe("usdroptablex")) so it only asserts the security
contract (e.g., no backticks/semicolons and normalized alphanumeric output)
rather than a specific sanitized string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7ceccd2e-be94-4ccf-9950-9bc1d527fb86
📒 Files selected for processing (9)
packages/opencode/src/altimate/native/finops/bq-utils.tspackages/opencode/src/altimate/native/finops/credit-analyzer.tspackages/opencode/src/altimate/native/finops/query-history.tspackages/opencode/src/altimate/native/finops/role-access.tspackages/opencode/src/altimate/native/finops/unused-resources.tspackages/opencode/src/altimate/native/finops/warehouse-advisor.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/warehouse-add.tspackages/opencode/test/altimate/finops-bq-visibility.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/native/finops/bq-utils.ts">
<violation number="1" location="packages/opencode/src/altimate/native/finops/bq-utils.ts:111">
P2: `isBqPermissionError` is too broad: matching any `403` can misclassify non-permission BigQuery errors as permission issues.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| msg.includes("accessdenied") || | ||
| msg.includes("bigquery.resourceadmin") || | ||
| msg.includes("iam permission") || | ||
| /\b403\b/.test(msg) |
There was a problem hiding this comment.
P2: isBqPermissionError is too broad: matching any 403 can misclassify non-permission BigQuery errors as permission issues.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/native/finops/bq-utils.ts, line 111:
<comment>`isBqPermissionError` is too broad: matching any `403` can misclassify non-permission BigQuery errors as permission issues.</comment>
<file context>
@@ -55,3 +55,59 @@ export function interpolateBqRegion(sqlTemplate: string, bqRegion?: unknown): st
+ msg.includes("accessdenied") ||
+ msg.includes("bigquery.resourceadmin") ||
+ msg.includes("iam permission") ||
+ /\b403\b/.test(msg)
+ )
+}
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Surfaces
bq_regionin every BigQuery finops tool response, adds two helpers inbq-utils.ts(augmentBqErrorfor region-related errors,isBqPermissionErrorfor theTABLE_STORAGE403 pattern), and warns inwarehouse_addwhen a BigQuery connection is registered without alocationfield (or with a whitespace-only value). Builds on the v0.6.1 multi-region work in #739; addresses the four UX/visibility items naturally clustered out of the seven follow-ups in #754.Type of change
Issue for this PR
Closes #755 (focused sub-issue split out of the broader #754 follow-up tracker — three behavioural items remain in #754).
How did you verify your code works?
packages/opencode/test/altimate/finops-bq-visibility.test.tscovering:augmentBqErrorpositive/negative paths (canonical INFORMATION_SCHEMA refs,Not found: Dataset region-...,region-based routing,multi-region-aware, bareregion-, idempotency, non-string inputs),isBqPermissionErrorpositive/negative paths (Permission denied,accessDenied,bigquery.resourceAdmin,IAM permission, canonical 403, numeric-prefix 4031/40322/port-4030 false-positives),warehouse_addwarning across BigQuery / Snowflake / explicit-location / empty-string / whitespace-only / null cases, andgetQueryHistoryend-to-endbq_regionsurfacing on success and error paths.bun turbo typecheck): 5/5 packages green./consensus:code-reviewwith 8 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/8 converged round 1; convergence corrected three accuracy issues from the initial draft (retractedaccessdeniedredundancy claim, fixed dead-code citation, dropped unverified-alias claims) — see commit body for details.Checklist
🤖 Generated with Claude Code
Summary by cubic
Shows the BigQuery region in all finops responses and makes region/permission errors actionable. Also warns on
warehouse_addwhen a BigQuery connection has nolocationto avoid silent US fallback.bq_region(sanitized) to all BigQuery finops results on success and error paths (query-history,credit-analyzer,expensive-queries,warehouse-advisor,unused-resources,role-access).augmentBqErrorto append a clear region hint to region-related BigQuery errors, andisBqPermissionErrorto detectINFORMATION_SCHEMA.TABLE_STORAGE403s with guidance onbigquery.resourceAdmin.warehouse_addto warn when adding a BigQuery connection without alocation(including whitespace-only).Written for commit 1ca5618. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests