Skip to content

Avoid SQLite warning for CLI version output#138

Merged
lzehrung merged 3 commits into
mainfrom
fix/lazy-sqlite-cli-version
Jun 22, 2026
Merged

Avoid SQLite warning for CLI version output#138
lzehrung merged 3 commits into
mainfrom
fix/lazy-sqlite-cli-version

Conversation

@lzehrung

Copy link
Copy Markdown
Owner

Summary

  • lazy-load Node's sqlite runtime inside the SQLite driver instead of importing it at module load
  • keep sqlite types as type-only imports
  • expand CLI regression coverage for version output

Verification

  • npm run test:run -- tests/cli-regressions.test.ts tests/sqlite-common.test.ts
  • node --trace-warnings ./dist/cli.js -v
  • npx eslint src/sqlite-driver.ts tests/cli-regressions.test.ts

@kilo-code-bot

kilo-code-bot Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • src/sqlite-driver.ts - explicit inline SqliteConstants and SqliteDatabaseConstructor types replace the previous typeof approach on type-only imports, resolving the TypeScript type system concern while preserving lazy loading via requireNodeModule
  • tests/cli-regressions.test.ts - string-based assertions replaced with regex matching for robustness; previous brittleness concern addressed
Previous Review Summaries (2 snapshots, latest commit 5423e4e)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 5423e4e)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • src/sqlite-driver.ts - the typeof NodeSqlite issue flagged by Copilot has been resolved; the new approach uses direct named type imports (import type { constants as sqliteConstants, ... }) and an explicit NodeSqliteModule interface, eliminating the type-only namespace import problem while preserving the lazy-load pattern
  • tests/cli-regressions.test.ts - unchanged; no issues carried forward

Previous review (commit 7dcc347)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • src/sqlite-driver.ts - lazy-loads node:sqlite via createRequire with proper type-only imports; StatementSync type is available in @types/node@^24.7.2; no runtime errors or breaking changes
  • tests/cli-regressions.test.ts - test assertions correctly verify the absence of static value imports and presence of the lazy-load pattern

Reviewed by kimi-k2.6-20260420 · Input: 55.2K · Output: 13.9K · Cached: 116.1K

@kilo-code-bot

kilo-code-bot Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • src/sqlite-driver.ts - lazy-loads node:sqlite via createRequire with proper type-only imports; StatementSync type is available in @types/node@^24.7.2; no runtime errors or breaking changes
  • tests/cli-regressions.test.ts - test assertions correctly verify the absence of static value imports and presence of the lazy-load pattern

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to prevent node:sqlite from being loaded (and emitting warnings) when the CLI is invoked only for version output (-v/--version/version), by moving the node:sqlite load behind a lazy runtime boundary and extending regression coverage to lock the behavior in.

Changes:

  • Refactors the SQLite driver to lazy-load the node:sqlite runtime (while keeping SQLite symbols as type-only imports).
  • Replaces the static read-only authorizer action set with a constants-driven predicate evaluated after loading SQLite.
  • Expands CLI regression tests to assert that version output does not statically load SQLite.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/sqlite-driver.ts Lazily loads node:sqlite at runtime and avoids touching SQLite constants during module initialization.
tests/cli-regressions.test.ts Adds a regression assertion that version output does not statically load SQLite (including the SQLite driver’s runtime import path).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/sqlite-driver.ts Outdated
@lzehrung

Copy link
Copy Markdown
Owner Author

Addressed Copilot feedback in 5423e4e: replaced the type-only namespace import with explicit type-only imports and named module/constant types. Verified with npm run build, targeted Vitest suites, ESLint, and node --trace-warnings ./dist/cli.js -v.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread src/sqlite-driver.ts Outdated
Comment on lines +17 to +21
type SqliteConstants = typeof sqliteConstants;
type NodeSqliteModule = {
DatabaseSync: typeof DatabaseSync;
constants: SqliteConstants;
};
Comment thread tests/cli-regressions.test.ts Outdated
Comment on lines +139 to +140
expect(sqliteDriverSource).not.toContain('import { constants, DatabaseSync');
expect(sqliteDriverSource).toContain('requireNodeModule("node:sqlite")');
@lzehrung

Copy link
Copy Markdown
Owner Author

Addressed the new comments in 2c598de:

  • removed type queries over type-only imports by replacing them with named structural types for the SQLite module, constants, and constructor
  • changed the regression assertions to regexes so they catch value imports regardless of specifier ordering/formatting and tolerate lazy-load formatting changes

Verified:

  • npm run build
  • npx eslint src/sqlite-driver.ts tests/cli-regressions.test.ts
  • npm run test:run -- tests/cli-regressions.test.ts tests/sqlite-common.test.ts
  • node --trace-warnings ./dist/cli.js -v

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@lzehrung lzehrung merged commit 3134a67 into main Jun 22, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants