[Makisuo]

Two run_sql failures where the work was wasted rather than the SQL wrong.

What changed

• Trace-id lookups without a time bound. A query that reads only traces with a literal TraceId = '<id>' / TraceId IN (...) and no time bound probes every daily partition. In 14 days the 30 external lookups like this had a p95 of 11.2 s, against 1.07 s for the 500 bounded ones. On the internal agent, 30 of its 31 budget failures on trace lookups had no time filter.
  • run_sql now ANDs onto that conjunct the same trace_index seek that inspect_trace uses: (ServiceName, toStartOfFiveMinutes(Timestamp), TraceId) IN (SELECT ... FROM trace_index ...). Because it is ANDed onto the trace-id predicate itself, it can only select rows the predicate already selects.
  • The seek is skipped when:
    • the SQL fails the org-filter gate on its own (the seek's subquery brings its own $__orgFilter);
    • the predicate sits inside if(), NOT or an OR;
    • an alias shadows ServiceName, Timestamp or TraceId;
    • the query reads any other table or already has a time bound.
  • If the warehouse cannot analyse the seeked form (for example the predicate is in an outer query over a subquery), the query runs as written. Any other failure is reported as it is, so a timeout never runs twice.
• Row cap. A result over 1,000 rows failed after the query had already run (42 failures in 14 days), even though the description promised the count would be reported. run_sql now wraps the statement in LIMIT 1000, returns those rows, and says it stopped at the cap (atRowCap). Dashboard widgets and alerts keep the hard failure.

How it was checked

• Seek replay. Every unbounded trace-id lookup from the last 14 days' failures (36 distinct statements) ran against the production warehouse (Maple's own org), plain and seeked, in both orders:
  • Same rows for every query that ran both ways. Two differ only in the order of tied rows.
  • Faster: the median fell from 1.58 s to 1.04 s, from 0.93 s to 0.72 s, and on the final rules from 0.92 s to 0.51 s, faster in 20 to 26 of 31 to 32 queries.
  • One query that hit the 10 s limit plainly finished in 1.9 s seeked.
  • The large tenants where these timed out in production aren't reachable from here, so their gain is not measured.
• Index coverage. trace_index held all 50 sampled traces at 1 hour, 20 days and 29 days old (the two tables share a 30-day TTL).
• Tests.
  • trace-seek.test.ts: rewrite shape and every skip rule.
  • run-sql-recovery.test.ts: the row cap, the issued LIMIT 1000, and the fallback when the seeked form cannot be analysed.
  • Scoped typecheck for apps/ai and packages/query-engine, vitest for the query-engine runtime and the apps/ai MCP suites, and bun run lint:core.

Stacked on #1959.

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5ccadcec-2288-41e6-9afa-a1492094e9c6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[maple-review-bot]

Maple review

🟢 Confidence 8/10 · likely safe to merge
Held at 8 because a warning is open.
_{1 warning −1 · tests covered · risk medium · 1/1 new units observable}

run_sql now ANDs a trace_index seek onto unbounded literal trace-id lookups (with a fallback to the query as written) and returns 1,000 rows at the cap instead of failing. The rewrite is well guarded; the new cap flag misfires on results of exactly 1,000 rows.

• withTraceIndexSeek ANDs a trace_index tuple seek onto literal TraceId predicates
• run_sql reruns the query as written when the seeked form is invalid SQL
• prepareRawSql({ truncateAtCap }) emits LIMIT 1000 and the tool returns those rows
• RunSqlOutput.atRowCap reports the cap instead of the hard failure

Findings

🟠 Warning · F1 · atRowCap fires on results of exactly 1,000 rows that were not cut

correctness · apps/ai/src/mcp/tools/run-sql.ts:411

prepareRawSql now emits LIMIT 1000 for run_sql (packages/query-engine/src/runtime/raw-sql.ts:158), which drops the extra row that used to be the overflow sentinel (RAW_SQL_FETCH_ROW_LIMIT = MAX_RAW_SQL_RESULT_ROWS + 1, "one extra row is the overflow sentinel for the public 1,000-row cap"). With the sentinel gone, rowCount >= MAX_RAW_SQL_RESULT_ROWS cannot tell a truncated result from one that happened to return exactly 1,000 rows, so those queries get the "Stopped at the 1000-row cap, so more rows may exist" notice and a false atRowCap in structuredContent, and the agent spends a round trip re-aggregating.

Keep the sentinel: have `prepareRawSql` keep `RAW_SQL_FETCH_ROW_LIMIT` when `truncateAtCap` is set, skip only the rows branch of `rawSqlResultLimitError` in that case, and truncate `rows` to `MAX_RAW_SQL_RESULT_ROWS` here so `atRowCap` can be `rowCount > MAX_RAW_SQL_RESULT_ROWS`.

🤖 Prompt to fix this finding with an AI agent

Findings from an automated review of commit 0c3d53679e99492911273d72b9a93d2a32089000. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F1 · Warning · correctness · apps/ai/src/mcp/tools/run-sql.ts:411
`atRowCap` fires on results of exactly 1,000 rows that were not cut
`prepareRawSql` now emits `LIMIT 1000` for `run_sql` (packages/query-engine/src/runtime/raw-sql.ts:158), which drops the extra row that used to be the overflow sentinel (`RAW_SQL_FETCH_ROW_LIMIT = MAX_RAW_SQL_RESULT_ROWS + 1`, "one extra row is the overflow sentinel for the public 1,000-row cap"). With the sentinel gone, `rowCount >= MAX_RAW_SQL_RESULT_ROWS` cannot tell a truncated result from one that happened to return exactly 1,000 rows, so those queries get the "Stopped at the 1000-row cap, so more rows may exist" notice and a false `atRowCap` in `structuredContent`, and the agent spends a round trip re-aggregating.
Suggested fix: Keep the sentinel: have `prepareRawSql` keep `RAW_SQL_FETCH_ROW_LIMIT` when `truncateAtCap` is set, skip only the rows branch of `rawSqlResultLimitError` in that case, and truncate `rows` to `MAX_RAW_SQL_RESULT_ROWS` here so `atRowCap` can be `rowCount > MAX_RAW_SQL_RESULT_ROWS`.

What was checked

• Seeked SQL still passes rawSqlIssue: the added subquery's $__orgFilter sits after a WHERE, so orgFilterEscapes returns false (raw-sql.ts:511)
• Protocol cap uses rows > maxRows, so 1,000 rows pass maxRows: 1000 (lib/effect-clickhouse-http/src/protocol.ts:99)
• Skip rules verified against definedNames (only AS x aliases) and referencedTableNames (one entry per catalog table)

Observability coverage: 1 of 1 changes observable

Change	Kind	Observable	Evidence
run_sql trace-id seek and fallback rerun	warehouse query path	yes	Both statements go through runRawSql → WarehouseQueryService.rawSqlQuery with context: "mcp.run_sql" (apps/ai/src/mcp/lib/run-raw-sql.ts:65), under the dispatcher's maple.mcp.tool span

_{0c3d536 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[maple-review-bot]

1 inline note from Maple's review. The score and summary are in the review comment above.

[maple-review-bot]

Maple review

🟢 Confidence 10/10 · safe to merge
The seek only ANDs a proven trace_index tuple onto an existing predicate, and every skip rule, the row cap and the invalid-SQL fallback have tests at this head.
_{no findings · tests covered · risk medium}

run_sql now ANDs a trace_index seek onto unbounded literal TraceId lookups and stops at 1,000 rows instead of failing. The seek can only narrow, the fallback covers the forms ClickHouse rejects, and both are tested.

• withTraceIndexSeek ANDs a trace_index seek onto an unbounded trace-id predicate
• Seek is skipped for other tables, time bounds, OR/NOT/if(), and shadowing aliases
• run_sql falls back to the query as written on WarehouseInvalidSqlError
• truncateAtCap returns the first 1,000 rows with atRowCap instead of failing

Fixed since the last review

• ✅ F1 · atRowCap fires on results of exactly 1,000 rows that were not cut

What was checked

• Seek copies inTraces' tuple from ch/tables.ts:142, and #trace_index_mv indexes every span unfiltered (materializations.ts:1160)
• The client errors only above maxRows 1,000 (packages/query-engine/src/protocol.ts), so a cap-sized page is returned
• Both $__orgFilter occurrences expand (ORG_FILTER_MACRO_RE is global) and pass orgFilterEscapes, so the seeked form passes the org gate

_{4a1deb0 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[maple-review-bot]

Maple review

🟢 Confidence 9/10 · safe to merge
The seek is a provable narrowing of the predicate and the row cap keeps the result limits; one edge case over the length gate and one untested wiring remain.
_{2 notes · tests partial −1 · risk medium · 1/1 new units observable}

run_sql now seeks unbounded TraceId lookups through trace_index and returns 1,000 rows with an atRowCap notice instead of failing. The rewrite can only narrow what the predicate already selects, so it is safe to merge.

• withTraceIndexSeek ANDs a trace_index seek onto unbounded TraceId predicates
• prepareRawSql emits LIMIT 1000 for run_sql through truncateAtCap
• run_sql reports atRowCap instead of failing past 1,000 rows

Findings

🔵 Note · F2 · Seek pushes a near-limit statement past MAX_RAW_SQL_LENGTH

correctness · apps/ai/src/mcp/lib/trace-seek.ts:56-57

rawSqlIssue rejects any statement longer than 32,768 characters (packages/domain/src/raw-sql.ts:633), and this file gates on the original SQL (line 36) before the ~180-character seek is appended. A statement already within 180 characters of the limit passes the gate, then the seeked form fails with RawSqlValidationError, which run-sql.ts:370 does not fall back on, so the agent is told its own valid SQL is too long.

Capture the `reduceRight` result, `return seeked.length > MAX_RAW_SQL_LENGTH ? undefined : seeked`, and import `MAX_RAW_SQL_LENGTH` from `@maple/domain/raw-sql`.

🔵 Note · F3 · Seek-fallback test passes whether or not the seek is issued

tests · apps/ai/src/mcp/__evals__/run-sql-recovery.test.ts:137

The fallback rule and the plain rule both match the same statement (sql.includes(FALLBACK_ID)), and the assertions only read the final rows, so the test stays green when withTraceIndexSeek is removed from run-sql.ts:364 — the headline rewrite would disappear with every test passing. Record every issued statement in the fake (as the product_events rule does) and assert the first one carried FROM trace_index.

🤖 Prompt to fix all 2 findings with an AI agent

Findings from an automated review of commit 32224d90bb957eb0c08329b669ef91ac36b94e3f. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F2 · Note · correctness · apps/ai/src/mcp/lib/trace-seek.ts:56-57
Seek pushes a near-limit statement past `MAX_RAW_SQL_LENGTH`
`rawSqlIssue` rejects any statement longer than 32,768 characters (`packages/domain/src/raw-sql.ts:633`), and this file gates on the original SQL (line 36) before the ~180-character seek is appended. A statement already within 180 characters of the limit passes the gate, then the seeked form fails with `RawSqlValidationError`, which `run-sql.ts:370` does not fall back on, so the agent is told its own valid SQL is too long.
Suggested fix: Capture the `reduceRight` result, `return seeked.length > MAX_RAW_SQL_LENGTH ? undefined : seeked`, and import `MAX_RAW_SQL_LENGTH` from `@maple/domain/raw-sql`.

---

F3 · Note · tests · apps/ai/src/mcp/__evals__/run-sql-recovery.test.ts:137
Seek-fallback test passes whether or not the seek is issued
The fallback rule and the plain rule both match the same statement (`sql.includes(FALLBACK_ID)`), and the assertions only read the final `rows`, so the test stays green when `withTraceIndexSeek` is removed from `run-sql.ts:364` — the headline rewrite would disappear with every test passing. Record every issued statement in the fake (as the `product_events` rule does) and assert the first one carried `FROM trace_index`.

What was checked

• Seek tuple is byte-identical to the production inTraces reader (packages/query-engine/src/ch/tables.ts:142)
• trace_index's MV has no row filter, so the seek stays a subset of the TraceId rows (materializations.ts:1160)
• truncateAtCap results still pass the 1,000-row and 5 MB caps (runtime/raw-sql.ts:163); atRowCap fires only at exactly 1,000

Observability coverage: 1 of 1 changes observable

Change	Kind	Observable	Evidence
run_sql second warehouse run when the seeked form fails	outbound DB call	yes	goes through WarehouseQueryService.rawSqlQuery and recordRawSqlAudit (apps/ai/src/mcp/lib/run-raw-sql.ts:57)

_{32224d9 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[Makisuo]

Both notes fixed in 6d4931f. F2: withTraceIndexSeek returns undefined when the seeked statement would exceed MAX_RAW_SQL_LENGTH, so the query runs as written (test added). F3: the fake warehouse records every statement, and the fallback test asserts the first lookup went through trace_index and the second did not; removing the seek from run-sql.ts now fails it (checked by mutation).

[maple-review-bot]

Maple review

🟢 Confidence 10/10 · safe to merge
Both earlier findings are fixed and pinned by tests; the seek's unchanged-result guarantee rests on trace_index covering every traces row, which only the author's replay measured.
_{no findings · tests covered · risk medium}

run_sql seeks unbounded trace-id lookups through trace_index and stops at a 1,000-row cap instead of failing after the query ran. Both earlier findings are fixed at this head and I found nothing new; safe to merge.

• withTraceIndexSeek ANDs a trace_index membership predicate onto unbounded TraceId lookups
• The seek is skipped unless the SQL is a single-table, unbounded, plain-conjunct traces read
• run_sql wraps the statement in LIMIT 1000 and reports atRowCap rather than failing
• A seeked statement over MAX_RAW_SQL_LENGTH runs as written

Fixed since the last review

• ✅ F2 · Seek pushes a near-limit statement past MAX_RAW_SQL_LENGTH
• ✅ F3 · Seek-fallback test passes whether or not the seek is issued

What was checked

• Seeked text is re-validated by prepareRawSql's rawSqlIssue before it reaches the warehouse (packages/query-engine/src/runtime/raw-sql.ts:97)
• truncateAtCap reaches prepareRawSql unchanged through makeExecuteRawSql (packages/query-engine/src/runtime/raw-sql.ts:210)
• trace_index is filled by trace_index_mv from traces with the same 30-day TTL (packages/domain/src/tinybird/materializations.ts:1153)

_{6d4931f · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[maple-review-bot]

Maple review

🟢 Confidence 8/10 · likely safe to merge
Held at 8 because a warning is open.
_{1 warning −1 · tests covered · risk medium · 1/1 new units observable}

Adds a trace_index seek to the tool's SQL when a trace-id filter has no time bound, and replaces the run_sql row-limit error with a capped result marked atRowCap. The seek logic is guarded and tested; one row-count edge case needs fixing before merge.

• withTraceIndexSeek ANDs a trace_index seek onto literal TraceId predicates in unbounded traces queries
• run_sql wraps the statement in LIMIT 1000 and returns rows with atRowCap instead of failing
• A seeked statement the warehouse cannot analyse is re-run as written on WarehouseInvalidSQL

Findings

🟠 Warning · F4 · limitedWithinCap reads the offset of LIMIT offset, count as the row count

correctness · apps/ai/src/mcp/tools/run-sql.ts:296-301

In ClickHouse's LIMIT offset, count form the count is the second number, but OWN_LIMIT captures the first, so SELECT ... LIMIT 5, 5000 yields own[1] = 5 and limitedWithinCap returns true. The result is then exactly 1,000 rows (atRowCap suppressed) and the model is told the truncated answer is complete — the failure this PR set out to remove. LIMIT 2000, 1000 fails the other way: 1,000 rows with nothing cut, and a spurious cap notice.

const OWN_LIMIT = /\bLIMIT\s+(?:(\d+)\s*,\s*(\d+)|(\d+)(?:\s+OFFSET\s+\d+)?)\s*;?\s*$/i

/** The statement ends in its own LIMIT of at most the cap, so a full page is its answer, not ours. */
const limitedWithinCap = (sql: string): boolean => {
	const own = OWN_LIMIT.exec(maskLiteralsAndComments(sql).trimEnd())
	const count = own === null ? undefined : Number(own[2] ?? own[3])
	return count !== undefined && count <= MAX_RAW_SQL_RESULT_ROWS
}

🤖 Prompt to fix this finding with an AI agent

Findings from an automated review of commit 33c8ed1dacb23ac7a909597ad9c9cf242b7b973b. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F4 · Warning · correctness · apps/ai/src/mcp/tools/run-sql.ts:296-301
`limitedWithinCap` reads the offset of `LIMIT offset, count` as the row count
In ClickHouse's `LIMIT offset, count` form the count is the second number, but `OWN_LIMIT` captures the first, so `SELECT ... LIMIT 5, 5000` yields `own[1] = 5` and `limitedWithinCap` returns true. The result is then exactly 1,000 rows (`atRowCap` suppressed) and the model is told the truncated answer is complete — the failure this PR set out to remove. `LIMIT 2000, 1000` fails the other way: 1,000 rows with nothing cut, and a spurious cap notice.
Replace those lines with:
const OWN_LIMIT = /\bLIMIT\s+(?:(\d+)\s*,\s*(\d+)|(\d+)(?:\s+OFFSET\s+\d+)?)\s*;?\s*$/i

/** The statement ends in its own LIMIT of at most the cap, so a full page is its answer, not ours. */
const limitedWithinCap = (sql: string): boolean => {
	const own = OWN_LIMIT.exec(maskLiteralsAndComments(sql).trimEnd())
	const count = own === null ? undefined : Number(own[2] ?? own[3])
	return count !== undefined && count <= MAX_RAW_SQL_RESULT_ROWS
}

What was checked

• maskLiteralsAndComments preserves offsets (raw-sql.ts:637), so the new splice-by-index logic lands on the right characters
• trace_index sort key (OrgId, TraceId, ServiceName, Bucket) serves the seek's TraceId IN (...) lookup, and traces is keyed on (OrgId, ServiceName, bucket, TraceId)
• rawSqlResultLimitError rejects only > the cap, so a truncated 1,000-row result passes; the seeked statement with a second $__orgFilter inside the subquery is issued by the eval test

Observability coverage: 1 of 1 changes observable

Change	Kind	Observable	Evidence
run_sql fallback re-run of the unseeked statement	outbound warehouse query	yes	goes through the same instrumented executeRawSql / rawSqlQuery path as the first attempt

_{33c8ed1 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[maple-review-bot]

1 inline note from Maple's review. The score and summary are in the review comment above.

[maple-review-bot]

Maple review

🟢 Confidence 10/10 · safe to merge
F4 is fixed by reading the second number of LIMIT offset, count, pinned by a test that fails on the old behaviour; no new defect found.
_{no findings · tests covered · risk medium · 2/2 new units observable}

run_sql now ANDs a trace_index seek onto unbounded TraceId predicates and truncates at the 1,000-row cap instead of failing, and the row-cap notice reads the count of LIMIT offset, count. Safe to merge.

• withTraceIndexSeek rewrites an unbounded, single-table TraceId conjunct into an index seek
• run_sql falls back to the query as written only on WarehouseInvalidSqlError
• truncateAtCap wraps the statement in LIMIT 1000 and reports atRowCap
• limitedWithinCap reads the count, not the offset, of LIMIT n, m

Fixed since the last review

• ✅ F4 · limitedWithinCap reads the offset of LIMIT offset, count as the row count

What was checked

• OWN_LIMIT run on 12 shapes in node: LIMIT 10, 5000 yields 5000, LIMIT 0, 5000 yields 5000, LIMIT 1000/OFFSET/trailing ; yield the count
• Seeked form passes rawSqlIssue: both $__orgFilter occurrences survive orgFilterEscapes and disallowedTableFunction reads the (SELECT after IN as a subquery
• trace_index is backfilled from traces for the full 30-day TTL (migrations/0042_trace_index.ts:13-21), so the seek cannot drop spans the bare predicate would return

Observability coverage: 2 of 2 changes observable

Change	Kind	Observable	Evidence
run_sql trace-id lookup rewritten to seek trace_index	outbound DB query	yes	Runs through the existing runRawSql → WarehouseQueryService.rawSqlQuery path (apps/ai/src/mcp/lib/run-raw-sql.ts:57), the same instrumented warehouse call every raw statement uses
run_sql row-cap truncation and its atRowCap notice	MCP tool response	yes	No new entrypoint or call: the same audited runRawSql call (recordRawSqlAudit, run-raw-sql.ts:68) with the wrapped statement

_{7548036 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[maple-review-bot]

Maple review

🟢 Confidence 10/10 · safe to merge
Seek only narrows an existing trace-id conjunct and falls back when unanalysable; sql-identifiers.ts hunks are not in this PR's diff, so unreviewed.
_{no findings · tests covered · risk medium · 1/1 new units observable}

run_sql now ANDs a trace_index seek onto unbounded literal trace-id predicates and stops at the 1,000-row cap instead of failing. The rewrite is strictly narrowing, skips every shape it cannot prove safe, and falls back to the SQL as written; safe to merge.

• withTraceIndexSeek ANDs a trace_index seek onto unbounded literal trace-id predicates
• run_sql reruns the SQL as written when the seeked form cannot be analysed
• prepareRawSql({ trunkAtCap }) caps run_sql results at 1,000 rows instead of failing
• atRowCap is suppressed when the query's own LIMIT is within the cap

Before merge

• Manual · Merge fix(ai): run_sql reruns misspelt column names and keeps internal SQL out of errors #1959 first: this branch imports sql-identifiers from that stacked base

What was checked

• Seek cannot widen or reorder results: it is inserted inside the existing conjunct, ids come from a strict hex-literal regex, and mixed/id-less lists skip (trace-seek.ts:41-57)
• Non-plain conjuncts (if(), NOT, OR), other tables, time bounds and shadowing aliases all skip the rewrite (trace-seek.ts:31-48)
• RAW_SQL_FETCH_ROW_LIMIT stays 1,001 for dashboards and alerts; only run_sql passes truncateAtCap (runtime/raw-sql.ts:158)

Observability coverage: 1 of 1 changes observable

Change	Kind	Observable	Evidence
run_sql trace-id seek rewrite and fallback rerun	SQL rewrite plus an extra warehouse query	yes	Each statement runs through the executor's outbound span (executor.ts:567-574), so a seeked run and its fallback appear as two statement spans

_{65468ed · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[maple-review-bot]

Maple review

🟢 Confidence 10/10 · safe to merge
Seek rewriting is guarded and every skip rule plus the row cap is pinned by tests; only the cap notice is heuristic, and the gain on large tenants is unmeasured.
_{1 note · tests covered · risk medium · 1/1 new units observable}

Rewrites run_sql's unbounded trace-id lookups to seek through trace_index and turns the 1,000-row hard failure into a truncated result with an atRowCap notice. The rewrite is conjunct-ANDed and conservative; safe to merge.

• withTraceIndexSeek ANDs a trace_index seek onto unbounded literal TraceId lookups
• run_sql re-runs the SQL as written when the seeked form fails as invalid SQL
• truncateAtCap wraps run_sql in LIMIT 1000 and sets atRowCap

Findings

🔵 Note · F5 · limitedWithinCap reads a subquery LIMIT as the statement's own cap

correctness · apps/ai/src/mcp/tools/run-sql.ts:300-302

OWN_LIMIT only matches a LIMIT at the very end of the statement, so it also reports false whenever the count came from a limit the trailing-clause check cannot see. SELECT * FROM (SELECT number FROM logs WHERE $__orgFilter ORDER BY Timestamp DESC LIMIT 1000) returns exactly 1,000 rows from its own inner LIMIT and still gets atRowCap: true, telling the agent more rows may exist and to add a LIMIT it already wrote.

Base the notice on the criteria that the result was actually cut, not on the shape of the statement's tail — e.g. keep the 1,001st overflow row for `run_sql` (fetch `RAW_SQL_FETCH_ROW_LIMIT`, drop the extra row, set `atRowCap` from its presence) and keep the `LIMIT 1000` wrapper for the rows returned.

🤖 Prompt to fix this finding with an AI agent

Findings from an automated review of commit 73e4b6b979d9b1faf5b6e4cef947eb37fe831953. Verify each one against the current code before changing anything, fix only those that still apply, and keep each fix to the lines it names.

---

F5 · Note · correctness · apps/ai/src/mcp/tools/run-sql.ts:300-302
`limitedWithinCap` reads a subquery `LIMIT` as the statement's own cap
`OWN_LIMIT` only matches a `LIMIT` at the very end of the statement, so it also reports `false` whenever the count came from a limit the trailing-clause check cannot see. `SELECT * FROM (SELECT number FROM logs WHERE $__orgFilter ORDER BY Timestamp DESC LIMIT 1000)` returns exactly 1,000 rows from its own inner `LIMIT` and still gets `atRowCap: true`, telling the agent more rows may exist and to add a `LIMIT` it already wrote.
Suggested fix: Base the notice on the criteria that the result was actually cut, not on the shape of the statement's tail — e.g. keep the 1,001st overflow row for `run_sql` (fetch `RAW_SQL_FETCH_ROW_LIMIT`, drop the extra row, set `atRowCap` from its presence) and keep the `LIMIT 1000` wrapper for the rows returned.

What was checked

• trace_index covers every span: trace_index_mv groups all of traces unfiltered, both 30-day TTLs
• The seeked SQL still passes rawSqlIssue: FROM trace_index is unrestricted and $__orgFilter expands globally
• LIMIT 1000 stays under the row check, which fails only above it (query-engine/src/runtime/raw-sql.ts:164)

Observability coverage: 1 of 1 changes observable

Change	Kind	Observable	Evidence
run_sql trace-id lookup (rewritten SQL)	warehouse query	yes	Still runs through runRawSql → WarehouseQueryService.rawSqlQuery on the existing traced raw-SQL path

_{73e4b6b · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}

[maple-review-bot]

Maple review

🟢 Confidence 10/10 · safe to merge
The sql-identifiers edits come from the base branch, and F5 cannot fire: only a statement-terminal LIMIT matches the end-anchored OWN_LIMIT.
_{no findings · tests covered · risk medium}

run_sql now seeks literal trace-id reads through trace_index and truncates at the 1,000-row cap with an atRowCap notice instead of failing. The seek is added only where it cannot change rows, and the cap notice stays truthful. Safe to merge.

• withTraceIndexSeek ANDs the trace_index seek onto an unbounded literal trace-id conjunct
• run_sql falls back to the SQL as written when the seeked form is refused as invalid SQL
• truncateAtCap makes the row-cap wrapper fetch 1,000 rows instead of 1,001
• atRowCap is reported unless the statement's own terminal LIMIT asked for a full page

Fixed since the last review

• ✅ F5 · limitedWithinCap reads a subquery LIMIT as the statement's own cap

What was checked

• Ran OWN_LIMIT in node: a nested LIMIT is always followed by ), so only a statement-terminal one matches
• The seek text matches inTraces (packages/query-engine/src/ch/tables.ts:142) over the same trace_index sort-key prefix
• The added $__orgFilter inside the seek subquery passes orgFilterIssue (its WHERE is its own scope), so the seeked form validates

_{c25d423 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.}