Conversation
The budgets plugin needs AST access to replace fragile string-parsing heuristics. Moving it from runtime-executor (framework) to sql-runtime (SQL domain) — next to the lints plugin — enables importing SQL AST types. Re-exports are rewired so the public API is unchanged. Unit tests for the current heuristic behavior are added alongside the move to guard against regressions during the AST rewrite.
Replace string-parsing heuristics with AST-based checks: - SELECT detection via instanceof SelectAst (not sql.startsWith) - LIMIT detection via ast.limit (not plan.meta.annotations) - Aggregate without GROUP BY estimates 1 row The raw SQL fallback path (string parsing, annotations, EXPLAIN) is preserved for plans without an AST.
runtime-executor README no longer lists budgets as an export. sql-runtime README describes budgets as a local AST-first plugin.
PlanRefs.tables is readonly string[], so the parameter type must accept readonly arrays.
… edge case Lock in the 'ast' source value in budget error details and add a test for unbounded SelectAst without table refs (code review F01 + F02).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMoves the budgets plugin and its type from the framework runtime-executor into the SQL runtime, implements a new AST-first SQL-domain budgets plugin with heuristic and optional EXPLAIN fallbacks, updates exports and READMEs, tweaks a CLI formatter line, and adds comprehensive Vitest tests for budgets behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Executor
participant BudgetsPlugin as Budgets Plugin
participant PlanAst as Plan.ast
participant Heuristics
participant Driver as ctx.driver.explain
participant Context as PluginContext
Executor->>BudgetsPlugin: beforeExecute(plan, ctx)
alt plan.ast is SelectAst
BudgetsPlugin->>PlanAst: inspect SelectAst (tables, limit, aggregates)
BudgetsPlugin-->>Context: emit BUDGET.ROWS_EXCEEDED (source: 'ast') or continue
else no AST or non-SelectAst
BudgetsPlugin->>Heuristics: analyze sql/meta for limits
alt heuristics indicate bounded
BudgetsPlugin-->>Context: continue (within budget)
else heuristics unbounded
BudgetsPlugin->>Driver: explain(sql, params) [optional fallback]
alt explain provides estimate
BudgetsPlugin-->>Context: emit violation (source: 'explain')
else explain fails
BudgetsPlugin-->>Context: proceed without row violation
end
end
end
Executor->>BudgetsPlugin: onRow(row)
BudgetsPlugin-->>Context: increment observed rows -> emit 'observed' violation if exceeded
Executor->>BudgetsPlugin: afterExecute(result)
BudgetsPlugin-->>Context: check latency -> warn or error per severities & mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
@prisma-next/runtime-executor
@prisma-next/sql-runtime
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/contract-authoring
@prisma-next/contract-ts
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/cli
@prisma-next/emitter
@prisma-next/eslint-plugin
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/family-sql
@prisma-next/sql-kysely-lane
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-lane-sql-builder-new
@prisma-next/sql-lane
@prisma-next/target-postgres
@prisma-next/adapter-postgres
@prisma-next/driver-postgres
@prisma-next/core-control-plane
@prisma-next/core-execution-plane
@prisma-next/config
@prisma-next/contract
@prisma-next/operations
@prisma-next/plan
@prisma-next/utils
commit: |
![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/2-sql/5-runtime/test/budgets.test.ts (1)
45-53: Consider removing unnecessary type assertions increatePlan.The
as PlanMeta(line 50) andas ExecutionPlan(line 52) casts might be hiding type mismatches. If the spread produces a validPlanMeta/ExecutionPlan, the casts are redundant. If not, the helper may need adjustment.Per coding guidelines, test code should use double casts (
as unknown as X) at mock boundaries rather than directascasts to make the unsafe boundary explicit.♻️ Suggested adjustment
function createPlan(overrides: PlanOverrides): ExecutionPlan { const { meta: metaOverrides, ...rest } = overrides; - return { + return { sql: 'SELECT 1', params: [], - meta: { ...baseMeta, ...(metaOverrides ?? {}) } as PlanMeta, + meta: { ...baseMeta, ...(metaOverrides ?? {}) }, ...rest, - } as ExecutionPlan; + } as unknown as ExecutionPlan; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/test/budgets.test.ts` around lines 45 - 53, The helper createPlan currently uses unsafe one-step casts "as PlanMeta" and "as ExecutionPlan"; remove those direct casts and let TypeScript infer types by ensuring the returned object matches ExecutionPlan (adjust the meta spread to produce a PlanMeta using baseMeta and metaOverrides) — if the test truly needs an unsafe mock, replace each single "as X" with an explicit double cast "as unknown as X" to mark the unsafe boundary; target the createPlan function and the PlanOverrides/PlanMeta spread (baseMeta, metaOverrides) when making the change.packages/2-sql/5-runtime/src/plugins/budgets.ts (1)
261-306: Consider extractinghasAggregateWithoutGroupBycall to avoid duplication.
hasAggregateWithoutGroupBy(ast)is called twice inevaluateSelectAst: once inestimateRowsFromAst(line 131) and again directly (line 267). Since this involves iterating over projections, consider storing the result in a local variable.♻️ Proposed optimization
function evaluateSelectAst( plan: ExecutionPlan, ast: SelectAst, ctx: PluginContext<TContract, TAdapter, TDriver>, ) { + const isAggregateOnly = hasAggregateWithoutGroupBy(ast); - const estimated = estimateRowsFromAst(ast, tableRows, defaultTableRows, plan.meta.refs); - const isUnbounded = ast.limit === undefined && !hasAggregateWithoutGroupBy(ast); + const estimated = isAggregateOnly + ? 1 + : estimateRowsFromAstCore(ast, tableRows, defaultTableRows, plan.meta.refs); + const isUnbounded = ast.limit === undefined && !isAggregateOnly; const shouldBlock = rowSeverity === 'error' || ctx.mode === 'strict';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 261 - 306, evaluateSelectAst calls hasAggregateWithoutGroupBy(ast) twice (once locally and once inside estimateRowsFromAst); compute it once and reuse: create a local const (e.g., hasAggNoGroup = hasAggregateWithoutGroupBy(ast)), use that in the isUnbounded check inside evaluateSelectAst, and refactor estimateRowsFromAst to accept an optional hasAggregateWithoutGroup boolean parameter (update its signature and all callers) so estimateRowsFromAst can use the precomputed value instead of recomputing; update calls to estimateRowsFromAst within evaluateSelectAst to pass hasAggNoGroup.
🤖 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/2-sql/5-runtime/src/plugins/budgets.ts`:
- Around line 261-306: evaluateSelectAst calls hasAggregateWithoutGroupBy(ast)
twice (once locally and once inside estimateRowsFromAst); compute it once and
reuse: create a local const (e.g., hasAggNoGroup =
hasAggregateWithoutGroupBy(ast)), use that in the isUnbounded check inside
evaluateSelectAst, and refactor estimateRowsFromAst to accept an optional
hasAggregateWithoutGroup boolean parameter (update its signature and all
callers) so estimateRowsFromAst can use the precomputed value instead of
recomputing; update calls to estimateRowsFromAst within evaluateSelectAst to
pass hasAggNoGroup.
In `@packages/2-sql/5-runtime/test/budgets.test.ts`:
- Around line 45-53: The helper createPlan currently uses unsafe one-step casts
"as PlanMeta" and "as ExecutionPlan"; remove those direct casts and let
TypeScript infer types by ensuring the returned object matches ExecutionPlan
(adjust the meta spread to produce a PlanMeta using baseMeta and metaOverrides)
— if the test truly needs an unsafe mock, replace each single "as X" with an
explicit double cast "as unknown as X" to mark the unsafe boundary; target the
createPlan function and the PlanOverrides/PlanMeta spread (baseMeta,
metaOverrides) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 29a9001b-37b2-4955-a410-748d2974695d
📒 Files selected for processing (7)
packages/1-framework/3-tooling/cli/src/utils/formatters/help.tspackages/1-framework/4-runtime-executor/README.mdpackages/1-framework/4-runtime-executor/src/exports/index.tspackages/2-sql/5-runtime/README.mdpackages/2-sql/5-runtime/src/exports/index.tspackages/2-sql/5-runtime/src/plugins/budgets.tspackages/2-sql/5-runtime/test/budgets.test.ts
💤 Files with no reviewable changes (1)
- packages/1-framework/4-runtime-executor/src/exports/index.ts
…domain-to-sql-domain-next-to-lints
…domain-to-sql-domain-next-to-lints
Code Review SummaryOverall: The domain move is architecturally sound — zero dependency violations, consistent with the 🔴 P1 — Critical (3 findings)1. EXPLAIN fallback is silently broken — driver interface mismatch The local Suggested fix: Align the call with the canonical const result = await driver.explain({ sql: plan.sql, params: [...plan.params] });2. The Suggested fix: Use a 3. Inconsistent
This means Suggested fix: Decide on a single convention and document it. 🟡 P2 — Important (5 findings)4. Structural duplication between 5. 6. 7. Duplicated test fixtures — 8. 🔵 P3 — Nice-to-Have (7 findings)
Automated multi-agent review (6 agents: architecture, patterns, security, performance, simplicity, git history) |
…ithoutGroup required
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/2-sql/5-runtime/src/plugins/budgets.ts (5)
192-197: Consider validating configuration options.No validation guards against invalid values like
maxRows: 0,maxRows: -1, orNaN. These could cause confusing runtime behavior (e.g.,0triggers immediate violations, negative values never trigger).const maxRows = options?.maxRows ?? 10_000; if (maxRows <= 0 || !Number.isFinite(maxRows)) { throw new Error(`budgets: maxRows must be a positive finite number, got ${maxRows}`); } // Similar for maxLatencyMs, defaultTableRows🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 192 - 197, Add validation for budget configuration values so invalid numbers (0, negative, NaN, Infinity) are rejected: check options?.maxRows (maxRows), options?.defaultTableRows (defaultTableRows) and options?.maxLatencyMs (maxLatencyMs) to ensure each is a positive finite number and throw a clear Error if not; similarly validate severities (options?.severities.rowCount -> rowSeverity and options?.severities.latency -> latencySeverity) to ensure they are one of the expected strings (e.g., "error","warn","info") and throw if invalid. Locate the assignments to maxRows, defaultTableRows, tableRows, maxLatencyMs, rowSeverity and latencySeverity in budgets.ts and add these guards immediately after those assignments so callers get fast, explicit failures instead of surprising runtime behavior.
361-361: HoistexplainEnabledto plugin creation time.This constant is derived from immutable
optionsbut recomputed on everybeforeExecutecall. Hoisting it alongside other option destructuring improves clarity and avoids repeated closure access.const rowSeverity = options?.severities?.rowCount ?? 'error'; const latencySeverity = options?.severities?.latency ?? 'warn'; + const explainEnabled = options?.explain?.enabled === true; // ... inside evaluateWithHeuristics: - const explainEnabled = options?.explain?.enabled === true; if (explainEnabled && isSelect && typeof ctx.driver === 'object' && ctx.driver !== null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` at line 361, Hoist the computed flag explainEnabled out of the beforeExecute handler and compute it once at plugin creation time where other option destructuring occurs; specifically, derive const explainEnabled = options?.explain?.enabled === true alongside existing option unpacking and remove the per-call recomputation inside beforeExecute so beforeExecute uses the precomputed explainEnabled constant (reference symbols: explainEnabled, beforeExecute, options).
257-378: Structural duplication betweenevaluateSelectAstandevaluateWithHeuristics.Both functions follow the same pattern: compute estimate → check unbounded → emit violation. Consider extracting a shared helper to reduce duplication:
Sketch of extracted helper
function checkRowBudget( ctx: PluginContext<unknown, unknown, unknown>, opts: { source: 'ast' | 'heuristic' | 'explain'; estimated: number | null; isUnbounded: boolean; shouldBlock: boolean; maxRows: number; }, ): void { const { source, estimated, isUnbounded, shouldBlock, maxRows } = opts; if (isUnbounded) { emitBudgetViolation( budgetError('BUDGET.ROWS_EXCEEDED', 'Unbounded SELECT query exceeds budget', { source, ...(estimated !== null && estimated >= maxRows ? { estimatedRows: estimated } : {}), maxRows, }), shouldBlock, ctx, ); return; } if (estimated !== null && estimated > maxRows) { emitBudgetViolation( budgetError('BUDGET.ROWS_EXCEEDED', 'Estimated row count exceeds budget', { source, estimatedRows: estimated, maxRows, }), shouldBlock, ctx, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 257 - 378, Extract a shared helper (e.g., checkRowBudget) and replace the duplicated emit/condition logic in evaluateSelectAst and evaluateWithHeuristics: the helper should accept ctx plus an opts object ({ source: 'ast'|'heuristic'|'explain', estimated: number|null, isUnbounded: boolean, shouldBlock: boolean, maxRows: number }) and implement the same behavior (if isUnbounded emit an "Unbounded SELECT" violation including estimatedRows only when estimated !== null && estimated >= maxRows, otherwise emit with maxRows; if not unbounded and estimated !== null && estimated > maxRows emit the "Estimated row count" violation). Call this helper from evaluateSelectAst (pass source='ast') and from evaluateWithHeuristics for the heuristic path (source='heuristic') and the explain path (source='explain'), keeping the existing shouldBlock calculation and ctx casts.
60-97: Consider adding recursion depth limit.
findPlanRowsrecursively walks the EXPLAIN output without a depth limit. While EXPLAIN results are typically shallow, a malformed or adversarial response could cause stack overflow. This is low-risk but worth hardening.Proposed depth-limited version
-function findPlanRows(node: unknown): number | undefined { +function findPlanRows(node: unknown, depth = 0): number | undefined { + if (depth > 50) { + return undefined; + } if (!node || typeof node !== 'object') { return undefined; } const explainNode = node as ExplainNode; const planRows = explainNode['Plan Rows']; if (typeof planRows === 'number') { return planRows; } if ('Plan' in explainNode && explainNode.Plan !== undefined) { - const nested = findPlanRows(explainNode.Plan); + const nested = findPlanRows(explainNode.Plan, depth + 1); if (nested !== undefined) { return nested; } } // ... similar changes for other recursive calls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 60 - 97, The recursive walker findPlanRows can overflow on deeply nested/malformed EXPLAIN objects; add a recursion depth limit by introducing an optional depth parameter (e.g., depth or remainingDepth) with a sensible default max (e.g., 50), check at the top of findPlanRows and return undefined if depth is exhausted, and decrement/pass the depth on every recursive call (calls from explainNode.Plan, explainNode.Plans children, and Object.values traversal) so recursion stops deterministically when the limit is reached.
208-214:instanceofcheck may silently fail ifsql-relational-coreis duplicated.If npm installs multiple copies of
@prisma-next/sql-relational-core, theinstanceof SelectAstcheck will fail even for validSelectAstinstances (different class identities). The code then falls through to line 212-214 and returns early, silently skipping all estimation.Consider logging when
plan.astexists but isn't recognized:if (plan.ast instanceof SelectAst) { return evaluateSelectAst(plan, plan.ast, ctx); } if (plan.ast) { + ctx.log.debug({ + message: 'Unknown AST type in budgets plugin, skipping AST-based estimation', + astConstructor: plan.ast.constructor?.name, + }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 208 - 214, When plan.ast exists but the instanceof SelectAst check fails we should not silently return; add a warning log there so we can detect duplicated sql-relational-core issues. Update the block that checks plan.ast/SelectAst (using the same symbols plan.ast, SelectAst, evaluateSelectAst and ctx) to detect the case where plan.ast is truthy but not an instance of SelectAst and call the logger (e.g., ctx.logger.warn/processLogger.warn) including the plan id/context, plan.ast.constructor?.name and a short serialised preview of plan.ast to aid debugging, then keep the early return as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/5-runtime/src/plugins/budgets.ts`:
- Around line 243-244: The latency budget uses AND semantics while row budgets
use OR, causing inconsistent blocking behavior; change the latency check so
shouldBlock for latency uses the same OR logic as rows (replace the
latencySeverity === 'error' && ctx.mode === 'strict' expression with
latencySeverity === 'error' || ctx.mode === 'strict' in the budgets plugin), and
update any related tests or BudgetsOptions docs/comments to reflect the unified
behavior (symbols to look for: shouldBlock, latencySeverity, rowSeverity,
ctx.mode).
- Line 206: The call to ctx.now() is dead because its return value is discarded;
either remove the call entirely or capture and use its result (e.g., const start
= ctx.now()) where appropriate. Locate the call to ctx.now() in the budgets
plugin (the line with "void ctx.now();") and either delete that statement if it
has no side effects, or assign its return to a meaningful variable
(startTime/now) and use it for timing/caching logic in the surrounding function.
- Around line 199-235: The shared closure variable observedRows causes
cross-request interference; replace it with a per-execution map (e.g., a WeakMap
keyed by the PluginContext or ExecutionPlan) so each execution tracks its own
count. In the budgets plugin, remove the top-level observedRows and introduce
observedRowsByCtx (WeakMap), set observedRowsByCtx.set(ctx, 0) in beforeExecute
(for SelectAst and heuristics paths), increment and read the count from
observedRowsByCtx using the ctx parameter inside onRow, and throw budgetError
when that per-context count exceeds maxRows; also clean up the entry
(observedRowsByCtx.delete(ctx)) in an appropriate lifecycle hook (e.g.,
afterExecute) to avoid leaks.
- Around line 19-40: The explain signature in DriverWithExplain doesn't match
the actual driver: change the interface and call site so explain accepts the
same single request object shape used by the driver (e.g. explain(request:
SqlExecuteRequest) or a compatible object), and update computeEstimatedRows to
pass the execution plan as that request (or construct a SqlExecuteRequest from
ExecutionPlan) when calling driver.explain(plan/req); keep the try/catch and
then call extractEstimatedRows(result.rows) as before so estimation works with
the real driver implementation.
---
Nitpick comments:
In `@packages/2-sql/5-runtime/src/plugins/budgets.ts`:
- Around line 192-197: Add validation for budget configuration values so invalid
numbers (0, negative, NaN, Infinity) are rejected: check options?.maxRows
(maxRows), options?.defaultTableRows (defaultTableRows) and
options?.maxLatencyMs (maxLatencyMs) to ensure each is a positive finite number
and throw a clear Error if not; similarly validate severities
(options?.severities.rowCount -> rowSeverity and options?.severities.latency ->
latencySeverity) to ensure they are one of the expected strings (e.g.,
"error","warn","info") and throw if invalid. Locate the assignments to maxRows,
defaultTableRows, tableRows, maxLatencyMs, rowSeverity and latencySeverity in
budgets.ts and add these guards immediately after those assignments so callers
get fast, explicit failures instead of surprising runtime behavior.
- Line 361: Hoist the computed flag explainEnabled out of the beforeExecute
handler and compute it once at plugin creation time where other option
destructuring occurs; specifically, derive const explainEnabled =
options?.explain?.enabled === true alongside existing option unpacking and
remove the per-call recomputation inside beforeExecute so beforeExecute uses the
precomputed explainEnabled constant (reference symbols: explainEnabled,
beforeExecute, options).
- Around line 257-378: Extract a shared helper (e.g., checkRowBudget) and
replace the duplicated emit/condition logic in evaluateSelectAst and
evaluateWithHeuristics: the helper should accept ctx plus an opts object ({
source: 'ast'|'heuristic'|'explain', estimated: number|null, isUnbounded:
boolean, shouldBlock: boolean, maxRows: number }) and implement the same
behavior (if isUnbounded emit an "Unbounded SELECT" violation including
estimatedRows only when estimated !== null && estimated >= maxRows, otherwise
emit with maxRows; if not unbounded and estimated !== null && estimated >
maxRows emit the "Estimated row count" violation). Call this helper from
evaluateSelectAst (pass source='ast') and from evaluateWithHeuristics for the
heuristic path (source='heuristic') and the explain path (source='explain'),
keeping the existing shouldBlock calculation and ctx casts.
- Around line 60-97: The recursive walker findPlanRows can overflow on deeply
nested/malformed EXPLAIN objects; add a recursion depth limit by introducing an
optional depth parameter (e.g., depth or remainingDepth) with a sensible default
max (e.g., 50), check at the top of findPlanRows and return undefined if depth
is exhausted, and decrement/pass the depth on every recursive call (calls from
explainNode.Plan, explainNode.Plans children, and Object.values traversal) so
recursion stops deterministically when the limit is reached.
- Around line 208-214: When plan.ast exists but the instanceof SelectAst check
fails we should not silently return; add a warning log there so we can detect
duplicated sql-relational-core issues. Update the block that checks
plan.ast/SelectAst (using the same symbols plan.ast, SelectAst,
evaluateSelectAst and ctx) to detect the case where plan.ast is truthy but not
an instance of SelectAst and call the logger (e.g.,
ctx.logger.warn/processLogger.warn) including the plan id/context,
plan.ast.constructor?.name and a short serialised preview of plan.ast to aid
debugging, then keep the early return as before.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6770fe1b-eb16-4d33-9d51-7f70e088c5b3
📒 Files selected for processing (1)
packages/2-sql/5-runtime/src/plugins/budgets.ts
|
Thanks for the thorough review @jkomyno! Here's what I addressed and what I'm leaving for follow-ups. Addressed in this PR
Both were also flagged by CodeRabbit and are fixed in the latest push. Not addressing (pre-existing, out of scope)P1-1 — EXPLAIN driver interface mismatch: Confirmed — the local P1-2 — P1-3 — P2-4 — Structural duplication: The two functions handle materially different cases — P2-6 — P2-7 — Test fixture duplication: Same — affects lints tests. Out of scope. P2-8 — P3-10 — Repeated P3-11 — P3-12 — P3-13/14/15 — Input validation, depth limit, hoist const: All pre-existing patterns, not introduced by this PR. Happy to file follow-up tickets for P1-1 (EXPLAIN interface), and P1-3 (severity semantics) if you'd like those tracked. |
…d code, shouldBlock - Fix DriverWithExplain to use single-object `explain(request)` matching the actual postgres driver signature (was silently failing via catch block) - Replace shared `observedRows` closure variable with per-plan WeakMap tracking to prevent cross-request row count interference in concurrent executions - Remove dead `void ctx.now()` call whose return value was discarded - Align latency shouldBlock to use OR logic (like row budgets) so strict mode consistently blocks for all budget types
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/2-sql/5-runtime/src/plugins/budgets.ts (4)
249-249: Repeated type casts could be avoided with narrower helper signatures.The
ctx as PluginContext<unknown, unknown, unknown>cast appears 8 times. Consider makingemitBudgetViolationgeneric or accepting a narrower interface containing onlylogandmode.Narrower interface approach
+interface BudgetEmitContext { + readonly mode: 'strict' | 'permissive'; + readonly log: { warn(event: unknown): void }; +} function emitBudgetViolation( error: ReturnType<typeof budgetError>, shouldBlock: boolean, - ctx: PluginContext<unknown, unknown, unknown>, + ctx: BudgetEmitContext, ): void { // ... implementation unchanged }This allows passing
ctxdirectly without casting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` at line 249, The repeated casts of ctx as PluginContext<unknown, unknown, unknown> can be eliminated by changing emitBudgetViolation to be generic or to accept a narrower interface (e.g., type BudgetEmitterContext = { log: typeof ctx.log; mode: typeof ctx.mode } or a generic type parameter C extends { log: Logger; mode: Mode }). Update the signature of emitBudgetViolation (and any internal calls) to use the new generic/narrow type so callers can pass ctx directly without casting; adjust any call sites that rely on the old specific PluginContext type to match the new parameter type while preserving existing behavior and exports.
99-116: Consider extracting a shared runtime error factory.This
budgetErrorfactory duplicates the pattern inlints.ts(lintError). A sharedcreateRuntimeError(category, code, message, details)factory would reduce duplication. This is a recommended refactor that could be addressed in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 99 - 116, The budgetError factory duplicates lintError; extract a shared helper like createRuntimeError(category: string, code: string, message: string, details?: Record<string, unknown>) and refactor both budgetError and lintError to call it; implement createRuntimeError to construct an Error, set name to 'RuntimeError', and assign code, category, severity: 'error', and details before returning so budgetError and lintError become thin wrappers that pass 'BUDGET' or the lint category plus the code/message/details into the new factory.
60-97: Unbounded recursion infindPlanRowscould overflow the stack on deeply nested EXPLAIN output.While unlikely in practice, a malformed or malicious EXPLAIN result with excessive nesting could cause a stack overflow. Consider adding a depth limit parameter.
Optional depth-limited fix
-function findPlanRows(node: unknown): number | undefined { +function findPlanRows(node: unknown, depth = 0): number | undefined { + if (depth > 100) return undefined; + if (!node || typeof node !== 'object') { return undefined; } const explainNode = node as ExplainNode; const planRows = explainNode['Plan Rows']; if (typeof planRows === 'number') { return planRows; } if ('Plan' in explainNode && explainNode.Plan !== undefined) { - const nested = findPlanRows(explainNode.Plan); + const nested = findPlanRows(explainNode.Plan, depth + 1); if (nested !== undefined) { return nested; } } if (Array.isArray(explainNode.Plans)) { for (const child of explainNode.Plans) { - const nested = findPlanRows(child); + const nested = findPlanRows(child, depth + 1); if (nested !== undefined) { return nested; } } } for (const value of Object.values(node as Record<string, unknown>)) { if (typeof value === 'object' && value !== null) { - const nested = findPlanRows(value); + const nested = findPlanRows(value, depth + 1); if (nested !== undefined) { return nested; } } } return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 60 - 97, findPlanRows can recurse indefinitely; add a depth-limit parameter and stop recursion once the limit is reached. Change the signature to accept an optional remainingDepth (e.g., findPlanRows(node: unknown, remainingDepth = 50)), immediately return undefined if remainingDepth <= 0, and pass remainingDepth - 1 on every recursive call (for explainNode.Plan, each child in explainNode.Plans, and when iterating Object.values). Keep the default remainingDepth high enough (e.g., 50) so typical EXPLAIN trees are unaffected and no caller changes are required.
255-376: Duplication betweenevaluateSelectAstandevaluateWithHeuristicscould be reduced.Both functions share a similar pattern: estimate rows → check if unbounded → emit violation if exceeds budget. Consider extracting a shared
checkRowBudget(estimated, isUnbounded, source, shouldBlock, ctx)helper to reduce duplication. This is a recommended refactor for maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 255 - 376, Both evaluateSelectAst and evaluateWithHeuristics duplicate row-budget logic; extract a helper (e.g. checkRowBudget) that accepts (estimated: number|null|undefined, isUnbounded: boolean, source: 'ast'|'heuristic'|'explain', shouldBlock: boolean, ctx: PluginContext<...>, maxRows: number) and encapsulates the decision tree: if isUnbounded -> if estimated != null && estimated >= maxRows emitBudgetViolation with source and estimatedRows then return else emitBudgetViolation without estimatedRows then return; else if estimated != null && estimated > maxRows emitBudgetViolation with estimatedRows; keep caller behavior identical (use same budgetError codes/messages and shouldBlock casting) and call this helper from evaluateSelectAst, evaluateWithHeuristics (including the explain branch) instead of duplicating the checks, ensuring types and null/undefined handling for estimated are preserved.
🤖 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/2-sql/5-runtime/src/plugins/budgets.ts`:
- Line 249: The repeated casts of ctx as PluginContext<unknown, unknown,
unknown> can be eliminated by changing emitBudgetViolation to be generic or to
accept a narrower interface (e.g., type BudgetEmitterContext = { log: typeof
ctx.log; mode: typeof ctx.mode } or a generic type parameter C extends { log:
Logger; mode: Mode }). Update the signature of emitBudgetViolation (and any
internal calls) to use the new generic/narrow type so callers can pass ctx
directly without casting; adjust any call sites that rely on the old specific
PluginContext type to match the new parameter type while preserving existing
behavior and exports.
- Around line 99-116: The budgetError factory duplicates lintError; extract a
shared helper like createRuntimeError(category: string, code: string, message:
string, details?: Record<string, unknown>) and refactor both budgetError and
lintError to call it; implement createRuntimeError to construct an Error, set
name to 'RuntimeError', and assign code, category, severity: 'error', and
details before returning so budgetError and lintError become thin wrappers that
pass 'BUDGET' or the lint category plus the code/message/details into the new
factory.
- Around line 60-97: findPlanRows can recurse indefinitely; add a depth-limit
parameter and stop recursion once the limit is reached. Change the signature to
accept an optional remainingDepth (e.g., findPlanRows(node: unknown,
remainingDepth = 50)), immediately return undefined if remainingDepth <= 0, and
pass remainingDepth - 1 on every recursive call (for explainNode.Plan, each
child in explainNode.Plans, and when iterating Object.values). Keep the default
remainingDepth high enough (e.g., 50) so typical EXPLAIN trees are unaffected
and no caller changes are required.
- Around line 255-376: Both evaluateSelectAst and evaluateWithHeuristics
duplicate row-budget logic; extract a helper (e.g. checkRowBudget) that accepts
(estimated: number|null|undefined, isUnbounded: boolean, source:
'ast'|'heuristic'|'explain', shouldBlock: boolean, ctx: PluginContext<...>,
maxRows: number) and encapsulates the decision tree: if isUnbounded -> if
estimated != null && estimated >= maxRows emitBudgetViolation with source and
estimatedRows then return else emitBudgetViolation without estimatedRows then
return; else if estimated != null && estimated > maxRows emitBudgetViolation
with estimatedRows; keep caller behavior identical (use same budgetError
codes/messages and shouldBlock casting) and call this helper from
evaluateSelectAst, evaluateWithHeuristics (including the explain branch) instead
of duplicating the checks, ensuring types and null/undefined handling for
estimated are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1a4d85cf-28d8-4375-926f-3e1d910197b0
📒 Files selected for processing (2)
packages/2-sql/5-runtime/src/plugins/budgets.tspackages/2-sql/5-runtime/test/budgets.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/2-sql/5-runtime/src/plugins/budgets.ts (2)
207-215:⚠️ Potential issue | 🟠 MajorDon't skip heuristic fallback for unrecognized
SELECTASTs.Line 211 returns on any truthy
plan.astthat failsinstanceof SelectAst. That means a duplicated@prisma-next/sql-relational-corecopy, or any non-identicalSelectAstconstructor, disables all pre-execution row checks forSELECTplans instead of falling back to the SQL-text heuristics.Suggested fix
if (plan.ast instanceof SelectAst) { return evaluateSelectAst(plan, plan.ast, ctx); } if (plan.ast) { - return; + const sqlUpper = plan.sql.trimStart().toUpperCase(); + if (sqlUpper.startsWith('SELECT')) { + return evaluateWithHeuristics(plan, ctx); + } + return; } return evaluateWithHeuristics(plan, ctx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 207 - 215, The current early return when plan.ast is truthy but not an instance of SelectAst skips heuristic fallback; instead, ensure non-SelectAst truthy ASTs fall through to the heuristics. Change the logic around SelectAst/plan.ast so that if plan.ast instanceof SelectAst you call evaluateSelectAst(plan, plan.ast, ctx), otherwise (when plan.ast is truthy but not SelectAst) call or fall through to evaluateWithHeuristics(plan, ctx) rather than returning; update the block that references SelectAst, evaluateSelectAst, evaluateWithHeuristics and plan.ast accordingly.
241-249:⚠️ Potential issue | 🟠 Major
latency: 'error'is still downgraded to warn in permissive mode.Line 242 only blocks when both knobs are set, so
severities.latency: 'error'merely logs in permissive mode. Row budgets on Line 269 do not behave that way, which makes the public budget API internally inconsistent again. Either align the blocking rule or explicitly split/document the semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 241 - 249, The latency budget currently only blocks when latencySeverity === 'error' AND ctx.mode === 'strict', which causes severities.latency: 'error' to be downgraded to a log in permissive mode; change the blocking logic to match row-budget semantics by making shouldBlock true whenever latencySeverity === 'error' (regardless of ctx.mode) and only block for 'warn' when ctx.mode === 'strict' (e.g. const shouldBlock = latencySeverity === 'error' || (latencySeverity === 'warn' && ctx.mode === 'strict')), then pass that shouldBlock into emitBudgetViolation (and ensure this matches the rows severity handling used elsewhere).
🧹 Nitpick comments (1)
packages/2-sql/5-runtime/src/plugins/budgets.ts (1)
189-197: ValidateBudgetsOptionsbefore closing over it.
NaN,Infinity, or negative numbers here silently disable or invert checks (observedRows > NaNis always false,Math.min(NaN, tableEstimate)staysNaN). Normalize this config once at construction time instead of trusting raw values.As per coding guidelines, "Use config validation and normalization patterns with Arktype" and "Use schema validators (Arktype) for structural validation of required fields, types, and shapes rather than redundant manual checks for these properties."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/5-runtime/src/plugins/budgets.ts` around lines 189 - 197, Normalize and validate the BudgetsOptions inside the budgets factory before closing over it: create an Arktype schema for BudgetsOptions and run options through it, coercing/normalizing numeric fields (maxRows, defaultTableRows, maxLatencyMs and every entry in tableRows) to finite non-negative integers (use Math.floor/Math.max(0, Number(value)) or equivalent) and replacing NaN/Infinity/negatives with the configured defaults; also normalize severities to allowed values and fall back to 'error'/'warn' when missing; then assign the normalized values to the closed-over variables (maxRows, defaultTableRows, tableRows, maxLatencyMs, rowSeverity, latencySeverity) so the rest of the code that uses budgets, tableRows, and severities operates on safe, validated data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/5-runtime/src/plugins/budgets.ts`:
- Around line 344-356: The heuristic pre-execution row estimate branch should
only run for SELECT plans: update the conditional that checks estimated !== null
in the budgets plugin (the block that calls emitBudgetViolation with
budgetError('BUDGET.ROWS_EXCEEDED', ...)) to also require isSelect, so only when
isSelect is true do you emit the pre-execution rows-exceeded violation; leave
onRow() behavior unchanged for non-SELECT queries. Ensure you reference the
existing estimated variable, the isSelect boolean, and the emitBudgetViolation /
budgetError call so the change is applied to that exact branch.
---
Duplicate comments:
In `@packages/2-sql/5-runtime/src/plugins/budgets.ts`:
- Around line 207-215: The current early return when plan.ast is truthy but not
an instance of SelectAst skips heuristic fallback; instead, ensure non-SelectAst
truthy ASTs fall through to the heuristics. Change the logic around
SelectAst/plan.ast so that if plan.ast instanceof SelectAst you call
evaluateSelectAst(plan, plan.ast, ctx), otherwise (when plan.ast is truthy but
not SelectAst) call or fall through to evaluateWithHeuristics(plan, ctx) rather
than returning; update the block that references SelectAst, evaluateSelectAst,
evaluateWithHeuristics and plan.ast accordingly.
- Around line 241-249: The latency budget currently only blocks when
latencySeverity === 'error' AND ctx.mode === 'strict', which causes
severities.latency: 'error' to be downgraded to a log in permissive mode; change
the blocking logic to match row-budget semantics by making shouldBlock true
whenever latencySeverity === 'error' (regardless of ctx.mode) and only block for
'warn' when ctx.mode === 'strict' (e.g. const shouldBlock = latencySeverity ===
'error' || (latencySeverity === 'warn' && ctx.mode === 'strict')), then pass
that shouldBlock into emitBudgetViolation (and ensure this matches the rows
severity handling used elsewhere).
---
Nitpick comments:
In `@packages/2-sql/5-runtime/src/plugins/budgets.ts`:
- Around line 189-197: Normalize and validate the BudgetsOptions inside the
budgets factory before closing over it: create an Arktype schema for
BudgetsOptions and run options through it, coercing/normalizing numeric fields
(maxRows, defaultTableRows, maxLatencyMs and every entry in tableRows) to finite
non-negative integers (use Math.floor/Math.max(0, Number(value)) or equivalent)
and replacing NaN/Infinity/negatives with the configured defaults; also
normalize severities to allowed values and fall back to 'error'/'warn' when
missing; then assign the normalized values to the closed-over variables
(maxRows, defaultTableRows, tableRows, maxLatencyMs, rowSeverity,
latencySeverity) so the rest of the code that uses budgets, tableRows, and
severities operates on safe, validated data.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9736e264-f286-4cae-b265-0154afe99491
📒 Files selected for processing (2)
packages/2-sql/5-runtime/src/plugins/budgets.tspackages/2-sql/5-runtime/test/budgets.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/2-sql/5-runtime/test/budgets.test.ts
wmadden
deleted the
tml-2099-move-budgets-plugin-from-framework-domain-to-sql-domain-next-to-lints
branch
2 participants
closes TML-2099
Key snippet (Before / After)
Intent
Move the
budgetsruntime plugin from the Framework domain to the SQL domain (next tolints) so it can inspectQueryAststructurally, replacing fragile SQL string parsing and annotation heuristics with AST-based SELECT detection, LIMIT detection, and row estimation. The public API (import { budgets } from '@prisma-next/sql-runtime') is unchanged.Change map
The story
Move the plugin across the layering boundary:
budgets.tsmoves frompackages/1-framework/4-runtime-executor/src/plugins/topackages/2-sql/5-runtime/src/plugins/, placing it in the SQL domain where it can import AST types. Imports are updated: relative./typesbecomes@prisma-next/runtime-executor(matching howlints.tsalready imports plugin types). Exports are rewired so@prisma-next/sql-runtimesources budgets locally, and@prisma-next/runtime-executorno longer exports it.Replace heuristics with AST-based dispatch: The
beforeExecutemethod now checksplan.ast instanceof SelectAstfirst. When AST is present, SELECT detection is structural (type check, not string parsing), LIMIT detection readsast.limitdirectly (notplan.meta.annotations?.['limit']), and aggregate-without-GROUP-BY queries get an estimate of 1 row. When AST is absent (raw SQL), the original heuristics are preserved as a fallback.Extract duplication into helpers: The throw-or-warn pattern, previously duplicated ~6 times inline, is extracted into
emitBudgetViolation. Row estimation is split into two pure functions:estimateRowsFromAstandestimateRowsFromHeuristics. The inner dispatch functionsevaluateSelectAstandevaluateWithHeuristicsclose over plugin config.Behavior changes & evidence
SELECT detection uses AST type checking instead of SQL string parsing: Before →
plan.sql.trimStart().toUpperCase().startsWith('SELECT'). After →plan.ast instanceof SelectAstwhen AST is present; string parsing only when AST is absent.LIMIT detection reads from AST instead of annotations: Before →
plan.meta.annotations?.['limit']. After →ast.limitwhen SelectAst is present; annotation fallback when AST is absent.Aggregate-without-GROUP-BY estimates 1 row: Adds a new cardinality reduction: when a
SelectAstprojection contains anAggregateExprandgroupByis undefined, the estimated row count is 1. With GROUP BY present, no reduction is applied.SELECT COUNT(*) FROM userswill always return exactly 1 row. Without this, the plugin would flag it as an unbounded query exceeding the budget.hasAggregateWithoutGroupByestimateRowsFromAstBudget details gain
source: 'ast'value: When the estimate comes from AST inspection, thesourcefield in budget error details is'ast'(alongside existing values'heuristic','explain','observed').source: 'ast'source: 'ast'Export rewiring (no behavior change):
@prisma-next/runtime-executorno longer exportsbudgets/BudgetsOptions.@prisma-next/sql-runtime's barrel export now sources these from the local plugin file instead of forwarding them from runtime-executor.Code structure refactoring (no behavior change): Inline throw-or-warn logic extracted to
emitBudgetViolation. Estimation split intoestimateRowsFromAst/estimateRowsFromHeuristics. Dispatch split intoevaluateSelectAst/evaluateWithHeuristics.Compatibility / migration / risk
import { budgets } from '@prisma-next/sql-runtime'continues to work. TheBudgetsOptionstype andbudgets()function signature are unchanged.budgetsdirectly from@prisma-next/runtime-executorwill get a compile-time error. This is intentional — the spec requires removing the export.Non-goals / intentionally out of scope
evaluateRawGuardrails(stays in Framework domain)sql-sizefrom ADR 023)instanceof)Summary by CodeRabbit
Breaking Changes
budgetsandBudgetsOptionsare now provided by the SQL runtime; update imports accordingly.New Features / Improvements
Documentation
Tests