Repository navigation
feat(analyst): sql_check predicate_effect reports what each filter removes - #1446
anandgupta42 wants to merge 28 commits into
Conversation
…removes
Analysts are told that a filter which removes nothing usually means the wrong
column, but there was no cheap way to verify it. `sql_check` gains an opt-in
`predicate_effect` check: given an explicit read-only population (`base_sql`,
a SELECT or a table reference) and one to eight boolean predicates, it runs a
single aggregate probe
SELECT COUNT(*), COUNT(*) FILTER (WHERE p1), …, COUNT(*) FILTER (WHERE p1 AND …)
and reports rows kept and removed per predicate plus the combined count,
flagging `this predicate removes no rows at this population`. NULL counts as
not kept and the output says so; an empty population is reported as such.
- No rewriting of the caller's WHERE clause; predicates are validated as bare
expressions through the existing read-only validator and the connection's
dialect, and all supplied SQL goes through the `sql_execute` permission
request before routing.
- Bounded: SQLite, DuckDB and Postgres only; a catalog row estimate above
`max_rows_scanned` (default 5,000,000) for a bare table refuses with a
suggestion to narrow the population; query timeouts return
`skipped: timeout` instead of failing the caller.
- Analyst prompt: the existing "every clause must bite" sentence points at the
check and says a vacuous filter is a reason to profile an alternative
column, not to invent an exclusion.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…uts and all-row removal Review follow-ups to the predicate-effect probe: - When the catalog row estimate is unknown (views, SQLite without ANALYZE, unanalysed or partitioned Postgres tables, ambiguous relations) the output says `row_estimate: unknown — probe runs under the query timeout` instead of proceeding silently. - Strict-schema fillers (`max_rows_scanned: 0`, blank predicates, empty `base_sql`) are treated as absent and produce the tool's own message, never a raw schema validation dump. - A predicate that removes every row is flagged; counts over a `LIMIT`ed SELECT population say so. - Postgres branch covered by tests (probe shape, `to_regclass`/`reltuples` guard, unknown-estimate path). - The analyst prompt sentence ends with a period. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
full receipts (20 sessions)
codex ·
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52f0e5ed74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…nd keeps caller positions Independent-review follow-ups: - A bare-table population is wrapped under the table's own name, so a predicate copied from the caller's WHERE clause (`orders.kind = 'a'`) works; SELECT populations keep the `t` alias and the description says so. - The limited-population note fires only for a top-level `LIMIT`, `FETCH FIRST … ROWS ONLY` or `TOP` on the population statement, not for a LIMIT inside a subquery. - Blank predicate entries are reported as skipped at their original position, so predicate numbers match the caller's array. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25baadb1c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 25baadb. Configure here.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
… in predicates and populations A review finding of the highest severity tier: on PostgreSQL connections allowed to signal other backends, `pg_terminate_backend(pid)` or `pg_cancel_backend(pid)` passes the read-only validator as a boolean expression. The shared validator now blocks backend-signalling calls (including escaped function names and CR-comment variants) before any query runs; harmless literals, comments, column names and scalar expressions remain accepted. Tests: `rejects backend-signaling predicate <case> before query execution` and the population variant (14 cases). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…' into feat/analyst-v10-probe
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9b84ed5e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…trol and dblink functions Three review findings of the highest severity tier, each with red-first tests: - The probe executes with the connector's read-only mode, so a user-defined function that writes cannot be reached through a predicate. - `pg_promote` and `pg_reload_conf` are denied in predicates and populations. - `dblink_send_query`, `dblink_connect` and `dblink_connect_u` are denied in predicates and populations. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fa390c878
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Previous Review Summaries (7 snapshots, latest commit 214e47e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 214e47e)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 9ae7d77)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit 5a69583)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit c9cfe17)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 2f824cf)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 8e56751)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (8 files)
Fix these issues in Kilo Cloud The three prior critical findings are fixed at the current HEAD. No new inline findings were raised; existing comments were not duplicated. Review based on source and existing tests only; no code or tests were executed. Previous review (commit 3fa390c)Status: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Review based on source and existing tests only; no code or tests were executed. Earlier active review comments were not duplicated. Reviewed by deepseek-pro-latest · Input: 69.6K · Output: 62.7K · Cached: 1M Review guidance: REVIEW.md from base branch |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
…_effect` - Read-only classification: deny `pg_logical_emit_message` (its non-transactional form is written to WAL immediately and survives the rollback), bare `dblink` (the synchronous form runs its string argument on a remote connection outside the local read-only transaction) and every `pg_advisory_lock` / `pg_try_advisory_lock` / `pg_advisory_xact_lock` variant (session-level locks survive ROLLBACK and the session returns to the pool). Applies to the population and to every predicate; case-insensitive and schema-qualified spellings covered. - Permission: `PermissionNext.ask` evaluated patterns in order and returned a prompt on the first `ask`, so a later pattern with an explicit `deny` would execute once the earlier one was approved. It now evaluates every pattern before publishing the approval request (`upstream_fix`). - Local engines honour `readOnly`: SQLite runs the probe under a scoped `query_only` pragma restored afterwards; DuckDB runs it in a READ ONLY transaction that is rolled back, and refuses to start one while the caller has its own transaction open rather than aborting it. Previously the flag was silently ignored on both engines. - Regression tests for each denied function family, for the deny-before-prompt ordering, and for SQLite/DuckDB read-only enforcement (hidden sequence writes in a view and in a predicate subquery, pragma restoration after failure, and the active-transaction refusal). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…te_effect pg_advisory_unlock, pg_advisory_unlock_shared and pg_advisory_unlock_all release session-level locks held by the pooled connection, and a read-only rollback does not undo that. The denylist now covers every pg_advisory_* / pg_try_advisory_* function; regression cases added for the three unlock variants. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… the predicate_effect branch Resolves the conflict in `packages/drivers/src/duckdb.ts` by keeping the shared-engine generation tracking from the base and this branch's `readOnly` flag on the engine call.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 962a10f7-5127-45e8-8bfa-0981b2640611) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fce7bad1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…s in read-only `sql_check` sources The denylist named individual dblink functions, so `dblink_open`/`dblink_fetch`/`dblink_close` (a remote data-modifying cursor whose close commits) passed, and it omitted `pg_wal_replay_pause`/`pg_wal_replay_resume`, which change recovery state on a standby and are not undone by rollback. The list now matches `dblink\w*` and `pg_wal_replay_\w*`. Tests: dblink cursor open/fetch/close (incl. a data-modifying CTE), and both WAL replay controls, each rejected before query execution in plain, schema-qualified and commented spellings. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 724a32c4-b274-4bd6-bad5-6b51aa6d6aca) |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
…' into feat/analyst-v10-probe
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 4f9e1f33-a9eb-4008-b191-5cc1bc244621) |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d41c01214
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…' into feat/analyst-v10-probe
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 5dd3daaa-d4bd-4b44-a88d-0c50392ab734) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4953d4bc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…and guards the PostgreSQL estimate atomically - Predicates may call only functions in a fixed allowlist of built-in pure functions (the earlier `PURE_PG_FUNCTIONS` names plus ordinary scalar built-ins); every other call, including schema-qualified and user-defined wrappers such as `emit_wrapper(id)` or DuckDB `postgres_execute(...)`, is refused before any SQL runs with the function named. The side-effect denylist stays as a second check. Applies to PostgreSQL, SQLite and DuckDB alike. Identifier handling preserves quoted names and folds only ASCII case. - Bare-table PostgreSQL probes run the `pg_class`/`to_regclass` estimate and the aggregate in one statement, with the estimate condition gating population rows, so two pool clients can no longer see different relations (temporary tables, `search_path`) between the guard and the scan; an exceeded estimate returns `skipped: "estimate"` without a population scan. - tests: wrapper, qualified, quoted and pg-prefixed calls refused on every dialect; scalar built-ins, casts and boolean grouping accepted; single-statement estimate guard, exceeded estimate gated, unknown estimate vs empty population, CTE name collisions. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 03b94af4-0da4-4b25-ab22-f37e5ab1158d) |
…' into feat/analyst-v10-probe
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 1e7e0c5d-2782-4999-9be7-cdb265db29b6) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5df7cfa97f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…inst the catalog and bounds local populations in one statement - The built-in pure-function allowlist now applies to the population SQL as well as the predicates, on every dialect, so a DuckDB `postgres_execute(...)` or a PostgreSQL wrapper population is refused before any SQL runs. - PostgreSQL: before the probe, one read-only `pg_operator` query and one `pg_proc` query (bound `ANY($1)` arrays) cover every operator and function name referenced by the population and predicates, including implicit operators behind `LIKE`/`IN`/`BETWEEN`/`IS DISTINCT FROM` and the `!=` alias; any definition outside `pg_catalog` refuses the probe with the name. DuckDB checks `duckdb_functions()` for non-internal macros the same way; SQLite has no SQL-level function catalog (commented). - SQLite and DuckDB run the estimate and the aggregate on one population bounded to `cap + 1` rows in a single statement, so a concurrent peer cannot swap the relation between the guard and the scan; an exceeded bound returns `skipped: "estimate"`. PostgreSQL keeps its atomic catalog-estimate gate. - Allowlist gains the SQLite/DuckDB date built-ins (`strftime`, `julianday`, `unixepoch`, `datetime`, `timediff`) and the bare `array` constructor. - Drivers: the PostgreSQL connector forwards bind parameters on both the regular and byte-limited read paths; it previously dropped them. - tests: wrapper and remote-execute populations refused per dialect; user-defined operator/overload/macro refused and built-in-only accepted after read-only catalog queries; implicit operators and trailing-sign splits resolved; SQLite/DuckDB single-statement bound with exceeded skip; date built-ins accepted; PostgreSQL bind forwarding on both read paths. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…' into feat/analyst-v10-probe
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 1f333a9d-d168-4824-bbd1-d1ded7aece80) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ff8f08159
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… the catalog; repairs the PostgreSQL test fixture - Explicit casts (`x::type`, `CAST(x AS type)`) in populations and predicates are resolved against `pg_catalog.pg_type` with the same read-only bound catalog query; a target type outside `pg_catalog` refuses the probe before any SQL runs. Casts selected by a user-defined SOURCE type cannot be seen textually; the code comment and PR body disclose that. - CTE declarations and their column lists are no longer mistaken for function calls, so the advertised `WITH population(id) AS (...) SELECT ...` form is accepted; projection, qualified and `count(*)` wildcards (including `DISTINCT ON`) are excluded from the operator catalog check while multiplication is still checked. - `sql-check-postgres.test.ts` had been left outside the previous round's verification and was red on the branch: its fixture now answers the `pg_operator`/`pg_proc`/`pg_type` catalog queries (seedable), preserves bind arguments, and the unsafe-function cases assert the pure-function refusal with no population or probe SQL executed. - tests: custom cast targets refused before the probe (predicate and population, `::` and `CAST`, qualified/quoted/array/typmod, Unicode-escaped), built-in casts accepted, CTE column lists accepted, wildcard populations accepted beside a seeded custom `*`, multiplication still checked; fixture controls for an ordinary population, seeded catalog rows and bind preservation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…' into feat/analyst-v10-probe # Conflicts: # packages/drivers/test/postgres-readonly.test.ts
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: f9b32441-10a3-4c79-b947-9417a97410b7) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 902ac47d4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…cepts FILTER/OVER clauses - Permission rules are matched per pattern, so a `sql_execute` deny rule scoped to a column-and-table combination (`*secret_col*FROM*sensitive_table*`) matched neither the population nor the predicate on its own although the probe joins them. The tool now also asks for the combined probe in the shape it executes (`SELECT COUNT(*) FILTER (WHERE <predicate>) … FROM (<population>) AS t`), so such a rule denies before any SQL runs. - `FILTER (…)`, `OVER (…)`, `WITHIN GROUP (…)`, `USING (…)` and similar clause introducers are no longer mistaken for function calls, so valid aggregate and window populations are accepted while unknown functions are still refused. - The permission regression tests use the package `it.instance`/`TestInstance` fixture instead of a manual `Instance.provide`/`dispose` lifecycle. - tests: combined-probe deny before SQL and a non-matching control; FILTER/OVER population accepted and `filterx(id)` refused. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…' into feat/analyst-v10-probe
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 46ddc0cc-cfbf-4256-ae1f-b3836f664323) |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
3 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 388d088b96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Text cannot determine whether a cast's source column has a user-defined type. | ||
| const typeNames = queries.map((sql) => postgresCasts(sql).map((target) => target.name)) | ||
| const types = [...new Set(typeNames.flat())] | ||
| if (types.length) { | ||
| const [type] = await run( |
There was a problem hiding this comment.
When a PostgreSQL source column has a user-defined type with an explicit cast to a built-in target such as custom_col::boolean, this records only boolean and therefore finds no non-pg_catalog target type. PostgreSQL casts can execute the function stored in pg_cast.castfunc, so a conversion function can still invoke dblink_send_query or another nontransactional side effect for every row. Fresh evidence after the prior explicit-cast fix is that the current catalog query validates only the target type name, not the source type or resolved cast function; resolve the cast through pg_cast and require its implementation to be trusted. PostgreSQL pg_cast documentation
Useful? React with 👍 / 👎.
| const queries = [population, ...predicates] | ||
| const functionNames = queries.map((sql) => pureFunctions(sql, dialect)) |
There was a problem hiding this comment.
When base_sql is SELECT * FROM reporting_view and that view's defining query calls a side-effecting wrapper or dblink_send_query, pureFunctions sees no call in the submitted text, so the catalog checks pass and the view body executes during the probe despite the local read-only transaction. PostgreSQL runs a view's defining query whenever the view is referenced. Fresh evidence after the wrapper fix is that the new validation only maps pureFunctions over caller-supplied SQL and performs no pg_rewrite or view-definition lookup; referenced views must be expanded and validated, or probes must use credentials incapable of those effects. PostgreSQL CREATE VIEW documentation
Useful? React with 👍 / 👎.
| ) | ||
|
|
||
| // Predicate probes and their populations accept only these pure functions; keep this list flat and sorted. | ||
| const PURE_PREDICATE_FUNCTIONS = new Set([ |
There was a problem hiding this comment.
When a valid population uses a standard window function, for example SELECT *, row_number() OVER (PARTITION BY account_id ORDER BY created_at) AS rn FROM orders, function validation rejects row_number because the allowlist contains aggregate functions such as count and accepts the OVER clause but omits the actual general-purpose window functions. This prevents predicate_effect from checking common ranked or deduplicated populations on both PostgreSQL and DuckDB; add trusted built-ins such as row_number, rank, dense_rank, lag, and lead to the dialect-aware allowlist. PostgreSQL window-function documentation, DuckDB window-function documentation
Useful? React with 👍 / 👎.
| const bare = | ||
| /^(?:"(?:[^"]|"")+"|[A-Za-z_][A-Za-z0-9_$]*)(?:\s*\.\s*(?:"(?:[^"]|"")+"|[A-Za-z_][A-Za-z0-9_$]*)){0,2}$/.test( | ||
| sql.trim(), | ||
| ) |
There was a problem hiding this comment.
When a SQLite table is supplied as a bare reference using SQLite-native quoting, such as [order details] or `order details`, this regex does not recognize it as a table and instead passes the identifier alone to readSql, which rejects it for not being a SELECT. SQLite explicitly supports both square brackets and grave accents for quoted identifiers, and the tokenizer already recognizes those forms, so the bare-table parser and alias extraction should accept them for the SQLite dialect. SQLite identifier-quoting documentation
Useful? React with 👍 / 👎.
| if (!/^\s*\(/.test(functions.slice(match.index + match[0].length))) continue | ||
| if (declarations.has(match.index)) continue |
There was a problem hiding this comment.
When a valid population supplies a column list for a derived table, such as SELECT id FROM (VALUES (1), (2)) AS v(id), this scan treats v( as a function call and rejects it because only CTE declaration positions are excluded. DuckDB explicitly supports this alias-column syntax, including (VALUES (2), (4)) _(x), and PostgreSQL supports the same derived-table form; record derived-table and table-function alias lists as declarations rather than requiring their aliases to appear in the function allowlist. DuckDB FROM-clause documentation
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Issue for this PR
Closes #
Type of change
What does this PR do?
Why. The analyst prompt tells the model that "a filter that removes nothing usually means the wrong column", but there was no cheap, deterministic way to check what a filter clause actually does at the population it is applied to. In practice the model either skipped the check or guessed.
What changed.
sql_checkgains an opt-incheck: "predicate_effect". The caller supplies an explicit read-only population (base_sql: a SELECT or a bare table reference) and one to eight boolean predicates. The tool runs a single aggregate probeand reports rows kept and removed per predicate plus the combined count. A predicate that removes no rows is flagged (
this predicate removes no rows at this population), as is one that removes every row; a NULL predicate value counts as not kept and the output says so; an empty population and aLIMITed SELECT population are called out.Bounded by design: no rewriting of the caller's own WHERE clause (predicates are validated as bare expressions through the existing read-only validator with the connection's dialect); SQLite, DuckDB and Postgres only; a catalog row estimate above
max_rows_scanned(default 5,000,000) for a bare table refuses with a suggestion to narrow the population, and an unknown estimate is stated rather than silently proceeding; query timeouts returnskipped: timeoutinstead of failing the caller; all supplied SQL goes through thesql_executepermission request before workspace routing. Strict-schema clients'""/[]/0fillers are treated as absent. Function calls that can change state outside the read-only transaction are refused as classes rather than by name: everypg_*call is denied unless it is a known pure read (type/size/catalog inspection,pg_get_*, stat getters other thanpg_stat_reset*, LSN and snapshot reads, privilege checks), and everylo_*/lowrite/loread,dblink*,checkpoint/force_checkpoint, sequence,set_config, extension-load and file-write call is denied, so a new server-control, WAL, replication, statistics-reset or large-object function cannot slip through one name at a time. Was: a fixed list of individual function names → now: deny-by-default classes with a pure-read allowlist (eight pure reads are covered by positive tests).The analyst prompt's existing "every clause must bite" sentence now points at the check and says a vacuous filter is a reason to profile an alternative column, not to invent an exclusion.
Base. Stacked on
feat/analyst-harness-dab-2026-10(#1441); the diff shown here is only this feature.Behaviour deltas.
sql_checkaccepts one newcheckvalue and two new fields (base_sql,predicates, plusmax_rows_scanned); other checks and every other tool are unchanged. One sentence of the analyst prompt is extended. No other agent is affected.How did you verify your code works?
Unit/integration, real in-memory SQLite and DuckDB engines, run outside any sandbox:
test/altimate/sql-check-predicate-effect.test.ts(new): zero-removal and all-removal flags, NULL semantics, empty population, nine predicates rejected, a write inside a predicate rejected before execution, unsupported dialect, combined count equals a directWHERE p1 AND p2count, estimate guard (known/unknown/zero cap), strict-schema fillers, view shadowing, mixed-case schema.test/altimate/sql-check-postgres.test.ts(new): Postgres probe shape,to_regclass/reltuplesguard, unknown-estimate path (driver mocked; no live Postgres).sql_executedenied → no query runs.Results: 86 pass / 0 fail across the four touched test files;
bun run typecheckclean;bun run script/upstream/analyze.ts --markers --base main --strictpasses (all edits in Altimate-owned files). An independent review of the first commit (approve-with-changes) drove the second commit's follow-ups.Not run: live Postgres; HTTP/browser journeys do not apply (tool-only change).
Screenshots / recordings
Not a UI change.
Checklist
🤖 Generated with Claude Code
Note
High Risk
Expands SQL execution and permission paths with catalog-driven validation and read-only transaction semantics; mistakes could block legitimate probes or still miss implicit server-side rewrites noted in the PR limitations.
Overview
Adds
sql_checkpredicate_effect: given a read-only population (base_sql) and up to eight boolean predicates, it runs one aggregate probe and reports rows kept/removed per filter, combined AND counts, and warnings for vacuous or all-removing predicates (NULL = not kept).Safety and drivers. Predicate/population SQL is validated with stricter read-only rules (deny-by-default side-effect
pg_*/dblink/lo_*classes, pure-function allowlist, Postgres catalog checks for custom operators/functions/casts). Allsql_checkwarehouse calls now usereadOnly: trueacross SQLite/DuckDB/Postgres; DuckDB/SQLite helpers wrap execution in rolled-back READ ONLY /query_only, and Postgres forwards bind parameters in read-only paths. Bare-table probes respectmax_rows_scanned(estimate skip, bounded materialization on local engines).Other. Permission
askevaluates every pattern before prompting so a later deny cannot be bypassed. Analyst prompt points analysts atpredicate_effectfor “filter removes nothing” cases.Reviewed by Cursor Bugbot for commit 388d088. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a
predicate_effectcheck tosql_checkso analysts can see what each filter predicate actually removes from a population.The check takes a read-only population (
base_sql) and up to eight boolean predicates, runs a single aggregate probe, and reports rows kept and removed per predicate plus the combined AND count. Vacuous or all-removing predicates are flagged, NULL counts as not kept, empty populations are reported as such, and blank predicate entries are reported as skipped in their original position.Details
FILTER/OVER/WITHIN GROUP/USINGclauses, and wildcard projections are accepted as such.sql_executedeny rule scoped to the population–predicate join matches before any SQL runs; every permission pattern is evaluated before any approval prompt, so a later explicit deny cannot be bypassed by approving an earlier pattern.query_onlypragma for SQLite; a rolled-back READ ONLY transaction for DuckDB, guarded with atxid_currentprobe so an active caller transaction is refused rather than aborted. DuckDB'squery()is denied because it executes the SQL in its string argument.max_rows_scanned(default 5,000,000) and returnskipped: "estimate"; unknown estimates are reported without failing the call. Postgres runs the catalog guard and aggregate in one statement, and SQLite and DuckDB run the estimate and aggregate on one population bounded tomax_rows_scanned + 1rows, so a concurrent peer cannot swap the relation between guard and scan.skipped: timeout, but rethrow when they carrystateLost: trueso the caller gets the lost-session disclosure.feat/analyst-harness-dab-2026-10base.Written for commit 388d088. Summary will update on new commits.
Known limitations
predicate_effectvalidates the population and predicate SQL textually and against the server catalog: function names (pg_proc,duckdb_functions()), operator names (pg_operator) and explicit cast targets (pg_type) must resolve to built-in definitions, and the probe itself is authorized as a whole before execution. It cannot see definitions the text does not name: an implicit cast selected by a user-defined operand type (a user-defined type with an implicit cast to a built-in type whose conversion function has side effects), or any server-side rewrite such as rules, policies or generated columns. UnderBEGIN READ ONLYthose can still run if the connected role can reach them. Mitigations: run the analyst role without access to such definitions, or do not enablepredicate_effecton databases that define user-defined types with implicit casts.