Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… turn. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mes them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…h tools off, and close the HTTP client when a transport fails to build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move a test's imports to the top of the file and assert the exact keys of the published environment. Check the Genie addresses against literal URLs so a wrong path constant fails. Share the Genie stub's expected values through conftest fixtures so the copies cannot drift. Rewrap the mint_token docstring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34bda71f96
ℹ️ 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".
| endpoint = await resolve_mcp_endpoint( | ||
| engine, method, vault.load(engine, name) or {}, connection_name=name | ||
| ) |
There was a problem hiding this comment.
Preserve Genie fields in the turn-key vault
Cloud turns pass a TurnKeyDataVault here, but its _fetch_uncached() still requires an access_token and only retains the existing OAuth allowlist plus _method; it drops workspace_url, client_id, _client_secret, and genie_space_id. Consequently this vault.load() can never provide the fields resolve_mcp_endpoint() requires, so a cloud Genie connection is either omitted before discovery or reported as incomplete and registers no tools. Extend the turn-key response handling and secure-key classification for the Genie service-principal fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Non-blocking: I think this doesn't apply for now: cowork-server refuses Genie in org mode, so cloud never has a Genie connection. Can you confirm and resolve?
| client = self._client | ||
| if client is not None: | ||
| return client |
There was a problem hiding this comment.
Reopen clients whose owner loop has stopped
In the cloud path, discover_mcp_tools() opens sessions inside asyncio.run(), whose shutdown cancels the still-waiting _owner task and closes the SDK client, while _client remains non-None. The first tool call later runs on the turn's event loop and this fast path returns that closed client, guaranteeing a failed attempt and consuming the sole retry merely to establish a usable session; any transient failure on the retried call is then surfaced without its intended retry. Treat a done/cancelled owner as a missing client and reconnect before returning it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Non-blocking: agree with this one. On the cloud path, the first tool call gets a dead client and uses up its only retry reconnecting. Could we treat a finished _owner task as "no client" in _acquire_client and reconnect right away? This affects HubSpot and Notion on cloud too.
martyna-mindsdb
left a comment
There was a problem hiding this comment.
Here are my comments:
- Blocking: The live test ran before the review fixes. Can you rerun it on the final code? Please include a couple of Genie questions plus one HubSpot or Notion chat, since
client.pychanged for all MCP connectors. - Non-blocking: The PR mentions Genie One dropping and reconnecting several times in one turn. Can we open a ticket for it, so it's tracked after merge?
- Non-blocking: The docstrings are quite long across the PR. Could you trim them to just the non-obvious parts?
- Non-blocking: The PR description reads like a commit log. Could you shorten it to why, what changed and how it was tested? Also, please put the Linear link on a
Linear:line.
Inline comments below.
| _MAX_SCHEMA_DEPTH = 20 | ||
|
|
||
| #: Rows a Genie query result may hand the model; the largest seen live was 12. | ||
| GENIE_RESULT_ROW_CAP = 200 |
There was a problem hiding this comment.
Non-blocking: this caps the number of rows but not their size, so a wide table could still be very large. Worth adding a size limit too?
| return token | ||
|
|
||
|
|
||
| async def wait_for_genie_answer( |
There was a problem hiding this comment.
Non-blocking: if the turn is cancelled during this wait, Genie keeps working on the question. Should we call genie_cancel_response on cancel?
| return None | ||
| host = canonical_workspace_host(workspace_url) | ||
| url = genie_mcp_url(host, fields.get("genie_space_id") or None) | ||
| token = await mint_token(host, client_id, client_secret) |
There was a problem hiding this comment.
Non-blocking: this runs on every turn with a Genie connection, even when the user doesn't ask about Databricks. A failure can add about 40 s to the turn. Fine for now, but worth keeping in mind.
| "notion-memory-search": "read", | ||
| "notion-get-file-download-urls": "read", | ||
| }, | ||
| # Genie One, seen live 2026-10-20: it answers questions over the workspace's |
There was a problem hiding this comment.
Non-blocking: can we drop the date from this comment? The reason is enough on its own.
https://linear.app/mindsdb/issue/ENG-3549
Why this matters
A Databricks connection can now ask Databricks Genie instead of running SQL from the scratchpad. Genie writes and runs the SQL inside Databricks under the service principal's Unity Catalog grants, and the agent gets a short answer plus the rows. Signing in uses a Databricks-managed service principal's client ID and secret, so it works for every desktop app user and on the self-hosted build, which has no browser sign-in. It ships to all desktop users once released. Building it also surfaced three bugs in the shared MCP client that hit HubSpot and Notion too: a 5 s HTTP timeout, a reconnect that crossed task scopes and could hang a turn for good, and a session that stayed dead after one failed reconnect.
What changed
*.cloud.databricks.com,*.azuredatabricks.netor*.gcp.databricks.com, with no port, path, user info or IP literal._client_secretnever reaches a published app./oidc/v1/token, scopeall-apis, no redirects, and no secret, token or body text in any error.genie-mcpjoinsmcpas an MCP method, the fourgenie_*tools are read-only, andgenie_askwaits for the answer inside the call and returns only status, final answer, query item ids and the deep link.DS_*variables listed for it._client_secretis redacted before the first Genie call too.Notes for the reviewer
Ships with
Checks
uv run pytest tests/core/mcp tests/test_scrubbing.py tests/test_datasources_prompt.py tests/test_publisher_state.py -quv run pytest -q(full suite, at 34bda71)databricks__genie_ask; secret masked in the connection API and absent from the agent's environment. Not yet rerun on the final head🤖 Generated with Claude Code