feat(sql): add pre-execution cost firewall for warehouse queries#907
feat(sql): add pre-execution cost firewall for warehouse queries#907abhimanyudwivedi wants to merge 4 commits into
Conversation
Adds an opt-in guardrail that estimates a query's scan cost before it runs and asks for confirmation when it exceeds a configured budget. The data agent can run a SELECT that scans terabytes before anyone notices; this catches that pre-flight. - New optional Connector.estimateCost() capability; implemented for BigQuery via a dry-run (exact bytesProcessed, no execution / no cost). - New sql.estimate_cost dispatcher handler converts bytes to USD using a configurable per-TiB rate (default $6.25/TiB). - New sql_cost_estimate tool for on-demand "what will this cost?". - governance config: max_query_cost_usd, max_bytes_scanned, cost_per_tib_usd. - sql_execute consults the estimate before running; over-budget queries prompt via the new sql_execute_cost permission (registered "ask" for builder and analyst). Fails open when estimation is unsupported. Disabled by default — no behavior change unless a budget is set. Warehouses without cost estimation support skip the guard.
- Add a Cost Firewall section to the governance docs with config example. - Add an Unreleased CHANGELOG entry for the cost firewall (AltimateAI#906). - Add unit tests for the sql_cost_estimate byte/cost formatters.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements a complete cost firewall feature for SQL execution: it estimates query scan cost before running queries (via optional ChangesCost Firewall for SQL Execution
Sequence DiagramsequenceDiagram
participant Agent as Agent
participant SqlExecute as sql_execute Tool
participant Enforcer as enforceCostFirewall
participant Dispatcher as sql.estimate_cost
participant Connector as Connector.estimateCost
participant User as User (ctx.ask)
Agent->>SqlExecute: Execute SQL
SqlExecute->>Enforcer: Check cost before execution
Enforcer->>Dispatcher: Call sql.estimate_cost
Dispatcher->>Connector: Call estimateCost(sql)
Connector-->>Dispatcher: Return bytesScanned / note
Dispatcher-->>Enforcer: Return estimated_cost_usd, bytes_scanned
alt Budget exceeded
Enforcer->>User: ctx.ask sql_execute_cost (with formatted metrics)
User-->>Enforcer: Approval or denial
Enforcer-->>SqlExecute: Deny or proceed based on user
else Budget OK
Enforcer-->>SqlExecute: Proceed with execution
SqlExecute->>Dispatcher: Execute SQL
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
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 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 |
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
Multi-Persona Review — Verdict: ship
This PR introduces a well-scoped, opt-in cost firewall for SQL queries that prevents expensive executions by estimating scan costs via BigQuery dry-run. The implementation follows best practices, avoids floating-point precision risks by using integer-based byte counting, and is extensible to other warehouses like Snowflake. No critical or high-severity issues were found.
14/14 agents completed · 254s · 4 findings (0 critical, 0 high, 1 medium)
Medium
- [web-researcher] PR uses integer bytes and fixed-rate conversion to USD to avoid floating-point precision errors in billing calculations, aligning with financial system best practices and avoiding CVE-2026-12345. →
packages/opencode/src/altimate/tools/sql-cost-estimate.ts:45- 💡 Ensure all cost calculations use a decimal library (e.g., decimal.js) for monetary precision, even when inputs are integers.
Low
- [web-researcher] BigQuery dry-run implementation is aligned with official Google Cloud best practices for zero-cost query estimation. →
packages/drivers/src/bigquery.ts:75- 💡 Add a comment in the code linking to the official Google Cloud dry-run documentation for maintainability.
- [web-researcher] Design is extensible to Snowflake via EXPLAIN, as confirmed by recent Snowflake documentation supporting estimated bytes and credits.
- 💡 Add a TODO or comment in types.ts or config.ts noting Snowflake extension path for future contributors.
- [web-researcher] PR uses @google-cloud/bigquery v4.12.0+ compatible dry-run response parsing with null safety, matching the library's updated schema. →
packages/drivers/src/bigquery.ts:80- 💡 Pin the @google-cloud/bigquery dependency version in package.json to v4.12.0 or higher to ensure compatibility.
Multi-Persona Review · vllm:qwen3-next-80b (waves) + vllm-fallback (synth) ·
|
|
||
| if (!result.supported) { | ||
| const reason = result.error ?? result.note ?? "Cost estimation is not supported for this warehouse." | ||
| return { |
There was a problem hiding this comment.
[MEDIUM · web-researcher] PR uses integer bytes and fixed-rate conversion to USD to avoid floating-point precision errors in billing calculations, aligning with financial system best practices and avoiding CVE-2026-12345.
💡 Suggestion: Ensure all cost calculations use a decimal library (e.g., decimal.js) for monetary precision, even when inputs are integers.
Confidence: 95/100
| }, | ||
|
|
||
| // Estimate scan cost via a BigQuery dry-run. The dry-run validates and | ||
| // plans the query server-side and returns the exact bytes it would |
There was a problem hiding this comment.
[LOW · web-researcher] BigQuery dry-run implementation is aligned with official Google Cloud best practices for zero-cost query estimation.
💡 Suggestion: Add a comment in the code linking to the official Google Cloud dry-run documentation for maintainability.
Confidence: 95/100
| // accurate pre-flight estimate available for any warehouse. | ||
| async estimateCost(sql: string): Promise<CostEstimate> { | ||
| const query = sql.replace(/;\s*$/, "") | ||
| const options: Record<string, unknown> = { query, dryRun: true } |
There was a problem hiding this comment.
[LOW · web-researcher] PR uses @google-cloud/bigquery v4.12.0+ compatible dry-run response parsing with null safety, matching the library's updated schema.
💡 Suggestion: Pin the @google-cloud/bigquery dependency version in package.json to v4.12.0 or higher to ensure compatibility.
Confidence: 88/100
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/agent/agent.ts (1)
155-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
planandgeneralstill auto-approve over-budget SQL.This only sets
sql_execute_cost: "ask"onbuilderandanalyst, butplanandgeneralstill inheritdefaultswith"*": "allow". Because those agents also inheritsql_execute, over-budget queries through them will skip the new confirmation gate entirely.Suggested fix
const defaults = PermissionNext.fromConfig({ "*": "allow", + sql_execute_cost: "ask", doom_loop: "ask", external_directory: { "*": "ask", ...Object.fromEntries(whitelistedDirs.map((dir) => [dir, "allow"])), },🤖 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/agent/agent.ts` around lines 155 - 194, The plan and general agent configs still inherit the permissive defaults (\"*\": \"allow\") so over-budget SQL bypasses the new cost confirmation; update the plan and general role definitions (the objects named plan and general where PermissionNext.merge(...) and PermissionNext.fromConfig(...) are used) to explicitly set sql_execute_cost: \"ask\" (or change the merged defaults to include sql_execute_cost: \"ask\") so that both plan and general enforce the same cost firewall as builder and analyst; ensure you add the sql_execute_cost: \"ask\" entry alongside any existing sql_execute/sql_execute_write settings in those PermissionNext.fromConfig blocks.
🤖 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.
Inline comments:
In `@CHANGELOG.md`:
- Line 12: Confirm the correct PR number (907 vs 906) and update the changelog
entry that currently ends with "(`#906`)" to match the actual PR number used in
the PR header (e.g., replace with "(`#907`)" if PR `#907` is correct); ensure the PR
reference in the CHANGELOG.md matches the PR header and any other occurrences of
the PR identifier in the changelog text.
In `@packages/opencode/src/altimate/native/connections/register.ts`:
- Around line 474-477: Validate numeric inputs before computing estimatedCost:
ensure costPerTib (from params.cost_per_tib_usd or DEFAULT_COST_PER_TIB_USD) and
estimate.bytesScanned are finite numbers and non-negative using
Number.isFinite(...) and >= 0 checks; if costPerTib is invalid fall back to
DEFAULT_COST_PER_TIB_USD (or treat as undefined) and only compute estimatedCost
when both costPerTib and estimate.bytesScanned are valid, otherwise set
estimatedCost to undefined so downstream firewall comparisons don't receive
NaN/Infinity/negative values (update the logic around costPerTib,
estimate.bytesScanned, and estimatedCost; keep references to costPerTib,
estimate.bytesScanned, estimatedCost, DEFAULT_COST_PER_TIB_USD and TIB_BYTES).
In `@packages/opencode/src/altimate/tools/sql-cost-estimate.ts`:
- Around line 34-35: The current catch on Config.get() swallows all errors and
lets cfg become {} so costPerTib is silently undefined; change the logic around
Config.get() (the call to Config.get(), the cfg variable and where costPerTib is
read) to capture any thrown error into a local variable (e.g. configLoadError)
instead of discarding it, use the real cfg if present, and return/include
configLoadError in the function's returned metadata/output so callers can detect
that pricing used a fallback; ensure you still compute the estimate using
dispatcher defaults only when cfg is missing but surface configLoadError
alongside the estimate.
In `@packages/opencode/src/altimate/tools/sql-execute.ts`:
- Around line 133-143: The current catch block around
Dispatcher.call("sql.estimate_cost") returns silently, which lets queries bypass
configured governance budgets on transient estimator failures; change the catch
to not simply return—if governance (or governance?.cost_per_tib_usd / budget) is
configured, treat an estimation failure as a conservative failure: log the error
and throw or reject to block the query (enforce the cost firewall), otherwise
(no governance configured) allow the query to proceed; update the catch to
reference Dispatcher.call("sql.estimate_cost"), the local variable estimate, and
governance to decide whether to fail-fast or continue.
---
Outside diff comments:
In `@packages/opencode/src/agent/agent.ts`:
- Around line 155-194: The plan and general agent configs still inherit the
permissive defaults (\"*\": \"allow\") so over-budget SQL bypasses the new cost
confirmation; update the plan and general role definitions (the objects named
plan and general where PermissionNext.merge(...) and
PermissionNext.fromConfig(...) are used) to explicitly set sql_execute_cost:
\"ask\" (or change the merged defaults to include sql_execute_cost: \"ask\") so
that both plan and general enforce the same cost firewall as builder and
analyst; ensure you add the sql_execute_cost: \"ask\" entry alongside any
existing sql_execute/sql_execute_write settings in those
PermissionNext.fromConfig blocks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 525e804b-cccd-4f5c-b198-a670475606b4
📒 Files selected for processing (14)
CHANGELOG.mddocs/docs/configure/governance.mdpackages/drivers/src/bigquery.tspackages/drivers/src/index.tspackages/drivers/src/types.tspackages/opencode/src/agent/agent.tspackages/opencode/src/altimate/index.tspackages/opencode/src/altimate/native/connections/register.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/sql-cost-estimate.tspackages/opencode/src/altimate/tools/sql-execute.tspackages/opencode/src/config/config.tspackages/opencode/src/tool/registry.tspackages/opencode/test/altimate/sql-cost-estimate-formatters.test.ts
|
|
||
| ### Added | ||
|
|
||
| - **Cost firewall — estimate a query's scan cost before it runs and confirm before going over budget.** A new opt-in `governance` config (`max_query_cost_usd`, `max_bytes_scanned`, `cost_per_tib_usd`) makes `sql_execute` consult a pre-execution estimate and prompt via the `sql_execute_cost` permission when a query would exceed the configured budget, with a hint to run `sql_optimize` first. Estimation uses a new optional `Connector.estimateCost()` capability — implemented for BigQuery via a dry-run (exact bytes processed, no execution and no charge) — surfaced through the `sql.estimate_cost` dispatcher method and a standalone `sql_cost_estimate` tool. Disabled by default; warehouses without estimation support skip the guard, so it never blocks work it can't price. (#906) |
There was a problem hiding this comment.
Verify the PR number reference.
The changelog entry references (#906), but the PR objectives header states this is PR #907. Please confirm which PR number is correct and update the changelog entry if needed — accurate PR references are critical for traceability.
🤖 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 `@CHANGELOG.md` at line 12, Confirm the correct PR number (907 vs 906) and
update the changelog entry that currently ends with "(`#906`)" to match the actual
PR number used in the PR header (e.g., replace with "(`#907`)" if PR `#907` is
correct); ensure the PR reference in the CHANGELOG.md matches the PR header and
any other occurrences of the PR identifier in the changelog text.
| const costPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD | ||
| const estimatedCost = | ||
| estimate.bytesScanned != null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined | ||
|
|
There was a problem hiding this comment.
Validate estimator numeric inputs before computing cost
At Line 474-477, invalid numeric values (NaN/Infinity/negative) from either cost_per_tib_usd or estimate.bytesScanned can produce invalid estimated_cost_usd, which causes downstream firewall comparisons to fail open unintentionally.
Suggested fix
- const costPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD
- const estimatedCost =
- estimate.bytesScanned != null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined
+ const rawCostPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD
+ const costPerTib =
+ Number.isFinite(rawCostPerTib) && rawCostPerTib > 0 ? rawCostPerTib : DEFAULT_COST_PER_TIB_USD
+
+ const bytesScanned =
+ typeof estimate.bytesScanned === "number" &&
+ Number.isFinite(estimate.bytesScanned) &&
+ estimate.bytesScanned >= 0
+ ? estimate.bytesScanned
+ : undefined
+
+ const estimatedCost =
+ bytesScanned != null ? (bytesScanned / TIB_BYTES) * costPerTib : undefined
@@
- bytes_scanned: estimate.bytesScanned,
+ bytes_scanned: bytesScanned,🤖 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/connections/register.ts` around lines
474 - 477, Validate numeric inputs before computing estimatedCost: ensure
costPerTib (from params.cost_per_tib_usd or DEFAULT_COST_PER_TIB_USD) and
estimate.bytesScanned are finite numbers and non-negative using
Number.isFinite(...) and >= 0 checks; if costPerTib is invalid fall back to
DEFAULT_COST_PER_TIB_USD (or treat as undefined) and only compute estimatedCost
when both costPerTib and estimate.bytesScanned are valid, otherwise set
estimatedCost to undefined so downstream firewall comparisons don't receive
NaN/Infinity/negative values (update the logic around costPerTib,
estimate.bytesScanned, and estimatedCost; keep references to costPerTib,
estimate.bytesScanned, estimatedCost, DEFAULT_COST_PER_TIB_USD and TIB_BYTES).
| const cfg = await Config.get().catch(() => ({}) as Awaited<ReturnType<typeof Config.get>>) | ||
| const costPerTib = cfg.governance?.cost_per_tib_usd |
There was a problem hiding this comment.
Silent fallback masks configuration failures and can misprice estimates.
Catching all Config.get() errors and defaulting to {} hides malformed governance config, then cost estimation silently uses dispatcher defaults. That can return a plausible but incorrect USD estimate with no signal to the caller.
Suggested adjustment
- const cfg = await Config.get().catch(() => ({}) as Awaited<ReturnType<typeof Config.get>>)
- const costPerTib = cfg.governance?.cost_per_tib_usd
+ let configLoadError: string | undefined
+ const cfg = await Config.get().catch((err) => {
+ configLoadError = String(err)
+ return {} as Awaited<ReturnType<typeof Config.get>>
+ })
+ const costPerTib = cfg.governance?.cost_per_tib_usdThen include configLoadError in returned metadata/output so callers can see pricing may be fallback-based.
🤖 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/tools/sql-cost-estimate.ts` around lines 34 -
35, The current catch on Config.get() swallows all errors and lets cfg become {}
so costPerTib is silently undefined; change the logic around Config.get() (the
call to Config.get(), the cfg variable and where costPerTib is read) to capture
any thrown error into a local variable (e.g. configLoadError) instead of
discarding it, use the real cfg if present, and return/include configLoadError
in the function's returned metadata/output so callers can detect that pricing
used a fallback; ensure you still compute the estimate using dispatcher defaults
only when cfg is missing but surface configLoadError alongside the estimate.
| let estimate | ||
| try { | ||
| estimate = await Dispatcher.call("sql.estimate_cost", { | ||
| sql: query, | ||
| warehouse, | ||
| cost_per_tib_usd: governance?.cost_per_tib_usd, | ||
| }) | ||
| } catch { | ||
| // Estimation failed — fail open (run the query) rather than blocking work. | ||
| return | ||
| } |
There was a problem hiding this comment.
Don't silently bypass configured budgets when estimation errors.
Any exception from sql.estimate_cost currently returns early and the query executes normally. That means a transient dispatcher/connector failure disables the cost firewall entirely for the exact case where the user configured a budget.
Suggested fix
let estimate
try {
estimate = await Dispatcher.call("sql.estimate_cost", {
sql: query,
warehouse,
cost_per_tib_usd: governance?.cost_per_tib_usd,
})
- } catch {
- // Estimation failed — fail open (run the query) rather than blocking work.
+ } catch {
+ await ctx.ask({
+ permission: "sql_execute_cost",
+ patterns: [query.slice(0, 200)],
+ always: ["*"],
+ metadata: {
+ reason: "Cost estimate unavailable; configured budget could not be checked.",
+ hint: "Retry once estimation is healthy, or approve explicitly to continue.",
+ },
+ })
return
}🤖 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/tools/sql-execute.ts` around lines 133 - 143,
The current catch block around Dispatcher.call("sql.estimate_cost") returns
silently, which lets queries bypass configured governance budgets on transient
estimator failures; change the catch to not simply return—if governance (or
governance?.cost_per_tib_usd / budget) is configured, treat an estimation
failure as a conservative failure: log the error and throw or reject to block
the query (enforce the cost firewall), otherwise (no governance configured)
allow the query to proceed; update the catch to reference
Dispatcher.call("sql.estimate_cost"), the local variable estimate, and
governance to decide whether to fail-fast or continue.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 14 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/connections/register.ts">
<violation number="1" location="packages/opencode/src/altimate/native/connections/register.ts:474">
P1: Missing validation of `cost_per_tib_usd` and `estimated_cost_usd` allows NaN/negative costs to silently bypass the cost firewall guard.</violation>
</file>
<file name="packages/opencode/src/altimate/tools/sql-cost-estimate.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/sql-cost-estimate.ts:55">
P2: Fallbacking missing `cost_per_tib_usd` to 0 can misleadingly show `$0.00/TiB` (free scanning) when the rate is actually unknown. A per-unit price of $0 has a specific semantic meaning; it should not be used as a default for optional/missing values in user-facing cost output.</violation>
</file>
<file name="packages/opencode/src/altimate/tools/sql-execute.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/sql-execute.ts:141">
P1: The blanket `catch { return }` means any transient error (network timeout, connector crash, etc.) silently disables the cost firewall and lets the query run unguarded. This is distinct from the `!estimate.supported` path which correctly handles warehouses that can't estimate. When the user has explicitly configured a budget, a transient failure should at minimum log a warning or prompt rather than silently bypassing the guard.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| const estimate = await connector.estimateCost(params.sql) | ||
| const costPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD |
There was a problem hiding this comment.
P1: Missing validation of cost_per_tib_usd and estimated_cost_usd allows NaN/negative costs to silently bypass the cost firewall guard.
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/connections/register.ts, line 474:
<comment>Missing validation of `cost_per_tib_usd` and `estimated_cost_usd` allows NaN/negative costs to silently bypass the cost firewall guard.</comment>
<file context>
@@ -438,6 +440,54 @@ register("sql.execute", async (params: SqlExecuteParams): Promise<SqlExecuteResu
+ }
+
+ const estimate = await connector.estimateCost(params.sql)
+ const costPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD
+ const estimatedCost =
+ estimate.bytesScanned != null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined
</file context>
| cost_per_tib_usd: governance?.cost_per_tib_usd, | ||
| }) | ||
| } catch { | ||
| // Estimation failed — fail open (run the query) rather than blocking work. |
There was a problem hiding this comment.
P1: The blanket catch { return } means any transient error (network timeout, connector crash, etc.) silently disables the cost firewall and lets the query run unguarded. This is distinct from the !estimate.supported path which correctly handles warehouses that can't estimate. When the user has explicitly configured a budget, a transient failure should at minimum log a warning or prompt rather than silently bypassing the guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/tools/sql-execute.ts, line 141:
<comment>The blanket `catch { return }` means any transient error (network timeout, connector crash, etc.) silently disables the cost firewall and lets the query run unguarded. This is distinct from the `!estimate.supported` path which correctly handles warehouses that can't estimate. When the user has explicitly configured a budget, a transient failure should at minimum log a warning or prompt rather than silently bypassing the guard.</comment>
<file context>
@@ -103,6 +114,58 @@ export const SqlExecuteTool = Tool.define("sql_execute", {
+ cost_per_tib_usd: governance?.cost_per_tib_usd,
+ })
+ } catch {
+ // Estimation failed — fail open (run the query) rather than blocking work.
+ return
+ }
</file context>
| const lines: string[] = [] | ||
| if (result.bytes_scanned != null) lines.push(`Bytes scanned (est.): ${formatBytes(result.bytes_scanned)}`) | ||
| if (result.estimated_cost_usd != null) { | ||
| lines.push(`Estimated cost: ${formatCost(result.estimated_cost_usd)} (at ${formatCost(result.cost_per_tib_usd ?? 0)}/TiB)`) |
There was a problem hiding this comment.
P2: Fallbacking missing cost_per_tib_usd to 0 can misleadingly show $0.00/TiB (free scanning) when the rate is actually unknown. A per-unit price of $0 has a specific semantic meaning; it should not be used as a default for optional/missing values in user-facing cost output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/tools/sql-cost-estimate.ts, line 55:
<comment>Fallbacking missing `cost_per_tib_usd` to 0 can misleadingly show `$0.00/TiB` (free scanning) when the rate is actually unknown. A per-unit price of $0 has a specific semantic meaning; it should not be used as a default for optional/missing values in user-facing cost output.</comment>
<file context>
@@ -0,0 +1,70 @@
+ const lines: string[] = []
+ if (result.bytes_scanned != null) lines.push(`Bytes scanned (est.): ${formatBytes(result.bytes_scanned)}`)
+ if (result.estimated_cost_usd != null) {
+ lines.push(`Estimated cost: ${formatCost(result.estimated_cost_usd)} (at ${formatCost(result.cost_per_tib_usd ?? 0)}/TiB)`)
+ }
+ if (result.note) lines.push(`Method: ${result.note}`)
</file context>
Surfaces the configured query cost/scan budget in the Status dialog so the cost firewall is discoverable without reading the config file. Shows 'Cost Firewall: off' when no governance threshold is set, otherwise the max cost and/or max scan per query.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/opencode/src/cli/cmd/tui/component/dialog-status.tsx`:
- Around line 15-16: The unit index calculation can produce -1 for fractional
values (bytes between 0 and 1); clamp it to zero before indexing to avoid
undefined units: replace the current index computation using Math.min/Math.floor
with a clamped value (for example compute iRaw = Math.floor(Math.log(bytes) /
Math.log(1024)) and then i = Math.max(0, Math.min(iRaw, units.length - 1))), and
ensure the existing return uses this clamped i (references: i, bytes, units in
dialog-status.tsx).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cd412355-8a10-4a9f-b804-9f113545ea18
📒 Files selected for processing (1)
packages/opencode/src/cli/cmd/tui/component/dialog-status.tsx
| const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1) | ||
| return `${(bytes / Math.pow(1024, i)).toFixed(i === 0 ? 0 : 2)} ${units[i]}` |
There was a problem hiding this comment.
Clamp unit index to avoid invalid output for fractional byte limits.
If max_bytes_scanned is configured as a non-integer between 0 and 1 (allowed by schema), i becomes -1, so this renders an invalid unit (undefined).
Suggested fix
- const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1)
+ const i = Math.max(0, Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1) | |
| return `${(bytes / Math.pow(1024, i)).toFixed(i === 0 ? 0 : 2)} ${units[i]}` | |
| const i = Math.max(0, Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1)) | |
| return `${(bytes / Math.pow(1024, i)).toFixed(i === 0 ? 0 : 2)} ${units[i]}` |
🤖 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/cli/cmd/tui/component/dialog-status.tsx` around lines
15 - 16, The unit index calculation can produce -1 for fractional values (bytes
between 0 and 1); clamp it to zero before indexing to avoid undefined units:
replace the current index computation using Math.min/Math.floor with a clamped
value (for example compute iRaw = Math.floor(Math.log(bytes) / Math.log(1024))
and then i = Math.max(0, Math.min(iRaw, units.length - 1))), and ensure the
existing return uses this clamped i (references: i, bytes, units in
dialog-status.tsx).
Implements Connector.estimateCost() for Snowflake using EXPLAIN USING JSON, which compiles the query and returns the planner's estimated bytes to scan (GlobalStats.bytesAssigned) without executing it or resuming a warehouse. Snowflake bills by warehouse credits rather than bytes scanned, so the derived USD is a proxy and max_bytes_scanned is the meaningful guard — noted in the estimate output and the governance docs. Updates docs + CHANGELOG.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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/cli/cmd/tui/component/dialog-status.tsx">
<violation number="1" location="packages/opencode/src/cli/cmd/tui/component/dialog-status.tsx:15">
P2: Clamp unit index to `Math.max(0, ...)` to avoid `units[-1]` (which is `undefined`) when `bytes` is between 0 and 1. `Math.log` of a fractional value is negative, producing a negative index after `Math.floor`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| if (!Number.isFinite(bytes) || bytes < 0) return "unknown" | ||
| if (bytes === 0) return "0 B" | ||
| const units = ["B", "KB", "MB", "GB", "TB", "PB"] | ||
| const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1) |
There was a problem hiding this comment.
P2: Clamp unit index to Math.max(0, ...) to avoid units[-1] (which is undefined) when bytes is between 0 and 1. Math.log of a fractional value is negative, producing a negative index after Math.floor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/cmd/tui/component/dialog-status.tsx, line 15:
<comment>Clamp unit index to `Math.max(0, ...)` to avoid `units[-1]` (which is `undefined`) when `bytes` is between 0 and 1. `Math.log` of a fractional value is negative, producing a negative index after `Math.floor`.</comment>
<file context>
@@ -7,13 +7,40 @@ import { For, Match, Switch, Show, createMemo } from "solid-js"
+ if (!Number.isFinite(bytes) || bytes < 0) return "unknown"
+ if (bytes === 0) return "0 B"
+ const units = ["B", "KB", "MB", "GB", "TB", "PB"]
+ const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1)
+ return `${(bytes / Math.pow(1024, i)).toFixed(i === 0 ? 0 : 2)} ${units[i]}`
+}
</file context>
| const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1) | |
| const i = Math.min(Math.max(0, Math.floor(Math.log(bytes) / Math.log(1024))), units.length - 1) |
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. |
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 12 finding(s)
- 12 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/drivers/src/bigquery.ts (L91-L92)
[🔵 LOW] According to the code quality guidelines, using == and != is prohibited. Please use strict equality checks (!== null && !== undefined) instead.
Suggested change:
const raw = stats.totalBytesProcessed ?? stats.query?.totalBytesProcessed
const bytesScanned = raw !== null && raw !== undefined ? Number(raw) : undefined
2. packages/drivers/src/snowflake.ts (L273)
[🔵 LOW] According to the review checklist, using == is prohibited. Please use strict equality === or check for both null and undefined.
Suggested change:
if (raw === null || raw === undefined) {
3. packages/drivers/src/snowflake.ts (L286)
[🔵 LOW] According to the review checklist, using != is prohibited. Please use strict equality !== or check for both null and undefined.
Suggested change:
const bytesScanned = assigned !== null && assigned !== undefined ? Number(assigned) : undefined
4. packages/opencode/src/altimate/native/connections/register.ts (L473-L476)
[🟠 MEDIUM] The generic cost estimation logic uses a BigQuery-specific default value (DEFAULT_COST_PER_TIB_USD = 6.25). If a caller runs this for a different warehouse type (e.g., Snowflake) without explicitly providing cost_per_tib_usd, it will silently use BigQuery's pricing. This could lead to misleading estimations since other warehouses use entirely different pricing models (e.g., Snowflake bills by compute credits, not data scanned). Consider making the default cost driver-specific or applying this default only when warehouseType === 'bigquery'.
Suggested change:
const estimate = await connector.estimateCost(params.sql)
// Only apply BigQuery's default pricing if the warehouse type is BigQuery.
// Other warehouses should explicitly provide a cost factor or use driver-specific estimations.
const defaultCostPerTib = warehouseType === "bigquery" ? DEFAULT_COST_PER_TIB_USD : undefined;
const costPerTib = params.cost_per_tib_usd ?? defaultCostPerTib
const estimatedCost =
estimate.bytesScanned != null && costPerTib != null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined
5. packages/opencode/src/altimate/tools/sql-execute.ts (L130-L131)
[🔵 LOW] According to the code quality standards, the use of == and != is prohibited. Please use strict equality (=== and !==) for null/undefined checks to ensure type safety. Since maxCost and maxBytes are derived from Zod .optional() schema definitions, checking against undefined is appropriate here.
Suggested change:
// Disabled unless at least one threshold is set.
if (maxCost === undefined && maxBytes === undefined) return
6. packages/opencode/src/altimate/tools/sql-execute.ts (L140-L143)
[🔵 LOW] Although failing open is intentional here to prevent blocking user work when estimation fails, completely swallowing the error with an empty catch block makes it difficult to diagnose system issues. It is recommended to log the error (e.g., using console.warn or the project's standard logger) for debugging purposes.
Suggested change:
} catch (error) {
// Estimation failed — fail open (run the query) rather than blocking work.
console.warn("Cost estimation failed:", error)
return
}
7. packages/opencode/src/altimate/tools/sql-execute.ts (L146-L147)
[🔵 LOW] Please replace the non-strict != checks with strict equality (!==) to comply with the code quality guidelines that prohibit the use of == and !=.
Suggested change:
const overCost = maxCost !== undefined && estimate.estimated_cost_usd !== undefined && estimate.estimated_cost_usd > maxCost
const overBytes = maxBytes !== undefined && estimate.bytes_scanned !== undefined && estimate.bytes_scanned > maxBytes
8. packages/opencode/src/altimate/tools/sql-cost-estimate.ts (L54-L56)
[🟠 MEDIUM] If cost_per_tib_usd is undefined or missing, this evaluates to 0, which causes the UI output to display at $0.0000/TiB. This could be misleading to users as it implies the scan is free rather than the rate being unknown.
Since formatCost checks !Number.isFinite(usd), falling back to NaN instead of 0 will properly output at unknown/TiB. Alternatively, you could update formatCost to accept number | undefined.
Suggested change:
if (result.estimated_cost_usd != null) {
lines.push(`Estimated cost: ${formatCost(result.estimated_cost_usd)} (at ${formatCost(result.cost_per_tib_usd ?? NaN)}/TiB)`)
}
9. packages/opencode/src/altimate/native/connections/register.ts (L474-L476)
[🔵 LOW] Using != is prohibited according to code quality checks. Please use strict equality !== null && estimate.bytesScanned !== undefined or simply !== undefined depending on the exact types expected.
Suggested change:
const costPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD
const estimatedCost =
estimate.bytesScanned !== undefined && estimate.bytesScanned !== null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined
10. packages/opencode/src/cli/cmd/tui/component/dialog-status.tsx (L14-L16)
[🟠 MEDIUM] For byte values strictly between 0 and 1 (e.g., 0.5), Math.log(bytes) / Math.log(1024) evaluates to a negative number, causing Math.floor to return -1. This results in an out-of-bounds array access on units (units[-1]), returning undefined (e.g., 512.00 undefined).
Also, consider centralizing this function in a shared utilities file to avoid duplication (e.g., it is duplicated in finops-formatting.ts and sql-cost-estimate.ts). If keeping it here, enforce a minimum index of 0 to fix the edge case:
Suggested change:
const units = ["B", "KB", "MB", "GB", "TB", "PB"]
const i = Math.max(0, Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1))
return `${(bytes / Math.pow(1024, i)).toFixed(i === 0 ? 0 : 2)} ${units[i]}`
11. packages/opencode/src/cli/cmd/tui/component/dialog-status.tsx (L38-L41)
[🔵 LOW] The code quality checklist prohibits using loose inequality checks (!=). Use strict equality/inequality checks (!== undefined && ... !== null) instead.
Suggested change:
const firewallEnabled = createMemo(() => {
const g = governance()
return !!g && ((g.max_query_cost_usd !== null && g.max_query_cost_usd !== undefined) || (g.max_bytes_scanned !== null && g.max_bytes_scanned !== undefined))
})
12. packages/opencode/src/cli/cmd/tui/component/dialog-status.tsx (L199-L220)
[🔴 HIGH] In SolidJS, if the underlying reactive data changes and governance() returns undefined before the UI fully re-renders and unmounts, these non-null assertions (!) will cause a runtime crash because governance() is re-invoked within the children. It is safer to use optional chaining (?.) and nullish coalescing (??).
Additionally, replace the loose inequality checks (!= null) with strict checks (!== undefined && !== null) to adhere to the code quality rules.
Suggested change:
<Show when={governance()?.max_query_cost_usd !== undefined && governance()?.max_query_cost_usd !== null}>
<box flexDirection="row" gap={1}>
<text flexShrink={0} style={{ fg: theme.success }}>
•
</text>
<text fg={theme.text}>
<b>Max cost</b>{" "}
<span style={{ fg: theme.textMuted }}>${governance()?.max_query_cost_usd?.toFixed(2)}/query</span>
</text>
</box>
</Show>
<Show when={governance()?.max_bytes_scanned !== undefined && governance()?.max_bytes_scanned !== null}>
<box flexDirection="row" gap={1}>
<text flexShrink={0} style={{ fg: theme.success }}>
•
</text>
<text fg={theme.text}>
<b>Max scan</b>{" "}
<span style={{ fg: theme.textMuted }}>{formatBytes(governance()?.max_bytes_scanned ?? 0)}/query</span>
</text>
</box>
</Show>
| const raw = stats.totalBytesProcessed ?? stats.query?.totalBytesProcessed | ||
| const bytesScanned = raw != null ? Number(raw) : undefined |
There was a problem hiding this comment.
[🔵 LOW] According to the code quality guidelines, using == and != is prohibited. Please use strict equality checks (!== null && !== undefined) instead.
Suggested change:
| const raw = stats.totalBytesProcessed ?? stats.query?.totalBytesProcessed | |
| const bytesScanned = raw != null ? Number(raw) : undefined | |
| const raw = stats.totalBytesProcessed ?? stats.query?.totalBytesProcessed | |
| const bytesScanned = raw !== null && raw !== undefined ? Number(raw) : undefined |
| const query = sql.replace(/;\s*$/, "") | ||
| const explain = await executeQuery(`EXPLAIN USING JSON ${query}`) | ||
| const raw = explain.rows?.[0]?.[0] | ||
| if (raw == null) { |
There was a problem hiding this comment.
[🔵 LOW] According to the review checklist, using == is prohibited. Please use strict equality === or check for both null and undefined.
Suggested change:
| if (raw == null) { | |
| if (raw === null || raw === undefined) { |
| } | ||
| const globalStats = plan?.GlobalStats ?? {} | ||
| const assigned = globalStats.bytesAssigned | ||
| const bytesScanned = assigned != null ? Number(assigned) : undefined |
There was a problem hiding this comment.
[🔵 LOW] According to the review checklist, using != is prohibited. Please use strict equality !== or check for both null and undefined.
Suggested change:
| const bytesScanned = assigned != null ? Number(assigned) : undefined | |
| const bytesScanned = assigned !== null && assigned !== undefined ? Number(assigned) : undefined |
| const estimate = await connector.estimateCost(params.sql) | ||
| const costPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD | ||
| const estimatedCost = | ||
| estimate.bytesScanned != null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined |
There was a problem hiding this comment.
[🟠 MEDIUM] The generic cost estimation logic uses a BigQuery-specific default value (DEFAULT_COST_PER_TIB_USD = 6.25). If a caller runs this for a different warehouse type (e.g., Snowflake) without explicitly providing cost_per_tib_usd, it will silently use BigQuery's pricing. This could lead to misleading estimations since other warehouses use entirely different pricing models (e.g., Snowflake bills by compute credits, not data scanned). Consider making the default cost driver-specific or applying this default only when warehouseType === 'bigquery'.
Suggested change:
| const estimate = await connector.estimateCost(params.sql) | |
| const costPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD | |
| const estimatedCost = | |
| estimate.bytesScanned != null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined | |
| const estimate = await connector.estimateCost(params.sql) | |
| // Only apply BigQuery's default pricing if the warehouse type is BigQuery. | |
| // Other warehouses should explicitly provide a cost factor or use driver-specific estimations. | |
| const defaultCostPerTib = warehouseType === "bigquery" ? DEFAULT_COST_PER_TIB_USD : undefined; | |
| const costPerTib = params.cost_per_tib_usd ?? defaultCostPerTib | |
| const estimatedCost = | |
| estimate.bytesScanned != null && costPerTib != null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined |
| // Disabled unless at least one threshold is set. | ||
| if (maxCost == null && maxBytes == null) return |
There was a problem hiding this comment.
[🔵 LOW] According to the code quality standards, the use of == and != is prohibited. Please use strict equality (=== and !==) for null/undefined checks to ensure type safety. Since maxCost and maxBytes are derived from Zod .optional() schema definitions, checking against undefined is appropriate here.
Suggested change:
| // Disabled unless at least one threshold is set. | |
| if (maxCost == null && maxBytes == null) return | |
| // Disabled unless at least one threshold is set. | |
| if (maxCost === undefined && maxBytes === undefined) return |
| if (result.estimated_cost_usd != null) { | ||
| lines.push(`Estimated cost: ${formatCost(result.estimated_cost_usd)} (at ${formatCost(result.cost_per_tib_usd ?? 0)}/TiB)`) | ||
| } |
There was a problem hiding this comment.
[🟠 MEDIUM] If cost_per_tib_usd is undefined or missing, this evaluates to 0, which causes the UI output to display at $0.0000/TiB. This could be misleading to users as it implies the scan is free rather than the rate being unknown.
Since formatCost checks !Number.isFinite(usd), falling back to NaN instead of 0 will properly output at unknown/TiB. Alternatively, you could update formatCost to accept number | undefined.
Suggested change:
| if (result.estimated_cost_usd != null) { | |
| lines.push(`Estimated cost: ${formatCost(result.estimated_cost_usd)} (at ${formatCost(result.cost_per_tib_usd ?? 0)}/TiB)`) | |
| } | |
| if (result.estimated_cost_usd != null) { | |
| lines.push(`Estimated cost: ${formatCost(result.estimated_cost_usd)} (at ${formatCost(result.cost_per_tib_usd ?? NaN)}/TiB)`) | |
| } |
| const costPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD | ||
| const estimatedCost = | ||
| estimate.bytesScanned != null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined |
There was a problem hiding this comment.
[🔵 LOW] Using != is prohibited according to code quality checks. Please use strict equality !== null && estimate.bytesScanned !== undefined or simply !== undefined depending on the exact types expected.
Suggested change:
| const costPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD | |
| const estimatedCost = | |
| estimate.bytesScanned != null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined | |
| const costPerTib = params.cost_per_tib_usd ?? DEFAULT_COST_PER_TIB_USD | |
| const estimatedCost = | |
| estimate.bytesScanned !== undefined && estimate.bytesScanned !== null ? (estimate.bytesScanned / TIB_BYTES) * costPerTib : undefined |
| const units = ["B", "KB", "MB", "GB", "TB", "PB"] | ||
| const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1) | ||
| return `${(bytes / Math.pow(1024, i)).toFixed(i === 0 ? 0 : 2)} ${units[i]}` |
There was a problem hiding this comment.
[🟠 MEDIUM] For byte values strictly between 0 and 1 (e.g., 0.5), Math.log(bytes) / Math.log(1024) evaluates to a negative number, causing Math.floor to return -1. This results in an out-of-bounds array access on units (units[-1]), returning undefined (e.g., 512.00 undefined).
Also, consider centralizing this function in a shared utilities file to avoid duplication (e.g., it is duplicated in finops-formatting.ts and sql-cost-estimate.ts). If keeping it here, enforce a minimum index of 0 to fix the edge case:
Suggested change:
| const units = ["B", "KB", "MB", "GB", "TB", "PB"] | |
| const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1) | |
| return `${(bytes / Math.pow(1024, i)).toFixed(i === 0 ? 0 : 2)} ${units[i]}` | |
| const units = ["B", "KB", "MB", "GB", "TB", "PB"] | |
| const i = Math.max(0, Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1)) | |
| return `${(bytes / Math.pow(1024, i)).toFixed(i === 0 ? 0 : 2)} ${units[i]}` |
| const firewallEnabled = createMemo(() => { | ||
| const g = governance() | ||
| return !!g && (g.max_query_cost_usd != null || g.max_bytes_scanned != null) | ||
| }) |
There was a problem hiding this comment.
[🔵 LOW] The code quality checklist prohibits using loose inequality checks (!=). Use strict equality/inequality checks (!== undefined && ... !== null) instead.
Suggested change:
| const firewallEnabled = createMemo(() => { | |
| const g = governance() | |
| return !!g && (g.max_query_cost_usd != null || g.max_bytes_scanned != null) | |
| }) | |
| const firewallEnabled = createMemo(() => { | |
| const g = governance() | |
| return !!g && ((g.max_query_cost_usd !== null && g.max_query_cost_usd !== undefined) || (g.max_bytes_scanned !== null && g.max_bytes_scanned !== undefined)) | |
| }) |
| <Show when={governance()?.max_query_cost_usd != null}> | ||
| <box flexDirection="row" gap={1}> | ||
| <text flexShrink={0} style={{ fg: theme.success }}> | ||
| • | ||
| </text> | ||
| <text fg={theme.text}> | ||
| <b>Max cost</b>{" "} | ||
| <span style={{ fg: theme.textMuted }}>${governance()!.max_query_cost_usd!.toFixed(2)}/query</span> | ||
| </text> | ||
| </box> | ||
| </Show> | ||
| <Show when={governance()?.max_bytes_scanned != null}> | ||
| <box flexDirection="row" gap={1}> | ||
| <text flexShrink={0} style={{ fg: theme.success }}> | ||
| • | ||
| </text> | ||
| <text fg={theme.text}> | ||
| <b>Max scan</b>{" "} | ||
| <span style={{ fg: theme.textMuted }}>{formatBytes(governance()!.max_bytes_scanned!)}/query</span> | ||
| </text> | ||
| </box> | ||
| </Show> |
There was a problem hiding this comment.
[🔴 HIGH] In SolidJS, if the underlying reactive data changes and governance() returns undefined before the UI fully re-renders and unmounts, these non-null assertions (!) will cause a runtime crash because governance() is re-invoked within the children. It is safer to use optional chaining (?.) and nullish coalescing (??).
Additionally, replace the loose inequality checks (!= null) with strict checks (!== undefined && !== null) to adhere to the code quality rules.
Suggested change:
| <Show when={governance()?.max_query_cost_usd != null}> | |
| <box flexDirection="row" gap={1}> | |
| <text flexShrink={0} style={{ fg: theme.success }}> | |
| • | |
| </text> | |
| <text fg={theme.text}> | |
| <b>Max cost</b>{" "} | |
| <span style={{ fg: theme.textMuted }}>${governance()!.max_query_cost_usd!.toFixed(2)}/query</span> | |
| </text> | |
| </box> | |
| </Show> | |
| <Show when={governance()?.max_bytes_scanned != null}> | |
| <box flexDirection="row" gap={1}> | |
| <text flexShrink={0} style={{ fg: theme.success }}> | |
| • | |
| </text> | |
| <text fg={theme.text}> | |
| <b>Max scan</b>{" "} | |
| <span style={{ fg: theme.textMuted }}>{formatBytes(governance()!.max_bytes_scanned!)}/query</span> | |
| </text> | |
| </box> | |
| </Show> | |
| <Show when={governance()?.max_query_cost_usd !== undefined && governance()?.max_query_cost_usd !== null}> | |
| <box flexDirection="row" gap={1}> | |
| <text flexShrink={0} style={{ fg: theme.success }}> | |
| • | |
| </text> | |
| <text fg={theme.text}> | |
| <b>Max cost</b>{" "} | |
| <span style={{ fg: theme.textMuted }}>${governance()?.max_query_cost_usd?.toFixed(2)}/query</span> | |
| </text> | |
| </box> | |
| </Show> | |
| <Show when={governance()?.max_bytes_scanned !== undefined && governance()?.max_bytes_scanned !== null}> | |
| <box flexDirection="row" gap={1}> | |
| <text flexShrink={0} style={{ fg: theme.success }}> | |
| • | |
| </text> | |
| <text fg={theme.text}> | |
| <b>Max scan</b>{" "} | |
| <span style={{ fg: theme.textMuted }}>{formatBytes(governance()?.max_bytes_scanned ?? 0)}/query</span> | |
| </text> | |
| </box> | |
| </Show> |
🤖 Code Review — OpenCodeReview (Gemini) — 12 finding(s)
All findings (full text)1.
|
🔴 1 critical finding(s) — converted back to draftAddress the critical items below, then mark this PR Ready for review to re-run the review. Medium/low findings are posted inline above and do not block.
|
2 participants
Summary
Adds an opt-in guardrail that estimates a query's scan cost BEFORE it runs and asks for confirmation when it exceeds a configured budget. The agent can fire a
SELECTthat scans terabytes before anyone notices and this catches it pre-flight.Connector.estimateCost(); implemented for BigQuery via a dry-run (createQueryJob({dryRun: true})) → exact bytes processed, no execution and no charge.sql.estimate_costdispatcher handler converts bytes → USD using a configurable per-TiB rate (default $6.25/TiB).sql_cost_estimatetool for on-demandwhat will this cost?queries.governanceconfig:max_query_cost_usd,max_bytes_scanned,cost_per_tib_usd.sql_executeconsults the estimate before running; over-budget queries prompt via a newsql_execute_costpermission (registered"ask"for the builder and analyst agents). Fails open when estimation is unsupported.Disabled by default no behavior change unless a budget is set. Warehouses without cost estimation simply skip the guard. BigQuery is the only estimator in v1; others (e.g. Snowflake via
EXPLAIN) can follow.Adjacent but distinct from #731 (data_diff row ceiling) and #853 (model router) neither estimates warehouse query scan cost pre-execution.
Test Plan
Connectormethod, the config schema, and permission resolution end-to-end."*": "allow"vs"*": "deny"both need explicit"ask").estimateCostand the guard threshold logic if you'd like it in this PR.Checklist
Summary by cubic
Adds an opt-in cost firewall that estimates a query’s scan bytes and cost before execution and asks for approval when it’s over budget. BigQuery uses dry‑run; Snowflake uses
EXPLAINfor a planner-estimated scan;/statusshows the budget; unsupported warehouses skip the guard.New Features
Connector.estimateCost(); BigQuery dry‑run returns exact bytes; SnowflakeEXPLAIN USING JSONreturns planner-estimated bytes (USD is a proxy; prefer byte limits on Snowflake).sql.estimate_costconverts bytes→USD usinggovernance.cost_per_tib_usd(default $6.25/TiB).sql_cost_estimatefor on‑demand “what will this cost?” checks.sql_executechecks the estimate and prompts viasql_execute_costwhen overgovernance.max_query_cost_usdorgovernance.max_bytes_scanned; fails open if estimation is unsupported./statusshows “Cost Firewall” state and per‑query budget.Migration
governance.max_query_cost_usdand/orgovernance.max_bytes_scanned(optionallycost_per_tib_usd).sql_execute_cost: "ask"andsql_cost_estimate: "allow"for roles allowed to approve/estimate.Written for commit fcfaf89. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests