[ResponseOps] ES|QL query rule executor silently rewrites the saved query via parse+pretty-print by doakalexi · Pull Request #275231 · elastic/kibana

doakalexi · 2026-06-26T19:27:45Z

Summary

When we execute an ES|QL rule we call, appendLimitToQuery, which parses the query, and re-emits it with the limit via BasicPrettyPrinter from @elastic/esql. Using the pretty printer drops parentheses that change the AST shape, so calculations that depend on grouping with same-precedence binary operators (*// or +/-) are rewritten and evaluate to a different value than what the user authored.

This PR updates appendLimitToQuery to find the end of the actual query and strip off any comments that would interfere with appending the limit and returns the rest of the query as written with the new limit.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

To verify

Create a few different ES|QL rules. Use queries that include parentheses and order of operations calculations.
Let the rules run
Verify that that results of the rule queries match what is expected.

github-actions

The new token-based appendLimitToQuery approach is sound and matches the existing antlr4 token-channel pattern used in kbn-esql-language/.../lexer_scope.ts, and antlr4 is a runtime dependency so the import is safe. One inline finding: a leftover casing mismatch in a CSV generator test assertion that lines up with the still-failing CI Jest test.

Generated by Claude Reviewer for issue #275231 · 182.3 AIC · ⌖ 10.3 AIC · ⊞ 3.9K

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…akalexi/kibana into bug/appendLimitToQuery-rewrites-query

infra-vault-gh-plugin-prod · 2026-06-29T19:50:20Z

Pinging @elastic/response-ops (Team:ResponseOps)

stratoula · 2026-06-30T05:03:47Z

We recently did some changes and we include parenthesis at the AST so @bartoval will help here

bartoval · 2026-06-30T05:22:56Z

We recently did some changes and we include parenthesis at the AST so @bartoval will help here

This Pr should solve the parens problems elastic/esql-js#130. This fix should be active on Kibana soon. https://github.com/elastic/kibana/pull/275286/changes#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R143

doakalexi · 2026-06-30T14:21:40Z

Thanks @stratoula and @bartoval! I am wondering what you think about using BasicPrettyPrinter to remove comments from the query like we were doing before? Maybe it's not worth the risk of accidentally alerting the query? and this approach I have in the PR of just stripping off any comments at the end is maybe safer?

adcoelho

response-ops changes OK, just some tests

stratoula · 2026-06-30T14:42:02Z

Thanks @stratoula and @bartoval! I am wondering what you think about using BasicPrettyPrinter to remove comments from the query like we were doing before? Maybe it's not worth the risk of accidentally alerting the query? and this approach I have in the PR of just stripping off any comments at the end is maybe safer?

Sorry Alexi, I am not sure I follow 😅 (maybe because it is late). Can you elaborate?

doakalexi · 2026-06-30T15:53:14Z

Thanks @stratoula and @bartoval! I am wondering what you think about using BasicPrettyPrinter to remove comments from the query like we were doing before? Maybe it's not worth the risk of accidentally alerting the query? and this approach I have in the PR of just stripping off any comments at the end is maybe safer?

Sorry Alexi, I am not sure I follow 😅 (maybe because it is late). Can you elaborate?

That's my bad, sorry! I wanted to get your thoughts about keeping this PR open with this change to appendLimitToQuery. Even though @bartoval's change will fix the issue in BasicPrettyPrinter, I am wondering if you think the approach in this PR to move away from BasicPrettyPrinter in appendLimitToQuery is worth keeping to prevent other bugs like this from happening? I don't think it's super necessary to use the pretty printer if we can just find the end of the query and remove any trailing comments and append the limit?

I'm also wondering if change in @bartoval's PR would be able to backported? bc I think we will want to backport a fix for this as well

bartoval · 2026-06-30T17:56:32Z

Thanks @stratoula and @bartoval! I am wondering what you think about using BasicPrettyPrinter to remove comments from the query like we were doing before? Maybe it's not worth the risk of accidentally alerting the query? and this approach I have in the PR of just stripping off any comments at the end is maybe safer?

Sorry Alexi, I am not sure I follow 😅 (maybe because it is late). Can you elaborate?

That's my bad, sorry! I wanted to get your thoughts about keeping this PR open with this change to appendLimitToQuery. Even though @bartoval's change will fix the issue in BasicPrettyPrinter, I am wondering if you think the approach in this PR to move away from BasicPrettyPrinter in appendLimitToQuery is worth keeping to prevent other bugs like this from happening? I don't think it's super necessary to use the pretty printer if we can just find the end of the query and remove any trailing comments and append the limit?

I'm also wondering if change in @bartoval's PR would be able to backported? bc I think we will want to backport a fix for this as well

Hello @doakalexi .
If you want a raw query, Does just doing someting like

const trimmedQuery = queryString.trimEnd();
return `${trimmedQuery}\n| LIMIT ${limit}`;

cause you problems?

doakalexi · 2026-06-30T22:16:31Z

If you want a raw query, Does just doing someting like
const trimmedQuery = queryString.trimEnd();
return `${trimmedQuery}\n| LIMIT ${limit}`;
cause you problems?

Thanks @bartoval! It looks like ${trimmedQuery}\n| LIMIT ${limit} would work too. Do you think I should go ahead update to appendLimitToQuery with that suggestion?

bartoval · 2026-07-01T04:54:29Z

If you want a raw query, Does just doing someting like
const trimmedQuery = queryString.trimEnd();
return `${trimmedQuery}\n| LIMIT ${limit}`;
cause you problems?
Thanks @bartoval! It looks like ${trimmedQuery}\n| LIMIT ${limit} would work too. Do you think I should go ahead update to appendLimitToQuery with that suggestion?

Does it make sense to have this utils in this package? it seems we already have appendToESQLQuery in utils.ts
..or you can just do `${queryString.trimEnd()}\n| LIMIT ${limit}`; directly

I see from past commits that you implemented this function and all the consumers (or part of it) to use it.

doakalexi · 2026-07-01T14:08:22Z

Does it make sense to have this utils in this package? it seems we already have appendToESQLQuery in utils.ts
..or you can just do ${queryString.trimEnd()}\n| LIMIT ${limit}; directly

I think it makes sense to keep appendLimitToQuery, I think there a couple places that use the function already and it's nice to have a consistent way to append a limit. but I can remove it if you think it's not useful too

Thanks for the suggestion, I updated to use \n in this commit, 211df81

bartoval · 2026-07-02T05:03:38Z

Does it make sense to have this utils in this package? it seems we already have appendToESQLQuery in utils.ts
..or you can just do ${queryString.trimEnd()}\n| LIMIT ${limit}; directly

I think it makes sense to keep appendLimitToQuery, I think there a couple places that use the function already and it's nice to have a consistent way to append a limit. but I can remove it if you think it's not useful too

Thanks for the suggestion, I updated to use \n in this commit, 211df81

That works for me; I'll leave the approval to @stratoula .

In any case, I am restating my thoughts above-> This existing function https://github.com/bartoval/kibana/blob/main/src/platform/packages/shared/kbn-esql-utils/src/utils/append_to_query/utils.ts#L90 It works in a similar way.

Just change your 2 consumers to

query = appendToESQLQuery(this.job.query.esql, `| LIMIT ${maxRows}`);

query = appendToESQLQuery(this.job.query.esql, `| LIMIT ${alertLimit}`);

Then we can delete this file and the test

stratoula

The appendLimitToQuery is only used in 2 consumers. I will keep it thought to esql-utils:

it is a common thing to do and I want to change it with the composer api
I prefer to have one place to add the utils and not have multiple solutions across consumers

Alexi the solution is ok for now. Can you just remove the wrong test I point out?

stratoula · 2026-07-02T05:06:33Z

+    );
+  });
+
+  it('removes trailing block comments before appending the limit', () => {


This is wrong, it doesnt remove them. I dont think we need this test, can we remove this please?

Thanks @stratoula and @bartoval for your help on this! Test removed in this commit, 87df6b0

…akalexi/kibana into bug/appendLimitToQuery-rewrites-query

kibanamachine · 2026-07-02T16:11:47Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 6f1c326
Build duration: 55 mins

Failed CI Steps

Test Failures

[job] [logs] Jest Tests #12 / AlertFlyoutOverviewTab wraps the overview tab in KibanaContextProvider and ReactQueryClientProvider
[job] [logs] Jest Tests #8 / EntryItem displays DOES NOT MATCH select option as disabled if doesNotMatchDisabled=true
[job] [logs] Scout Lane #59 - serverless-observability_logs_essentials / uiam_local / local-serverless-observability_logs_essentials - [NON-MKI] UIAM API Keys grant and invalidate functions - should be able to grant a UIAM API key with valid UIAM credentials
[job] [logs] Scout Lane #64 - serverless-security_complete / default / local-serverless-security_complete - Saved Objects Tagging - usage collector - reports correct tag usage counts via the telemetry endpoint
[job] [logs] Scout Lane #4 - stateful-classic / default / local-stateful-classic - Discover tabs - opening a new tab - should create a new tab in classic mode
[job] [logs] Scout Lane #4 - stateful-classic / default / local-stateful-classic - Metrics in Discover - Inspect - should open inspector and navigate through views and tabs
[job] [logs] Scout Lane #15 - stateful-classic / default / local-stateful-classic - Saved query menu — CRUD (Discover) - save, load, update, save-as-new, delete via the popover
[job] [logs] Scout Lane #13 - stateful-classic / default / local-stateful-classic - UptimeIntegrationDeprecation - returns true when non-managed synthetics policies exist

Metrics [docs]

✅ unchanged

History

💔 Build #462951 failed c1f281f
💔 Build #462370 failed 5739272
💔 Build #462213 failed 0c2ac35
💛 Build #461012 was flaky 15f20a0
💔 Build #460934 failed e49d9c0

kibanamachine · 2026-07-02T16:12:36Z

Starting backport for target branches: 9.4

https://github.com/elastic/kibana/actions/runs/28604635640

kibanamachine · 2026-07-02T16:23:25Z

💚 All backports created successfully

Status	Branch	Result
✅	9.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…aved query via parse+pretty-print (#275231) (#276032) # Backport This will backport the following commits from `main` to `9.4`: - [[ResponseOps] ES|QL query rule executor silently rewrites the saved query via parse+pretty-print (#275231)](#275231)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Alexi Doak <109488926+doakalexi@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…uery via parse+pretty-print (elastic#275231) Resolves elastic#273173 ## Summary When we execute an ES|QL rule we call, `appendLimitToQuery`, which parses the query, and re-emits it with the limit via BasicPrettyPrinter from @elastic/esql. Using the pretty printer drops parentheses that change the AST shape, so calculations that depend on grouping with same-precedence binary operators (*// or +/-) are rewritten and evaluate to a different value than what the user authored. This PR updates `appendLimitToQuery` to find the end of the actual query and strip off any comments that would interfere with appending the limit and returns the rest of the query as written with the new limit. ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### To verify 1. Create a few different ES|QL rules. Use queries that include parentheses and order of operations calculations. 2. Let the rules run 3. Verify that that results of the rule queries match what is expected. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

doakalexi added 2 commits June 26, 2026 12:25

Update appendLimitToQuery to just remove the trailing comments

d62ecbd

Fixing test failure

e49d9c0

doakalexi changed the title ~~Update appendLimitToQuery to just remove the trailing comments~~ [ResponseOps] ES|QL query rule executor silently rewrites the saved query via parse+pretty-print Jun 29, 2026

doakalexi added release_note:fix Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// backport:version Backport to applied version labels v9.5.0 v9.4.4 labels Jun 29, 2026

github-actions Bot reviewed Jun 29, 2026

View reviewed changes

Comment thread src/platform/packages/private/kbn-generate-csv/src/generate_csv_esql.test.ts Outdated

doakalexi and others added 4 commits June 29, 2026 10:08

Apply suggestion from @github-actions[bot]

d7ec459

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'main' into bug/appendLimitToQuery-rewrites-query

caf6a8b

Fixing test failure

245219a

Merge branch 'bug/appendLimitToQuery-rewrites-query' of github.com:do…

15f20a0

…akalexi/kibana into bug/appendLimitToQuery-rewrites-query

doakalexi marked this pull request as ready for review June 29, 2026 19:50

doakalexi requested review from a team as code owners June 29, 2026 19:50

adcoelho approved these changes Jun 30, 2026

View reviewed changes

doakalexi and others added 3 commits July 1, 2026 07:14

Updating the function

211df81

Updating tests

0c2ac35

Merge branch 'main' into bug/appendLimitToQuery-rewrites-query

5739272

stratoula reviewed Jul 2, 2026

View reviewed changes

doakalexi and others added 3 commits July 2, 2026 07:45

Removing tests

87df6b0

Merge branch 'bug/appendLimitToQuery-rewrites-query' of github.com:do…

c1f281f

…akalexi/kibana into bug/appendLimitToQuery-rewrites-query

Merge branch 'main' into bug/appendLimitToQuery-rewrites-query

6f1c326

stratoula approved these changes Jul 2, 2026

View reviewed changes

doakalexi enabled auto-merge (squash) July 2, 2026 16:03

doakalexi merged commit 89d065f into elastic:main Jul 2, 2026
39 checks passed

kibanamachine mentioned this pull request Jul 2, 2026

[9.4] [ResponseOps] ES|QL query rule executor silently rewrites the saved query via parse+pretty-print (#275231) #276032

Merged

kibanamachine added the v9.4.3 label Jul 2, 2026

kibanamachine removed the v9.4.3 label Jul 2, 2026

Uh oh!

Conversation

doakalexi commented Jun 26, 2026 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

To verify

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

infra-vault-gh-plugin-prod Bot commented Jun 29, 2026

Uh oh!

stratoula commented Jun 30, 2026

Uh oh!

bartoval commented Jun 30, 2026

Uh oh!

doakalexi commented Jun 30, 2026

Uh oh!

adcoelho left a comment

Choose a reason for hiding this comment

Uh oh!

stratoula commented Jun 30, 2026

Uh oh!

doakalexi commented Jun 30, 2026

Uh oh!

bartoval commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doakalexi commented Jun 30, 2026

Uh oh!

bartoval commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doakalexi commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bartoval commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stratoula left a comment

Choose a reason for hiding this comment

Uh oh!

stratoula Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

doakalexi Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 2, 2026

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

Uh oh!

Uh oh!

kibanamachine commented Jul 2, 2026

Uh oh!

kibanamachine commented Jul 2, 2026

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

doakalexi commented Jun 26, 2026 •

edited by kibanamachine

Loading

bartoval commented Jun 30, 2026 •

edited

Loading

bartoval commented Jul 1, 2026 •

edited

Loading

doakalexi commented Jul 1, 2026 •

edited

Loading

bartoval commented Jul 2, 2026 •

edited

Loading