refactor(databricks-devtools): replace MCP plugin with CLI-based skill - #511
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request deprecates the databricks-devtools MCP plugin by removing its complete implementation, configuration files, tests, and documentation. The using-databricks-explorer skill is refactored to describe executing SQL directly via Databricks CLI and Statements API instead of through MCP tools. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
skills/using-databricks-explorer/SKILL.md (1)
14-14:brew install databricksis macOS/Homebrew-only.Linux and Windows users require different installation paths. Consider noting the official installation docs link, or using a platform-neutral phrasing.
✏️ Suggested update
-- `databricks` CLI installed (`brew install databricks`) +- `databricks` CLI installed (see [Install the Databricks CLI](https://docs.databricks.com/dev-tools/cli/install.html))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/using-databricks-explorer/SKILL.md` at line 14, Replace the macOS-only installation hint "`databricks` CLI installed (`brew install databricks`)" in SKILL.md with a platform-neutral instruction: state that the Databricks CLI must be installed and link to the official installation docs (e.g., "See Databricks CLI installation guide") and/or list common install commands for macOS, Linux, and Windows (Homebrew, pip, Windows installer/winget) so non-mac users have guidance; update the single-line bullet that currently contains the brew command to this neutral phrasing and include the official docs URL.commands/databricks.explore.md (1)
3-3: Stale description language — "SQL explorer tools" now refers to CLI/API, not MCP tools.✏️ Suggested update
-description: Explore Databricks Unity Catalog schema with SQL explorer tools +description: Explore Databricks Unity Catalog schema via the Databricks CLI and Statements API🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/databricks.explore.md` at line 3, Update the stale description string in the frontmatter: replace "SQL explorer tools" with wording that reflects the CLI/API nature (e.g., "SQL explorer CLI/API" or "SQL explorer via CLI/API") so the description line (the description field) reads something like "Explore Databricks Unity Catalog schema with SQL explorer CLI/API" to accurately reflect current tooling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/using-databricks-explorer/SKILL.md`:
- Around line 43-56: The skill currently only parses immediate POST responses
and doesn't handle async responses when "wait_timeout" expires; update the
Executing SQL section to detect when the POST response has status.state ==
"RUNNING" or "PENDING" and then poll the GET endpoint
/api/2.0/sql/statements/{statement_id} (using the returned statement_id) in a
loop with a short sleep (e.g., 2s) until status.state becomes "SUCCEEDED" or
"FAILED", then parse columns from manifest.schema.columns[] and rows from
result.data_array or errors from status.error.message; ensure the documentation
shows the GET call and the polling logic and where to extract statement_id,
status.state, manifest.schema.columns[].name/type_name, result.data_array, and
status.error.message.
- Around line 27-34: Replace the inline code spans that attempt to escape
backticks with double-backtick-delimited code spans so inner backticks are
preserved (e.g., use ``SHOW SCHEMAS IN `catalog` `` instead of `SHOW SCHEMAS IN
\`catalog\``), and update the examples that use the shorthand c.s.t (DESCRIBE
TABLE c.s.t, DESCRIBE DETAIL c.s.t, SELECT * FROM c.s.t LIMIT 10) to
consistently backtick-quote each identifier level (e.g.,
`catalog`.`schema`.`table`) to obey the "Always backtick-quote identifiers"
rule.
---
Nitpick comments:
In `@commands/databricks.explore.md`:
- Line 3: Update the stale description string in the frontmatter: replace "SQL
explorer tools" with wording that reflects the CLI/API nature (e.g., "SQL
explorer CLI/API" or "SQL explorer via CLI/API") so the description line (the
description field) reads something like "Explore Databricks Unity Catalog schema
with SQL explorer CLI/API" to accurately reflect current tooling.
In `@skills/using-databricks-explorer/SKILL.md`:
- Line 14: Replace the macOS-only installation hint "`databricks` CLI installed
(`brew install databricks`)" in SKILL.md with a platform-neutral instruction:
state that the Databricks CLI must be installed and link to the official
installation docs (e.g., "See Databricks CLI installation guide") and/or list
common install commands for macOS, Linux, and Windows (Homebrew, pip, Windows
installer/winget) so non-mac users have guidance; update the single-line bullet
that currently contains the brew command to this neutral phrasing and include
the official docs URL.
| | User target | SQL | | ||
| | --- | --- | | ||
| | *(no target)* | `list_catalogs` | | ||
| | `<catalog>` | `list_schemas(catalog)` | | ||
| | `<catalog>.<schema>` | `list_tables(catalog, schema)` | | ||
| | `<catalog>.<schema>.<table>` | `describe_table(table)` → `table_metadata(table)` → `preview_data(table)` | | ||
| | *(no target)* | `SHOW CATALOGS` | | ||
| | `<catalog>` | `SHOW SCHEMAS IN \`catalog\`` | | ||
| | `<catalog>.<schema>` | `SHOW TABLES IN \`catalog\`.\`schema\`` | | ||
| | `<catalog>.<schema>.<table>` | `DESCRIBE TABLE c.s.t` → `DESCRIBE DETAIL c.s.t` → `SELECT * FROM c.s.t LIMIT 10` | | ||
|
|
||
| If the user explicitly asks for only one output (for example, columns only), call only the relevant table-level tool. | ||
| Always backtick-quote identifiers. |
There was a problem hiding this comment.
Backtick escape ``` inside code spans is a CommonMark violation (lines 30–31), and line 32 uses unquoted identifiers that contradict the "Always backtick-quote identifiers" rule.
Lines 30–31 — rendering bug: Per the CommonMark spec, "inside a backtick string, all backslashes are treated literally", so \`` does not escape a backtick inside a code span — it terminates the span early. For example, `` SHOW SCHEMAS IN `catalog``` renders as code span `` SHOW SCHEMAS IN \ `` followed by plain textcatalog``. The fix is to use double-backtick delimiters so the inner single backticks are treated as literal content.
Line 32 — quoting inconsistency: The c.s.t shorthand is unquoted, directly contradicting the "Always backtick-quote identifiers" instruction two lines below. Claude may use this table as a reference template and omit backtick quoting for multi-level targets.
✏️ Proposed fix for lines 30–32
-| `<catalog>` | `SHOW SCHEMAS IN \`catalog\`` |
-| `<catalog>.<schema>` | `SHOW TABLES IN \`catalog\`.\`schema\`` |
-| `<catalog>.<schema>.<table>` | `DESCRIBE TABLE c.s.t` → `DESCRIBE DETAIL c.s.t` → `SELECT * FROM c.s.t LIMIT 10` |
+| `<catalog>` | `` SHOW SCHEMAS IN `catalog` `` |
+| `<catalog>.<schema>` | `` SHOW TABLES IN `catalog`.`schema` `` |
+| `<catalog>.<schema>.<table>` | `` DESCRIBE TABLE `c`.`s`.`t` `` → `` DESCRIBE DETAIL `c`.`s`.`t` `` → `` SELECT * FROM `c`.`s`.`t` LIMIT 10 `` |As per coding guidelines, Markdown files should pass markdownlint validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/using-databricks-explorer/SKILL.md` around lines 27 - 34, Replace the
inline code spans that attempt to escape backticks with
double-backtick-delimited code spans so inner backticks are preserved (e.g., use
``SHOW SCHEMAS IN `catalog` `` instead of `SHOW SCHEMAS IN \`catalog\``), and
update the examples that use the shorthand c.s.t (DESCRIBE TABLE c.s.t, DESCRIBE
DETAIL c.s.t, SELECT * FROM c.s.t LIMIT 10) to consistently backtick-quote each
identifier level (e.g., `catalog`.`schema`.`table`) to obey the "Always
backtick-quote identifiers" rule.
| ```bash | ||
| databricks api post /api/2.0/sql/statements \ | ||
| --profile <profile> \ | ||
| --json '{ | ||
| "statement": "<SQL>", | ||
| "warehouse_id": "<warehouse_id>", | ||
| "wait_timeout": "30s" | ||
| }' | ||
| ``` | ||
|
|
||
| - Treat `/databricks:explore` as the command prefix only. | ||
| - Parse the remaining token as target shape: *(none)*, `<catalog>`, `<catalog>.<schema>`, or `<catalog>.<schema>.<table>`. | ||
| - Execute MCP tools directly from target shape; do not re-invoke slash command as a skill. | ||
| Parse result from: | ||
| - columns: `manifest.schema.columns[].name`, `manifest.schema.columns[].type_name` | ||
| - rows: `result.data_array` | ||
| - error: `status.error.message` when `status.state == "FAILED"` |
There was a problem hiding this comment.
No handling for the async case when wait_timeout expires.
By default, after 10 seconds, if the SQL statement has not yet finished executing through the warehouse, the Databricks SQL Statement Execution API returns only the SQL statement ID and its current status, instead of the statement's result. With wait_timeout: "30s", any query that takes longer than 30s — plausible on a cold warehouse for even a simple SELECT * LIMIT 10 — will return status.state == "RUNNING" or "PENDING" with no data. The skill gives Claude no guidance for this case.
If the POST call returns a pending state, the statement_id from the JSON response should be used to make a GET call to /api/2.0/sql/statements/{statement_id} in a loop, waiting between each call, until the status returns as succeeded or failed.
Consider adding a "Polling" subsection or a note under "Executing SQL":
If `status.state` is `"RUNNING"` or `"PENDING"` after the POST, poll until complete:
```bash
databricks api get /api/2.0/sql/statements/<statement_id> --profile <profile>Repeat with a short sleep (e.g. 2s) until status.state is "SUCCEEDED" or "FAILED".
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @skills/using-databricks-explorer/SKILL.md around lines 43 - 56, The skill
currently only parses immediate POST responses and doesn't handle async
responses when "wait_timeout" expires; update the Executing SQL section to
detect when the POST response has status.state == "RUNNING" or "PENDING" and
then poll the GET endpoint /api/2.0/sql/statements/{statement_id} (using the
returned statement_id) in a loop with a short sleep (e.g., 2s) until
status.state becomes "SUCCEEDED" or "FAILED", then parse columns from
manifest.schema.columns[] and rows from result.data_array or errors from
status.error.message; ensure the documentation shows the GET call and the
polling logic and where to extract statement_id, status.state,
manifest.schema.columns[].name/type_name, result.data_array, and
status.error.message.
</details>
<!-- fingerprinting:phantom:triton:churro -->
<!-- This is an auto-generated comment by CodeRabbit -->
1 participant
Summary
databricks-devtoolsMCP 플러그인(TypeScript 코드, 테스트, 빌드 설정 전체)을 제거하고 CLI 기반 skill로 대체skills/using-databricks-explorer/SKILL.md를 MCP tool 호출 대신databricksCLI + Statements API 직접 실행 방식으로 재작성commands/databricks.explore.mdslash command를 skill로 위임하도록 업데이트docs/plugin-migration/databricks-devtools-README.md제거Changes
plugins/databricks-devtools/(plugin.json, .mcp.json, README, bun.lock, vitest.config)src/databricks-devtools/(cli, config, sql, mcp TypeScript source)tests/databricks-devtools/(all test files)docs/plugin-migration/databricks-devtools-README.mdskills/using-databricks-explorer/SKILL.md— CLI-based rewritecommands/databricks.explore.md— delegates to skillHow it works now
Claude runs
databricksCLI commands directly via Bash:databricks warehouses list --output json --profile <name>databricks api post /api/2.0/sql/statements --json '...'All 6 previous MCP tools map to SQL statements (
SHOW CATALOGS,SHOW SCHEMAS,SHOW TABLES,DESCRIBE TABLE,DESCRIBE DETAIL,SELECT * LIMIT n).Summary by CodeRabbit
Refactor
Documentation