fix(cli): case-insensitive SQL integration ID matching (#325) by muunkky · Pull Request #401 · deepnote/deepnote

muunkky · 2026-06-10T05:51:23Z

Note

Maintainers — CI needs your approval to run. As a fork PR from a first-time contributor, the GitHub Actions workflows are held in action_required until a maintainer clicks "Approve and run workflows." Reproduced locally on this branch: pnpm --filter @deepnote/cli test → 31/31 on the changed suites, tsc --noEmit clean, Biome clean on the changed files.

Closes #325

What this does

Makes SQL integration ID matching case-insensitive across the CLI — for both built-in and external integrations — so a sql_integration_id written in any casing is treated consistently.

Today the matching is case-sensitive in a few places, which causes three observable problems:

Built-in integrations escape the filter when mis-cased. BUILTIN_INTEGRATIONS holds canonical lowercase ids (deepnote-dataframe-sql, pandas-dataframe), checked with a raw Set.has. A block using Pandas-DataFrame isn't recognized as built-in, so it's collected as an external integration and deepnote lint flags it as missing.
The same external integration in two casings is treated as two. My-Warehouse and my-warehouse both map to the one env var SQL_MY_WAREHOUSE, yet are collected as two required ids, reported twice as missing/configured, and fetched twice.
Malformed metadata can crash analysis. metadata.sql_integration_id was cast as string; a non-string value would reach .toLowerCase() and throw.

The change

One coherent fix, three parts:

Built-in matching — a single isBuiltinIntegration(id) helper in constants.ts (BUILTIN_INTEGRATIONS.has(id.toLowerCase())), routed through by both call sites. Single source of truth; mirrors the existing lowercase convention in integrations/fetch-and-merge-integrations.ts.
External matching — collectRequiredIntegrationIds dedups case-insensitively via Map<lowercased, firstSeenOriginal> and returns the first-seen original casing; checkMissingIntegrations keys its configured/missing/usage collections by the lowercased id while displaying the first-seen casing. Same integration → collected once, reported once, fetched once.
Robustness — the unsafe as string cast becomes a typeof rawId === 'string' guard, so non-string metadata is ignored rather than crashing.

Tests

Developed test-first (vitest), behavior-asserting:

constants.test.ts — isBuiltinIntegration across canonical / upper / mixed casing; false for externals and suffix collisions.
collect-integrations.test.ts — mixed-case built-in not collected; external deduped to first-seen casing.
analysis.test.ts — non-string id doesn't crash and isn't flagged; missing-summary dedup.
Capstone (drives checkForIssues, the function deepnote lint calls): a project with Pandas-DataFrame (built-in), My-Warehouse and my-warehouse (one external, unset) → collectRequiredIntegrationIds returns exactly ["My-Warehouse"] and lint's missing summary is exactly ["My-Warehouse"].

31/31 across the three suites; tsc --noEmit clean; Biome clean on the changed files. Behavior is verified at the checkForIssues/collectRequiredIntegrationIds library boundary (what the lint command invokes); a CLI-binary end-to-end run additionally requires the toolkit analysis server and is out of scope for this change.

Scope

Six files, all under packages/cli/. No behavior change for ids already in consistent casing.

Summary by CodeRabbit

Bug Fixes
- SQL integration IDs are now deduplicated case-insensitively while preserving the first-seen casing in reports.
- Built-in integrations are recognized case-insensitively for improved consistency.
- Enhanced metadata validation to safely handle non-string integration ID values.
Tests
- Added comprehensive test coverage for case-insensitive integration ID handling and deduplication scenarios.

Built-in integration IDs were compared case-sensitively while external IDs were deduplicated and keyed on their raw casing, even though the downstream env-var derivation and API fetch are case-insensitive. As a result the same integration referenced in different casing surfaced as two required/missing entries and triggered redundant credential fetches. - Add isBuiltinIntegration() in constants.ts, comparing against the canonical lowercase BUILTIN_INTEGRATIONS set case-insensitively, and route both call sites through it. - Guard a non-string sql_integration_id in checkMissingIntegrations instead of casting, so a malformed value never reaches toLowerCase(). - Deduplicate external IDs case-insensitively in collectRequiredIntegrationIds and key the configured/missing/usage collections in checkMissingIntegrations by lowercased id, preserving the first-seen original casing as the single display representative. Closes the gap left by deepnote#325, which normalized only the built-in check.

coderabbitai · 2026-06-10T05:51:39Z

📝 Walkthrough

Walkthrough

This PR implements case-insensitive matching for built-in SQL integration IDs across the CLI. It introduces an isBuiltinIntegration() helper function that normalizes IDs to lowercase before checking membership, then applies this pattern in collection and analysis logic. External integration IDs are deduplicated and aggregated case-insensitively while preserving original casing for display. The change also makes metadata reading more defensive by type-checking sql_integration_id as a string before use.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Title clearly summarizes the main change: adding case-insensitive SQL integration ID matching to the CLI.
Linked Issues check	✅ Passed	All objectives from `#325` met: isBuiltinIntegration performs case-insensitive checks, external IDs deduplicated by normalized ID with original casing preserved, unsafe cast replaced with type guard.
Out of Scope Changes check	✅ Passed	All changes align with `#325` requirements: helper function addition, case-insensitive deduplication logic, type safety improvements, and comprehensive test coverage.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Updates Docs	✅ Passed	PR is a code-level bugfix (case-insensitive integration ID matching) with no new user-facing features requiring documentation updates. Only internal CLI logic and tests modified in packages/cli/src/.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

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

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

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/cli/src/integrations/collect-integrations.test.ts`:
- Around line 59-62: The test "capstone: collects a mixed-case external
integration once, first-seen casing" is duplicated; remove the redundant test
case to avoid duplicate assertions. Locate the test block that calls
makeFileWithSqlBlocks(['My-Warehouse', 'my-warehouse']) and asserts
collectRequiredIntegrationIds(file) equals ['My-Warehouse'] and delete the
duplicate so only one instance of this test remains (the assertion uses
collectRequiredIntegrationIds and helper makeFileWithSqlBlocks).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d556b71d-413c-40be-8100-56d9f9bcccbf

📥 Commits

Reviewing files that changed from the base of the PR and between d921be7 and 99e2757.

📒 Files selected for processing (6)

packages/cli/src/constants.test.ts
packages/cli/src/constants.ts
packages/cli/src/integrations/collect-integrations.test.ts
packages/cli/src/integrations/collect-integrations.ts
packages/cli/src/utils/analysis.test.ts
packages/cli/src/utils/analysis.ts

coderabbitai · 2026-06-10T05:57:14Z

+  it('capstone: collects a mixed-case external integration once, first-seen casing', () => {
+    const file = makeFileWithSqlBlocks(['My-Warehouse', 'my-warehouse'])
+    expect(collectRequiredIntegrationIds(file)).toEqual(['My-Warehouse'])
+  })


⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Duplicate test case.

This test is identical to lines 44-47. Consider removing one.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/integrations/collect-integrations.test.ts` around lines 59 - 62, The test "capstone: collects a mixed-case external integration once, first-seen casing" is duplicated; remove the redundant test case to avoid duplicate assertions. Locate the test block that calls makeFileWithSqlBlocks(['My-Warehouse', 'my-warehouse']) and asserts collectRequiredIntegrationIds(file) equals ['My-Warehouse'] and delete the duplicate so only one instance of this test remains (the assertion uses collectRequiredIntegrationIds and helper makeFileWithSqlBlocks).

…fix shipped as PR deepnote#401)

Only the interesting assets: roadmap (yaml + rendered html), the EXTIDCI1 sprint cards, and the executor/reviewer/router/dispatcher agent artifacts. Scaffold (templates, hooks, configs) trimmed. Companion to deepnote#401 - not for merge.

muunkky requested a review from a team as a code owner June 10, 2026 05:51

coderabbitai Bot requested changes Jun 10, 2026

View reviewed changes

muunkky added a commit to muunkky/deepnote that referenced this pull request Jun 10, 2026

chore(gitban): close out sprint EXTIDCI1 board (complete deepnote#325 …

07ffb18

…fix shipped as PR deepnote#401)

muunkky mentioned this pull request Jun 10, 2026

[showcase] the gitban harness behind #401 (not for merge) #402

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): case-insensitive SQL integration ID matching (#325)#401

fix(cli): case-insensitive SQL integration ID matching (#325)#401
muunkky wants to merge 1 commit into
deepnote:mainfrom
muunkky:fix/case-insensitive-integration-ids

muunkky commented Jun 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 10, 2026 •

edited

Loading

Walkthrough

Estimated code review effort

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jun 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

muunkky commented Jun 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

The change

Tests

Scope

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Walkthrough

Estimated code review effort

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

muunkky commented Jun 10, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 10, 2026 •

edited

Loading