1.23 beta2 - #883
Merged
Merged
Conversation
…) injection point (#879) ## What A single, additive hook on the Node client: `createClient({ connection })` lets a caller plug an externally-built `Connection<Stream.Readable>` into `createClient` instead of letting the package build its default HTTP connection. ```ts import { createClient } from '@clickhouse/client'; import { createChdbConnection } from 'chdb/connection'; const client = createClient({ connection: createChdbConnection({ path: ':memory:' }), }); ``` `client.query` / `insert` / `command` / `exec` / `ping` / `close` then route through the injected `Connection`'s existing contract. When `connection` is omitted (the default), behavior is byte-identical to today. Tracking issue: #865. Companion PR (the chdb-node side): **[chdb-io/chdb-node#52](chdb-io/chdb-node#52. ## Shape 3 lines of actual code: `make_connection` factory override when `connection` is provided, spread of `NodeConfigImpl` otherwise. No HTTP/HTTPS code touched. No new public types added. The new field is `@experimental` so we can iterate on shape without semver pressure. See `docs/design/pluggable-connection.md` for the full rationale and the asymmetric upstream-clean design (zero chdb code, zero chdb tests, zero chdb CI in this repo — all chdb-side logic lives in chdb-node). ## Why this shape Two design pressures, in tension: 1. **chDB's reason for existing is in-process zero-copy I/O.** Hiding chdb behind a loopback HTTP server (or any wire-format boundary) erases that. 2. **`@clickhouse/client`'s public surface is intentionally slim.** Bolting a second client family onto it — one mirroring the entire public API for chdb — is a maintenance trap. The compromise: - One public client API stays `createClient` from `@clickhouse/client`. - The `Connection<Stream>` interface behind it is unchanged — only injectable. - The backend implementation lives in the backend's repo. chdb-node ships `ChdbConnection` from `chdb/connection`; all chdb-side data, skip list, CI matrix that runs **this** suite against ChdbConnection stays in chdb-node's repo. ## Verification (chdb-node side) The companion chdb-node PR carries `tests/clickhouse-js/runner.mjs`, which clones this repo's integration suite and runs all 232 tests against an in-process ChdbConnection. **Currently 202 / 232 pass.** The 30 skips are documented in chdb-node's `skip_list.json` with reasons — every one is either an HTTP-only behavior (no socket in-process) or a chdb engine gap (default timezone, etc.). This is the first real-world consumer of the injection point; if shape needs to change based on what we learn here, the `@experimental` tag is the room to do that without semver pressure. ## Backwards compatibility Default code path (`connection` omitted) is unchanged — same `NodeConfigImpl`, same `make_connection`, byte-identical behavior. No existing user of `createClient` sees a difference. ## Test plan - [x] Existing test suite green (no behavior change when `connection` is omitted) - [x] TypeScript check clean - [x] End-to-end verification via chdb-node companion PR: 202 / 232 integration tests pass against `ChdbConnection` Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Happy <yesreply@happy.engineering>
7-day Dependabot cooldown for npm and github-actions. key is `cooldown.default-days`, not `minimum-release-age` (that's Renovate's), Dependabot silently ignores unknown keys: https://docs.github.com/en/code-security/dependabot/working-with-dependabot/dependabot-options-reference#cooldown--
fails PRs that add `package-lock.json` entries published less than 7
days ago. complements the Dependabot cooldown by catching anything that
bypasses it (manual edits, `--package-lock-only`, escape flags), audits
the lockfile diff so it isn't tool-specific.
set-diff against `git merge-base origin/main HEAD`, not `+` lines, so
lockfile reorders don't false-positive. existing lockfile grandfathered.
~1 registry request per added package, concurrency 8. `@clickhouse/*`
excluded.
escape hatch is the `lockfile-age-skip` label, for legit CVE bumps
inside the window.
failure looks like:
```
✗ 1 entries younger than 7 days:
foo@1.2.3
published: 2026-XX-XXT14:00:00Z (2 days ago)
mergeable after: 2026-XX-XXT14:00:00Z
```
advisory only until repo admin adds `Lockfile age audit / audit` as a
required check on main. will follow up once this lands.
companion: #881.
---------
Co-authored-by: Peter Leonov <peter.leonov@clickhouse.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…w-up to #879) (#880) ## What this does A small follow-up to #879. It dials the pluggable `connection` option back from a *public plugin surface* to a **deliberately narrow, internal `@experimental` hook** whose only intended consumer today is the chDB integration. Concretely: - **Removes the public re-exports** of the `Connection<Stream>` contract (`Connection`, `ConnBaseQueryParams`, `ConnQueryResult`, `ConnInsertParams`, `ConnPingResult`, `ClickHouseSummary`, …) from the `@clickhouse/client` (Node) entrypoint. - **Deletes `docs/design/pluggable-connection.md`**, which described a fuller pluggable-backend system we are intentionally not committing to yet. - **Trims the `CHANGELOG` and `connection?` JSDoc** to state plainly that the field exists to unblock chDB and may change. - **Moves the rationale into the `connection?` JSDoc itself**, so it lives right where anyone touching the field will read it. The runtime behavior is unchanged: the `make_connection` factory override still works exactly as merged in #879. This is purely a framing/surface-area change. ## Why Two pressures, gently in tension: 1. We genuinely want to **unblock chDB** ([chdb-io/chdb-node#52](chdb-io/chdb-node#52)) with an in-process backend, today. 2. We do **not** want to turn the Node client into the common package replacement — a public pluggable-backend framework implies a stable third-party contract and invites a second client family mirroring the entire public API, which is a real long-term maintenance burden. Keeping the `Connection` types out of the public entrypoint is what *enforces* this distinction. A backend has to deep-import or structurally match the shape — and that small friction is by design: it signals "unsupported, may change" rather than promising an API we are not ready to own. Nothing internal depends on the re-exports (internal code imports these types from `./common`), and `npm run typecheck` passes. ## Next steps (not in this PR) 1. **Verify it works and is actually needed in production** with the real chDB backend before hardening anything. 2. If validated, **build a specialized client** that drops the HTTP connection-pooling burden entirely and leans into the in-process transport — leaving the higher-level query API as the surface we invest in and improve, rather than retrofitting it onto the HTTP-shaped client. Feedback very welcome — this is intentionally the smallest step that unblocks chDB while keeping our options open. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds two security/maintenance-oriented capabilities to the repository: (1) a CI guard to prevent merging newly-published npm dependencies via package-lock.json, and (2) an experimental injection point in the Node.js client to allow swapping the default HTTP connection implementation with a pre-built backend connection (intended for chDB integration/testing).
Changes:
- Add a GitHub Actions workflow + Node script to audit newly introduced
package-lock.jsonresolutions against a minimum “package age” threshold (with an explicit skip label). - Add an
@experimentalconnection?: Connection<Stream.Readable>option to the Node client config and wirecreateClient()to use it instead of the default HTTP connection factory. - Add unit tests covering the
createClient({ connection })injection behavior and document the option inCHANGELOG.md.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/ci/lockfile-age-audit.mjs | Implements the lockfile “minimum publish age” audit by comparing merge-base vs HEAD resolutions and querying npm registry metadata. |
| .github/workflows/lockfile-age-audit.yml | Adds a PR workflow that runs the audit only when package-lock.json changes and supports a lockfile-age-skip bypass label. |
| .github/dependabot.yml | Introduces a 7-day Dependabot cooldown (and excludes @clickhouse/* in one update config) to align with the lockfile age gate intent. |
| packages/client-node/src/config.ts | Adds the @experimental connection option to Node client config types/docs. |
| packages/client-node/src/client.ts | Uses the injected connection by overriding make_connection when provided. |
| packages/client-node/tests/unit/node_create_client_with_connection.test.ts | Adds unit tests ensuring the default factory isn’t called and client methods route through the injected connection. |
| CHANGELOG.md | Documents the new experimental Node.js connection option. |
2 tasks
…rettify (#886) ## Summary Addresses CI failure and review feedback on the lockfile age gate (`scripts/ci/lockfile-age-audit.mjs`, introduced in #882). These changes are made in a fresh PR to `main` because #883 targets the `release` branch and cannot be modified. Two fixes, both in `scripts/ci/lockfile-age-audit.mjs`: 1. **Fail closed on non-registry sources** (Copilot review comment). Previously `extractNpmResolutions` silently `continue`d on any entry whose `resolved` host was not an allowed registry, so a PR could bypass the age gate entirely by pinning a new dependency from an alternative registry, a plain-`http` URL, or a `git+https` source. Now `isRegistryEntry` is replaced by `classifyResolved`: - `registry` — allowed HTTPS registry tarball → audited against the age gate (unchanged behavior). - `foreign` — a URL but not an allowed HTTPS registry host → **fails closed** (new). - `local` — no resolved URL (workspace source dirs, `file:` links) → skipped, nothing to age-check. The `lockfile-age-skip` PR label (handled in `.github/workflows/lockfile-age-audit.yml`) remains the intentional escape hatch. 2. **Prettier formatting** (Copilot review comment + the failing `code-quality` check). The file was written in single-quote / no-semicolon style; the repo's Prettier defaults (`.prettierrc` = `{}`) use double quotes + semicolons. Reformatted with `prettier --write`. ## Verification - `prettier --check` passes on the file (full-repo check is clean apart from an untracked local scratch dir). - `node --check` passes. - Manually exercised the fail-closed path against synthetic lockfiles: new deps resolved from an alternative host, `git+https`, and plain `http` all fail closed; unchanged legit registry entries are not flagged. ## Test plan - [x] `npm run prettier:check` clean for tracked files - [x] Fail-closed logic verified against synthetic base/head lockfiles 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A short description of the changes with a link to an open issue.
Checklist
Delete items not relevant to your PR: