Skip to content

Disable AI SQL generation in embedded client#100290

Merged
alexey-milovidov merged 12 commits intomasterfrom
disable-ai-embedded-client
Apr 10, 2026
Merged

Disable AI SQL generation in embedded client#100290
alexey-milovidov merged 12 commits intomasterfrom
disable-ai-embedded-client

Conversation

@alexey-milovidov
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov commented Mar 21, 2026

The embedded client (used by SSH and WebSocket terminal protocols) runs inside the server process. The initAIProvider function reads API keys from environment variables (OPENAI_API_KEY, ANTHROPIC_API_KEY), which is a security concern since the server's environment should not be exposed to connected users via the ?? command.

Skip initAIProvider when isEmbeeddedClient returns true, both in interactive and non-interactive mode paths. The ?? command will print "AI SQL generator is not initialized" instead of accessing environment variables.

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Disable AI SQL generation (?? command) in the embedded client (SSH and WebSocket protocols) to prevent access to the server's environment variables.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

The embedded client (used by SSH and WebSocket protocols) runs
inside the server process. The AI provider initialization reads
API keys from environment variables, which is a security concern
since the server's environment should not be exposed to users.

Skip `initAIProvider` when `isEmbeeddedClient` returns true,
both in interactive and non-interactive mode paths.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Mar 21, 2026

Workflow [PR], commit [8c86dd9]

Summary:


AI Review

Summary

This PR disables initAIProvider in embedded client mode (SSH and WebSocket terminal protocols) in both interactive and non-interactive paths, preventing access to AI provider credentials from the server environment. The change is minimal, targeted, and consistent with existing embedded-mode restrictions in ClientBase. I found no correctness, safety, performance, or compatibility issues in the patch.

ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
PR metadata quality
Safe rollout
Compilation time
No large/binary files
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 21, 2026
@thevar1able thevar1able self-assigned this Mar 21, 2026
@alexey-milovidov alexey-milovidov added pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-improvement Pull request with some product improvements and removed pr-bugfix Pull request with bugfix, not backported by default labels Mar 21, 2026
@alexey-milovidov
Copy link
Copy Markdown
Member Author

#100299

@alexey-milovidov
Copy link
Copy Markdown
Member Author

#100378

@alexey-milovidov
Copy link
Copy Markdown
Member Author

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Apr 9, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.00% 84.10% +0.10%
Functions 90.90% 90.90% +0.00%
Branches 76.60% 76.50% -0.10%

Changed lines: 100.00% (11/11) | lost baseline coverage: 1 line(s) · Uncovered code

Full report · Diff report

@alexey-milovidov alexey-milovidov added this pull request to the merge queue Apr 10, 2026
Copy link
Copy Markdown
Member Author

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. It was not easy, but we made it!

@alexey-milovidov
Copy link
Copy Markdown
Member Author

Now please review #100277

Merged via the queue into master with commit f6c4a03 Apr 10, 2026
163 checks passed
@alexey-milovidov alexey-milovidov deleted the disable-ai-embedded-client branch April 10, 2026 01:37
@robot-ch-test-poll robot-ch-test-poll added pr-synced-to-cloud The PR is synced to the cloud repo pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR labels Apr 10, 2026
clickhouse-gh Bot added a commit that referenced this pull request Apr 10, 2026
Backport #100290 to 26.3: Disable AI SQL generation in embedded client
alexey-milovidov added a commit that referenced this pull request Apr 10, 2026
Backport #100290 to 26.1: Disable AI SQL generation in embedded client
alexey-milovidov added a commit that referenced this pull request Apr 10, 2026
Backport #100290 to 25.8: Disable AI SQL generation in embedded client
alexey-milovidov added a commit that referenced this pull request Apr 10, 2026
Backport #100290 to 26.2: Disable AI SQL generation in embedded client
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-improvement Pull request with some product improvements pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants