From c2e28b9015405bf7e067140a68c39bf3ade85bbc Mon Sep 17 00:00:00 2001 From: ShawnChen Date: Tue, 23 Jun 2026 16:04:33 +0800 Subject: [PATCH 1/5] =?UTF-8?q?feat(client-node):=20pluggable=20Connection?= =?UTF-8?q?=20=E2=80=94=20createClient({=20connection=20})=20injection=20p?= =?UTF-8?q?oint=20(#879)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What A single, additive hook on the Node client: `createClient({ connection })` lets a caller plug an externally-built `Connection` 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](https://github.com/chdb-io/chdb-node/pull/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` 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 Co-authored-by: Happy --- CHANGELOG.md | 2 + docs/design/pluggable-connection.md | 138 ++++++++++++ ...node_create_client_with_connection.test.ts | 204 ++++++++++++++++++ packages/client-node/src/client.ts | 14 +- packages/client-node/src/config.ts | 21 ++ packages/client-node/src/index.ts | 16 ++ 6 files changed, 394 insertions(+), 1 deletion(-) create mode 100644 docs/design/pluggable-connection.md create mode 100644 packages/client-node/__tests__/unit/node_create_client_with_connection.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 3140c1a67..6c17c8388 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ - (Node.js) Added a RowBinary reader library and agent skill under [`skills/clickhouse-js-node-rowbinary-parser`](./skills/clickhouse-js-node-rowbinary-parser). It ships type-specific, monomorphizable building blocks for decoding `RowBinary` / `RowBinaryWithNames` / `RowBinaryWithNamesAndTypes` streams (full-buffer and chunked), plus a skill that guides an agent to generate bespoke high-performance parsers from a query's column types. The skill is bundled into `@clickhouse/client` (registered in `agents.skills`) and is also published independently as the [`@clickhouse/rowbinary`](https://www.npmjs.com/package/@clickhouse/rowbinary) package. A matching RowBinary writer is planned. ([#864]) +- (Node.js, `@experimental`) Added an additive `connection?: Connection` option to `createClient` that lets a caller plug an externally-built backend `Connection` in place of the default HTTP(S) factory. When provided, the client routes every method (`query` / `insert` / `command` / `exec` / `ping` / `close`) through the injected connection's `Connection` implementation, and the HTTP-related options (`tls`, `keep_alive`, `http_agent`, `max_open_connections`, …) are ignored; when omitted, behavior is byte-identical to today. The implementation is a 3-line `make_connection` factory override on `NodeConfigImpl` — no new HTTP code, no public types added beyond the field itself. The `Connection` contract types (`Connection`, `ConnBaseQueryParams`, `ConnQueryResult`, `ConnInsertParams`, `ConnPingResult`, etc.) are now re-exported from the public `@clickhouse/client` entrypoint so third-party backends don't need to deep-import. Intended consumer is an embedded chDB backend ([`chdb-node`](https://github.com/chdb-io/chdb-node)) shipping `createChdbConnection` from `chdb/connection`; see [`docs/design/pluggable-connection.md`](./docs/design/pluggable-connection.md) for the full design and asymmetric upstream-clean rationale. The field is tagged `@experimental` so the shape can iterate based on what the first real backend learns. ([#879]) + # 1.22.0 ## New features diff --git a/docs/design/pluggable-connection.md b/docs/design/pluggable-connection.md new file mode 100644 index 000000000..6c3eb8028 --- /dev/null +++ b/docs/design/pluggable-connection.md @@ -0,0 +1,138 @@ +# Pluggable Connection — design + +> **Status**: minimal upstream hook implemented in this branch +> (`feat/pluggable-connection`). +> Tracking issue: [#865](https://github.com/ClickHouse/clickhouse-js/issues/865). +> Parallel Python proposal: [ClickHouse/clickhouse-connect#809](https://github.com/ClickHouse/clickhouse-connect/issues/809). +> chDB-side implementation: +> (the `chdb/connection` subpath export). + +## What this PR adds + +A single, additive hook on the `@clickhouse/client` Node client: + +```ts +createClient({ + connection: createChdbConnection({ path: ":memory:" }), +}); +``` + +`NodeClickHouseClientConfigOptions.connection?: Connection` +lets a caller plug an externally-built `Connection` into `createClient` +instead of letting the package build its default HTTP connection from +`tls` / `keep_alive` / `http_agent` / etc. options. The existing +`Connection` interface is unchanged — the backend implements +that exact contract and gets dropped in. + +When `connection` is omitted (the default), behavior is byte-identical +to today: the bundled HTTP factory is used. + +Implementation is a 3-line wrap of `NodeConfigImpl`: when `connection` +is provided, spread `NodeConfigImpl` and override only its +`make_connection` factory to return the injected connection verbatim; +otherwise pass `NodeConfigImpl` through unchanged. No HTTP/HTTPS code +is touched. + +## Why this shape + +Two design pressures, in tension, drove it: + +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: every new public + method, every parameter change, every streaming refactor would have + to be applied twice. + +The compromise: + +- **One public client API** stays `createClient` from `@clickhouse/client`. +- **The Connection interface** behind it is unchanged — we only + made it injectable. +- **The backend implementation lives in the backend's repo.** chdb-node + ships `ChdbConnection` from `chdb/connection`. All chdb-side data and + logic (the in-process memory model, the `.chdb` extension namespace + for chDB-specific escape hatches, the skip list of tests chdb does + not support, the CI matrix that runs THIS suite against + ChdbConnection) stays in chdb-node's repo. **This repo carries zero + chdb code, zero chdb tests, zero chdb CI dependency.** + +## Usage + +```ts +import { createClient } from "@clickhouse/client"; +import { createChdbConnection } from "chdb/connection"; + +const client = createClient({ + connection: createChdbConnection({ path: ":memory:" }), +}); + +// All downstream code is identical across connections. +const rs = await client.query({ + query: "SELECT * FROM numbers(5)", + format: "JSONEachRow", +}); +for await (const row of rs.stream()) { + /* ... */ +} +await client.insert({ table: "t", values: rows }); +await client.close(); +``` + +The `chdb/connection` subpath is exposed by chdb-node v3.1+; see its +README and design doc for the in-process memory model, the `.chdb` +extension namespace, and the streaming roadmap. + +## Test parity — who runs what, where + +This repo does **not** participate in chdb-vs-server test parity. +There are no chdb-specific markers, no chdb skip lists, no chdb test +runners here. + +Parity is owned by the backend's repo. chdb-node maintains: + +- A blacklist of integration tests that chdb does not support + (in chdb-node's repo, as `tests/clickhouse-js/skip_list.json`). +- A CI workflow that clones `@clickhouse/client` at a configured + ref, runs its integration suite with the chdb backend injected, + and applies the local blacklist. + +chdb-node tests against the **latest released version** of +`@clickhouse/client`. Only when `@clickhouse/client` cuts a new release +does chdb-node re-run its parity job and update its blacklist for any +newly-failing tests. + +## Data-vs-code split + +``` +┌──────────────────────────────────────┐ ┌──────────────────────────────────────┐ +│ @clickhouse/client (this repo) │ │ chdb-node (and any future backend) │ +│ │ │ │ +│ • Connection interface │ │ • ChdbConnection implements │ +│ (unchanged) │ │ Connection │ +│ • createClient({ connection }) │ ◄── │ • chdb/connection subpath export │ +│ new public injection point │ │ • tests/clickhouse-js/skip_list.json │ +│ │ │ (chdb-owned blacklist) │ +│ ❌ zero chdb code │ │ • CI clones THIS repo and runs its │ +│ ❌ zero chdb tests │ │ suite with skip_list applied │ +│ ❌ zero chdb CI dependency │ │ │ +└──────────────────────────────────────┘ └──────────────────────────────────────┘ +``` + +Adding a future backend (e.g. native TCP) repeats the same pattern in +the backend's own repo. No PR to this repo is needed. + +## What this PR deliberately does NOT do + +- **No chdb-specific code paths.** This repo stays backend-agnostic. +- **No dependency on `chdb`.** Not in any `package.json`. +- **No per-feature test markers.** Tests stay un-tagged. Backends own + their own blacklists in their own repos. +- **No skip-profile loader.** The injection point is the only hook; + what to skip is the backend's data, applied by the backend's + runner. +- **No client-web changes.** chdb is N-API and can only run in Node / + Bun / Deno; a future web-runtime backend repeats this pattern in + `client-web` if one ever lands. diff --git a/packages/client-node/__tests__/unit/node_create_client_with_connection.test.ts b/packages/client-node/__tests__/unit/node_create_client_with_connection.test.ts new file mode 100644 index 000000000..2a7039874 --- /dev/null +++ b/packages/client-node/__tests__/unit/node_create_client_with_connection.test.ts @@ -0,0 +1,204 @@ +/** + * Unit tests for the pluggable Connection injection point. + * + * `createClient({ connection })` is the public hook for third-party + * backends (e.g. an embedded chdb-node) to plug into the Node client. + * This file verifies the two contract guarantees: + * + * 1. When a `Connection` is injected, the default HTTP factory + * (`NodeConnectionFactory.create`) is NOT invoked. + * 2. The shared `ClickHouseClient` routes its public methods + * (`query`, `insert`, `command`, `exec`, `ping`, `close`) through + * the injected connection. + */ + +import { describe, it, expect, vi, beforeEach } from "vitest"; +import type Stream from "stream"; +import { Readable } from "stream"; + +import { createClient } from "../../src/client"; +import * as connectionModule from "../../src/connection"; +import type { + Connection, + ConnBaseQueryParams, + ConnExecParams, + ConnInsertParams, +} from "../../src/common/index"; + +beforeEach(() => { + vi.restoreAllMocks(); +}); + +function readableFromString(s: string): Readable { + return Readable.from([Buffer.from(s, "utf8")]); +} + +function makeStubConnection(): Connection & { + /** Bookkeeping so the test can assert which methods were touched. */ + calls: Array<{ op: string; params: unknown }>; +} { + const calls: Array<{ op: string; params: unknown }> = []; + const stub = { + connectionName: "stub" as const, + calls, + close: vi.fn().mockResolvedValue(undefined), + ping: vi.fn().mockResolvedValue({ success: true as const }), + query: vi.fn((p: ConnBaseQueryParams) => { + calls.push({ op: "query", params: p }); + return Promise.resolve({ + stream: readableFromString('{"n":1}\n'), + query_id: "stub-query-id", + response_headers: {}, + http_status_code: 200, + }); + }), + insert: vi.fn((p: ConnInsertParams) => { + calls.push({ op: "insert", params: p }); + return Promise.resolve({ + query_id: "stub-insert-id", + response_headers: {}, + http_status_code: 200, + summary: { + read_rows: "0", + read_bytes: "0", + written_rows: "1", + written_bytes: "1", + result_rows: "0", + result_bytes: "0", + elapsed_ns: "1", + }, + }); + }), + command: vi.fn((p: ConnBaseQueryParams) => { + calls.push({ op: "command", params: p }); + return Promise.resolve({ + query_id: "stub-command-id", + response_headers: {}, + http_status_code: 200, + summary: { + read_rows: "0", + read_bytes: "0", + written_rows: "0", + written_bytes: "0", + result_rows: "0", + result_bytes: "0", + elapsed_ns: "1", + }, + }); + }), + exec: vi.fn((p: ConnExecParams) => { + calls.push({ op: "exec", params: p }); + return Promise.resolve({ + stream: readableFromString(""), + query_id: "stub-exec-id", + response_headers: {}, + http_status_code: 200, + summary: { + read_rows: "0", + read_bytes: "0", + written_rows: "0", + written_bytes: "0", + result_rows: "0", + result_bytes: "0", + elapsed_ns: "1", + }, + }); + }), + }; + return stub; +} + +describe("[Node.js] createClient({ connection }) — pluggable backend injection", () => { + it("does NOT call NodeConnectionFactory.create when a connection is injected", () => { + const createSpy = vi.spyOn( + connectionModule.NodeConnectionFactory, + "create", + ); + const stub = makeStubConnection(); + + const client = createClient({ connection: stub }); + + expect(createSpy).not.toHaveBeenCalled(); + expect(client).toBeDefined(); + }); + + it("DOES call NodeConnectionFactory.create when no connection is injected (default HTTP path unchanged)", () => { + const createSpy = vi.spyOn( + connectionModule.NodeConnectionFactory, + "create", + ); + + createClient({ url: "http://localhost:8123" }); + + expect(createSpy).toHaveBeenCalledTimes(1); + }); + + it("routes client.query through the injected connection's query()", async () => { + const stub = makeStubConnection(); + const client = createClient({ connection: stub }); + + const rs = await client.query({ query: "SELECT 1", format: "JSONEachRow" }); + expect(stub.query).toHaveBeenCalledTimes(1); + // ConnBaseQueryParams shape — `query` is the SQL string (the Client + // layer appends `\nFORMAT ` when caller specifies `format`). + expect(stub.calls[0]?.op).toBe("query"); + const sql = (stub.calls[0]?.params as ConnBaseQueryParams).query; + expect(sql).toMatch(/^SELECT 1\b/); + expect(sql).toContain("FORMAT JSONEachRow"); + // ResultSet drains the injected stream. + const rows = await rs.json(); + expect(rows).toEqual([{ n: 1 }]); + }); + + it("routes client.insert through the injected connection's insert()", async () => { + const stub = makeStubConnection(); + const client = createClient({ connection: stub }); + + await client.insert({ + table: "t", + values: [{ id: 1 }], + format: "JSONEachRow", + }); + expect(stub.insert).toHaveBeenCalledTimes(1); + expect(stub.calls[0]?.op).toBe("insert"); + // Client translates { table, format, values } into an `INSERT ... FORMAT X` query string. + expect( + (stub.calls[0]?.params as ConnInsertParams).query, + ).toContain("INSERT INTO t"); + }); + + it("routes client.command through the injected connection's command()", async () => { + const stub = makeStubConnection(); + const client = createClient({ connection: stub }); + + await client.command({ query: "CREATE TABLE t (x Int32) ENGINE = Memory" }); + expect(stub.command).toHaveBeenCalledTimes(1); + expect(stub.calls[0]?.op).toBe("command"); + }); + + it("routes client.exec through the injected connection's exec()", async () => { + const stub = makeStubConnection(); + const client = createClient({ connection: stub }); + + await client.exec({ query: "SELECT 1" }); + expect(stub.exec).toHaveBeenCalledTimes(1); + expect(stub.calls[0]?.op).toBe("exec"); + }); + + it("routes client.ping through the injected connection's ping()", async () => { + const stub = makeStubConnection(); + const client = createClient({ connection: stub }); + + const r = await client.ping(); + expect(stub.ping).toHaveBeenCalledTimes(1); + expect(r.success).toBe(true); + }); + + it("routes client.close through the injected connection's close()", async () => { + const stub = makeStubConnection(); + const client = createClient({ connection: stub }); + + await client.close(); + expect(stub.close).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/client-node/src/client.ts b/packages/client-node/src/client.ts index e57482fc3..8e6553d49 100644 --- a/packages/client-node/src/client.ts +++ b/packages/client-node/src/client.ts @@ -25,8 +25,20 @@ export class NodeClickHouseClient extends ClickHouseClient { export function createClient( config?: NodeClickHouseClientConfigOptions, ): NodeClickHouseClient { + // If the caller injected a pre-built Connection (pluggable-backend path + // — see NodeClickHouseClientConfigOptions.connection), override the + // default HTTP make_connection factory to return THAT connection + // instead. The factory is invoked with (config, params) by the shared + // Client; we ignore both because the injected connection is already + // fully built by its own factory (e.g. + // `createChdbConnection({ path: ':memory:' })`). + const injected = config?.connection; + const impl = + injected !== undefined + ? { ...NodeConfigImpl, make_connection: () => injected } + : NodeConfigImpl; return new ClickHouseClient({ - impl: NodeConfigImpl, + impl, ...(config || {}), }) as NodeClickHouseClient; } diff --git a/packages/client-node/src/config.ts b/packages/client-node/src/config.ts index ff7e5ffdc..0bb9fb839 100644 --- a/packages/client-node/src/config.ts +++ b/packages/client-node/src/config.ts @@ -8,6 +8,7 @@ import type { import { type BaseClickHouseClientConfigOptions, type CompressionMethod, + type Connection, type ConnectionParams, numberConfigURLValue, } from "./common/index"; @@ -79,6 +80,26 @@ export type NodeClickHouseClientConfigOptions = * through to the request options. * @default undefined */ max_response_headers_size?: number; + /** Pre-built backend connection to use for this client instead of the + * default HTTP(S) connection factory. When provided, the client routes + * every method (`query` / `insert` / `command` / `exec` / `ping` / + * `close`) through this connection's implementation of the public + * {@link Connection} contract, and the HTTP-related options above + * (`tls`, `keep_alive`, `http_agent`, `max_open_connections`, …) are + * ignored. + * + * This is the integration point for pluggable backends — most notably + * an embedded `chdb-node` connection + * (`createChdbConnection({ path: ':memory:' })`) — so the same + * higher-level client API can target either a remote ClickHouse server + * or an in-process backend with a one-line change at construction. + * See https://github.com/ClickHouse/clickhouse-js/blob/main/docs/design/pluggable-connection.md + * for the full design and the asymmetric upstream-clean rationale. + * + * @experimental - unstable API; it might be a subject to change in the + * future; please provide your feedback in the repository. + * @default undefined */ + connection?: Connection; }; interface BasicTLSOptions { diff --git a/packages/client-node/src/index.ts b/packages/client-node/src/index.ts index ba04008b0..10072b699 100644 --- a/packages/client-node/src/index.ts +++ b/packages/client-node/src/index.ts @@ -61,6 +61,22 @@ export { type ClickHouseSpanAttributes, type ClickHouseSpanStatus, type ClickHouseSpanName, + // Pluggable Connection contract — re-exported for third-party backends + // (e.g. chdb-node) implementing `createClient({ connection })`. + type Connection, + type ConnBaseQueryParams, + type ConnBaseResult, + type ConnQueryResult, + type ConnInsertParams, + type ConnInsertResult, + type ConnExecParams, + type ConnExecResult, + type ConnCommandResult, + type ConnPingParams, + type ConnPingResult, + type ConnOperation, + type ClickHouseSummary, + type WithClickHouseSummary, } from "./common/index"; /** From 08679ecc0e9154421eb06ff40e339900bd0ffc84 Mon Sep 17 00:00:00 2001 From: Rahul Nair <254529899+motsc@users.noreply.github.com> Date: Tue, 23 Jun 2026 12:48:50 +0200 Subject: [PATCH 2/5] chore: add 7-day Dependabot cooldown (#881) 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-- --- .github/dependabot.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index e21c3cc12..4ad70ecad 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -6,6 +6,8 @@ updates: schedule: interval: "weekly" day: "monday" + cooldown: + default-days: 7 groups: workflows: dependency-type: "development" @@ -14,6 +16,10 @@ updates: schedule: interval: "weekly" day: "monday" + cooldown: + default-days: 7 + exclude: + - "@clickhouse/*" groups: dev-dependencies: dependency-type: "development" From 66acb4d175f22b161a8c349163a7b4e2aa9888bb Mon Sep 17 00:00:00 2001 From: Rahul Nair <254529899+motsc@users.noreply.github.com> Date: Tue, 23 Jun 2026 12:49:10 +0200 Subject: [PATCH 3/5] ci: add 7-day lockfile age audit for package-lock.json (#882) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .github/workflows/lockfile-age-audit.yml | 68 ++++++++ scripts/ci/lockfile-age-audit.mjs | 208 +++++++++++++++++++++++ 2 files changed, 276 insertions(+) create mode 100644 .github/workflows/lockfile-age-audit.yml create mode 100644 scripts/ci/lockfile-age-audit.mjs diff --git a/.github/workflows/lockfile-age-audit.yml b/.github/workflows/lockfile-age-audit.yml new file mode 100644 index 000000000..7aa449232 --- /dev/null +++ b/.github/workflows/lockfile-age-audit.yml @@ -0,0 +1,68 @@ +name: Lockfile age audit + +on: + pull_request: + types: [opened, synchronize, reopened, labeled, unlabeled] + +permissions: + contents: read + +concurrency: + group: lockfile-age-audit-${{ github.ref }} + cancel-in-progress: true + +jobs: + audit: + runs-on: ubuntu-latest + steps: + # Step-level skip (not job-level) so the job still completes with status "success" + # when the label is set — required for branch protection to be satisfied. + - name: Honor lockfile-age-skip label + id: skip + env: + SKIP: ${{ contains(github.event.pull_request.labels.*.name, 'lockfile-age-skip') }} + shell: bash + run: | + set -euo pipefail + if [[ "$SKIP" == "true" ]]; then + echo "lockfile-age-skip label present — audit will be bypassed." + echo "skip=true" >> "$GITHUB_OUTPUT" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + if: steps.skip.outputs.skip == 'false' + with: + fetch-depth: 0 + - name: Ensure base ref is fetched + if: steps.skip.outputs.skip == 'false' + env: + BASE_REF: ${{ github.base_ref }} + shell: bash + run: | + set -euo pipefail + git fetch --no-tags --quiet origin "$BASE_REF":"refs/remotes/origin/$BASE_REF" + - name: Detect package-lock.json change + id: lockfile + if: steps.skip.outputs.skip == 'false' + env: + BASE_REF: ${{ github.base_ref }} + shell: bash + run: | + set -euo pipefail + changed_files=$(git diff --name-only "origin/${BASE_REF}...HEAD") + if echo "$changed_files" | grep -qx 'package-lock.json'; then + echo "changed=true" >> "$GITHUB_OUTPUT" + else + echo "changed=false" >> "$GITHUB_OUTPUT" + fi + - uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6.3.0 + if: steps.skip.outputs.skip == 'false' && steps.lockfile.outputs.changed == 'true' + with: + node-version: "22" + - name: Audit new package-lock.json entries against 7-day age gate + if: steps.skip.outputs.skip == 'false' && steps.lockfile.outputs.changed == 'true' + env: + MIN_AGE_DAYS: "7" + GITHUB_BASE_REF: ${{ github.base_ref }} + run: node scripts/ci/lockfile-age-audit.mjs diff --git a/scripts/ci/lockfile-age-audit.mjs b/scripts/ci/lockfile-age-audit.mjs new file mode 100644 index 000000000..7b73c74ae --- /dev/null +++ b/scripts/ci/lockfile-age-audit.mjs @@ -0,0 +1,208 @@ +#!/usr/bin/env node +// Fails if the PR introduces package-lock.json entries published less than MIN_AGE_DAYS ago. +// True new entries only — compares base vs head lockfile resolutions, not just diff `+` lines, +// so lockfile reorders don't trigger false positives. +// Skips first-party @clickhouse/* packages (no upstream-compromise risk). +// Fails closed on registry errors (skip via the 'lockfile-age-skip' PR label). +import { execSync } from 'node:child_process' +import { readFileSync } from 'node:fs' + +const MIN_AGE_DAYS = Number.parseInt(process.env.MIN_AGE_DAYS ?? '7', 10) +if (!Number.isFinite(MIN_AGE_DAYS) || MIN_AGE_DAYS <= 0) { + console.error( + `MIN_AGE_DAYS must be a positive integer (got: ${process.env.MIN_AGE_DAYS ?? '7'})`, + ) + process.exit(2) +} +const BASE_REF = process.env.GITHUB_BASE_REF || 'main' +const cutoffMs = Date.now() - MIN_AGE_DAYS * 86400_000 + +// First-party scopes — npm has no native equivalent to yarn's npmPreapprovedPackages, +// so hardcoded here. Mirrors the Dependabot cooldown.exclude list. +const PREAPPROVED_SCOPES = ['@clickhouse/'] + +const REGISTRY_HOSTS = ['registry.npmjs.org', 'registry.npmmirror.com'] + +function isRegistryEntry(resolved) { + if (!resolved || typeof resolved !== 'string') return false + try { + const u = new URL(resolved) + return REGISTRY_HOSTS.includes(u.host) + } catch { + return false + } +} + +function isPreapproved(name) { + return PREAPPROVED_SCOPES.some((scope) => name.startsWith(scope)) +} + +// node_modules/foo -> foo +// node_modules/@scope/bar -> @scope/bar +// node_modules/foo/node_modules/baz -> baz +// node_modules/foo/node_modules/@scope/baz -> @scope/baz +function packageNameFromPath(path) { + const idx = path.lastIndexOf('node_modules/') + if (idx === -1) return null + return path.slice(idx + 'node_modules/'.length) +} + +function extractNpmResolutions(text) { + const out = new Map() + let parsed + try { + parsed = JSON.parse(text) + } catch (err) { + throw new Error(`Invalid JSON in lockfile: ${err.message}`) + } + const version = parsed.lockfileVersion + if (version !== 2 && version !== 3) { + throw new Error( + `Unsupported lockfileVersion ${version}. This audit handles package-lock.json v2/v3 (npm 7+).`, + ) + } + const packages = parsed.packages + if (!packages || typeof packages !== 'object') return out + for (const [path, entry] of Object.entries(packages)) { + if (path === '') continue // root project + if (!entry || typeof entry !== 'object') continue + if (entry.link === true) continue // workspace symlinks + if (!entry.version) continue + if (!isRegistryEntry(entry.resolved)) continue + const name = packageNameFromPath(path) + if (!name) continue + // Key by name@version so multiple paths resolving to the same registry entry collapse. + const key = `${name}@${entry.version}` + out.set(key, { name, version: entry.version }) + } + return out +} + +// Use the merge-base, not the branch tip — what the PR *actually introduced* +// is what's in head but not in the common ancestor with main. Comparing +// against the branch tip would flag stale pins as "new" when they pre-date +// the PR. +let mergeBase +try { + mergeBase = execSync(`git merge-base "origin/${BASE_REF}" HEAD`, { + encoding: 'utf8', + }).trim() +} catch (err) { + console.error(`Could not find merge-base of HEAD and origin/${BASE_REF}: ${err.message}`) + process.exit(2) +} + +let baseLockfile +try { + baseLockfile = execSync(`git show "${mergeBase}:package-lock.json"`, { + encoding: 'utf8', + maxBuffer: 256 * 1024 * 1024, + }) +} catch (err) { + console.error( + `Could not read package-lock.json at merge-base ${mergeBase.slice(0, 10)}: ${err.message}`, + ) + console.error( + `If package-lock.json is being introduced for the first time, use the 'lockfile-age-skip' label.`, + ) + process.exit(2) +} +const headLockfile = readFileSync('package-lock.json', 'utf8') + +let baseResolutions, headResolutions +try { + baseResolutions = extractNpmResolutions(baseLockfile) + headResolutions = extractNpmResolutions(headLockfile) +} catch (err) { + console.error(err.message) + process.exit(2) +} + +const added = new Map() +for (const [key, value] of headResolutions) { + if (baseResolutions.has(key)) continue + if (isPreapproved(value.name)) continue + added.set(key, value) +} + +if (added.size === 0) { + console.log('No new npm resolutions to audit.') + process.exit(0) +} + +console.log(`Auditing ${added.size} new lockfile entries against ${MIN_AGE_DAYS}-day age gate.`) + +const violations = [] +const errors = [] + +async function checkOne({ name, version }) { + // Scoped names contain '/' which must be percent-encoded for the registry URL. + // encodeURIComponent handles all unsafe chars (replace('/', ...) only hits the first). + const url = `https://registry.npmjs.org/${encodeURIComponent(name)}` + let res + try { + res = await fetch(url, { headers: { Accept: 'application/vnd.npm.install-v1+json' } }) + } catch (err) { + errors.push(`${name}: fetch failed (${err.message})`) + return + } + if (!res.ok) { + errors.push(`${name}: registry returned ${res.status}`) + return + } + let data + try { + data = await res.json() + } catch { + errors.push(`${name}: invalid JSON from registry`) + return + } + const publishedAt = data.time?.[version] + if (!publishedAt) { + errors.push(`${name}@${version}: missing publish time in registry response`) + return + } + const publishedMs = new Date(publishedAt).getTime() + if (publishedMs > cutoffMs) { + const ageDays = Math.floor((Date.now() - publishedMs) / 86400_000) + const mergeAfter = new Date(publishedMs + MIN_AGE_DAYS * 86400_000).toISOString() + violations.push({ name, version, publishedAt, ageDays, mergeAfter }) + } +} + +const queue = [...added.values()] +const CONCURRENCY = 8 +async function worker() { + while (queue.length) { + const next = queue.shift() + if (next) await checkOne(next) + } +} +await Promise.all(Array.from({ length: CONCURRENCY }, worker)) + +let failed = false + +if (errors.length > 0) { + console.error(`\n✗ ${errors.length} registry lookup error(s) — failing closed:`) + for (const e of errors) console.error(` - ${e}`) + failed = true +} + +if (violations.length > 0) { + console.error(`\n✗ ${violations.length} entries younger than ${MIN_AGE_DAYS} days:`) + for (const v of violations) { + console.error(` ${v.name}@${v.version}`) + console.error(` published: ${v.publishedAt} (${v.ageDays} days ago)`) + console.error(` mergeable after: ${v.mergeAfter}`) + } + const latestMergeAfter = violations.map((v) => v.mergeAfter).sort().at(-1) + console.error(`\nEarliest this PR can merge: ${latestMergeAfter}`) + failed = true +} + +if (failed) { + console.error(`\nTo bypass for an urgent security fix, add the 'lockfile-age-skip' label to the PR.`) + process.exit(1) +} + +console.log(`✓ All ${added.size} new entries ≥ ${MIN_AGE_DAYS} days old.`) From 274844f1495be0d63279241ee26dd7cc198fefab Mon Sep 17 00:00:00 2001 From: Peter Leonov Date: Tue, 23 Jun 2026 12:49:33 +0200 Subject: [PATCH 4/5] Keep pluggable connection as a narrow internal chDB experiment (follow-up to #879) (#880) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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` 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](https://github.com/chdb-io/chdb-node/pull/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) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- docs/design/pluggable-connection.md | 138 ---------------------------- packages/client-node/src/client.ts | 8 +- packages/client-node/src/config.ts | 20 ++-- packages/client-node/src/index.ts | 16 ---- 5 files changed, 13 insertions(+), 171 deletions(-) delete mode 100644 docs/design/pluggable-connection.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c17c8388..f6343bbe8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ - (Node.js) Added a RowBinary reader library and agent skill under [`skills/clickhouse-js-node-rowbinary-parser`](./skills/clickhouse-js-node-rowbinary-parser). It ships type-specific, monomorphizable building blocks for decoding `RowBinary` / `RowBinaryWithNames` / `RowBinaryWithNamesAndTypes` streams (full-buffer and chunked), plus a skill that guides an agent to generate bespoke high-performance parsers from a query's column types. The skill is bundled into `@clickhouse/client` (registered in `agents.skills`) and is also published independently as the [`@clickhouse/rowbinary`](https://www.npmjs.com/package/@clickhouse/rowbinary) package. A matching RowBinary writer is planned. ([#864]) -- (Node.js, `@experimental`) Added an additive `connection?: Connection` option to `createClient` that lets a caller plug an externally-built backend `Connection` in place of the default HTTP(S) factory. When provided, the client routes every method (`query` / `insert` / `command` / `exec` / `ping` / `close`) through the injected connection's `Connection` implementation, and the HTTP-related options (`tls`, `keep_alive`, `http_agent`, `max_open_connections`, …) are ignored; when omitted, behavior is byte-identical to today. The implementation is a 3-line `make_connection` factory override on `NodeConfigImpl` — no new HTTP code, no public types added beyond the field itself. The `Connection` contract types (`Connection`, `ConnBaseQueryParams`, `ConnQueryResult`, `ConnInsertParams`, `ConnPingResult`, etc.) are now re-exported from the public `@clickhouse/client` entrypoint so third-party backends don't need to deep-import. Intended consumer is an embedded chDB backend ([`chdb-node`](https://github.com/chdb-io/chdb-node)) shipping `createChdbConnection` from `chdb/connection`; see [`docs/design/pluggable-connection.md`](./docs/design/pluggable-connection.md) for the full design and asymmetric upstream-clean rationale. The field is tagged `@experimental` so the shape can iterate based on what the first real backend learns. ([#879]) +- (Node.js, `@experimental`) Added an additive `connection?: Connection` option to `createClient` that lets a caller plug an externally-built backend `Connection`-like object in place of the default HTTP(S) factory. Only supposed to be used for testing the `chDB` integration. ([#879]) # 1.22.0 diff --git a/docs/design/pluggable-connection.md b/docs/design/pluggable-connection.md deleted file mode 100644 index 6c3eb8028..000000000 --- a/docs/design/pluggable-connection.md +++ /dev/null @@ -1,138 +0,0 @@ -# Pluggable Connection — design - -> **Status**: minimal upstream hook implemented in this branch -> (`feat/pluggable-connection`). -> Tracking issue: [#865](https://github.com/ClickHouse/clickhouse-js/issues/865). -> Parallel Python proposal: [ClickHouse/clickhouse-connect#809](https://github.com/ClickHouse/clickhouse-connect/issues/809). -> chDB-side implementation: -> (the `chdb/connection` subpath export). - -## What this PR adds - -A single, additive hook on the `@clickhouse/client` Node client: - -```ts -createClient({ - connection: createChdbConnection({ path: ":memory:" }), -}); -``` - -`NodeClickHouseClientConfigOptions.connection?: Connection` -lets a caller plug an externally-built `Connection` into `createClient` -instead of letting the package build its default HTTP connection from -`tls` / `keep_alive` / `http_agent` / etc. options. The existing -`Connection` interface is unchanged — the backend implements -that exact contract and gets dropped in. - -When `connection` is omitted (the default), behavior is byte-identical -to today: the bundled HTTP factory is used. - -Implementation is a 3-line wrap of `NodeConfigImpl`: when `connection` -is provided, spread `NodeConfigImpl` and override only its -`make_connection` factory to return the injected connection verbatim; -otherwise pass `NodeConfigImpl` through unchanged. No HTTP/HTTPS code -is touched. - -## Why this shape - -Two design pressures, in tension, drove it: - -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: every new public - method, every parameter change, every streaming refactor would have - to be applied twice. - -The compromise: - -- **One public client API** stays `createClient` from `@clickhouse/client`. -- **The Connection interface** behind it is unchanged — we only - made it injectable. -- **The backend implementation lives in the backend's repo.** chdb-node - ships `ChdbConnection` from `chdb/connection`. All chdb-side data and - logic (the in-process memory model, the `.chdb` extension namespace - for chDB-specific escape hatches, the skip list of tests chdb does - not support, the CI matrix that runs THIS suite against - ChdbConnection) stays in chdb-node's repo. **This repo carries zero - chdb code, zero chdb tests, zero chdb CI dependency.** - -## Usage - -```ts -import { createClient } from "@clickhouse/client"; -import { createChdbConnection } from "chdb/connection"; - -const client = createClient({ - connection: createChdbConnection({ path: ":memory:" }), -}); - -// All downstream code is identical across connections. -const rs = await client.query({ - query: "SELECT * FROM numbers(5)", - format: "JSONEachRow", -}); -for await (const row of rs.stream()) { - /* ... */ -} -await client.insert({ table: "t", values: rows }); -await client.close(); -``` - -The `chdb/connection` subpath is exposed by chdb-node v3.1+; see its -README and design doc for the in-process memory model, the `.chdb` -extension namespace, and the streaming roadmap. - -## Test parity — who runs what, where - -This repo does **not** participate in chdb-vs-server test parity. -There are no chdb-specific markers, no chdb skip lists, no chdb test -runners here. - -Parity is owned by the backend's repo. chdb-node maintains: - -- A blacklist of integration tests that chdb does not support - (in chdb-node's repo, as `tests/clickhouse-js/skip_list.json`). -- A CI workflow that clones `@clickhouse/client` at a configured - ref, runs its integration suite with the chdb backend injected, - and applies the local blacklist. - -chdb-node tests against the **latest released version** of -`@clickhouse/client`. Only when `@clickhouse/client` cuts a new release -does chdb-node re-run its parity job and update its blacklist for any -newly-failing tests. - -## Data-vs-code split - -``` -┌──────────────────────────────────────┐ ┌──────────────────────────────────────┐ -│ @clickhouse/client (this repo) │ │ chdb-node (and any future backend) │ -│ │ │ │ -│ • Connection interface │ │ • ChdbConnection implements │ -│ (unchanged) │ │ Connection │ -│ • createClient({ connection }) │ ◄── │ • chdb/connection subpath export │ -│ new public injection point │ │ • tests/clickhouse-js/skip_list.json │ -│ │ │ (chdb-owned blacklist) │ -│ ❌ zero chdb code │ │ • CI clones THIS repo and runs its │ -│ ❌ zero chdb tests │ │ suite with skip_list applied │ -│ ❌ zero chdb CI dependency │ │ │ -└──────────────────────────────────────┘ └──────────────────────────────────────┘ -``` - -Adding a future backend (e.g. native TCP) repeats the same pattern in -the backend's own repo. No PR to this repo is needed. - -## What this PR deliberately does NOT do - -- **No chdb-specific code paths.** This repo stays backend-agnostic. -- **No dependency on `chdb`.** Not in any `package.json`. -- **No per-feature test markers.** Tests stay un-tagged. Backends own - their own blacklists in their own repos. -- **No skip-profile loader.** The injection point is the only hook; - what to skip is the backend's data, applied by the backend's - runner. -- **No client-web changes.** chdb is N-API and can only run in Node / - Bun / Deno; a future web-runtime backend repeats this pattern in - `client-web` if one ever lands. diff --git a/packages/client-node/src/client.ts b/packages/client-node/src/client.ts index 8e6553d49..ec0cbce0e 100644 --- a/packages/client-node/src/client.ts +++ b/packages/client-node/src/client.ts @@ -25,13 +25,9 @@ export class NodeClickHouseClient extends ClickHouseClient { export function createClient( config?: NodeClickHouseClientConfigOptions, ): NodeClickHouseClient { - // If the caller injected a pre-built Connection (pluggable-backend path - // — see NodeClickHouseClientConfigOptions.connection), override the + // If the caller injected a pre-built Connection, override the // default HTTP make_connection factory to return THAT connection - // instead. The factory is invoked with (config, params) by the shared - // Client; we ignore both because the injected connection is already - // fully built by its own factory (e.g. - // `createChdbConnection({ path: ':memory:' })`). + // instead. Used for the experimental integration with chDB only. const injected = config?.connection; const impl = injected !== undefined diff --git a/packages/client-node/src/config.ts b/packages/client-node/src/config.ts index 0bb9fb839..7c797eac9 100644 --- a/packages/client-node/src/config.ts +++ b/packages/client-node/src/config.ts @@ -83,21 +83,21 @@ export type NodeClickHouseClientConfigOptions = /** Pre-built backend connection to use for this client instead of the * default HTTP(S) connection factory. When provided, the client routes * every method (`query` / `insert` / `command` / `exec` / `ping` / - * `close`) through this connection's implementation of the public + * `close`) through this connection's implementation of the internal * {@link Connection} contract, and the HTTP-related options above * (`tls`, `keep_alive`, `http_agent`, `max_open_connections`, …) are * ignored. * - * This is the integration point for pluggable backends — most notably - * an embedded `chdb-node` connection - * (`createChdbConnection({ path: ':memory:' })`) — so the same - * higher-level client API can target either a remote ClickHouse server - * or an in-process backend with a one-line change at construction. - * See https://github.com/ClickHouse/clickhouse-js/blob/main/docs/design/pluggable-connection.md - * for the full design and the asymmetric upstream-clean rationale. + * This is a deliberately narrow, internal experiment to unblock the chDB + * integration — NOT the start of a public pluggable-backend / plugin system. + * The {@link Connection} contract is intentionally not re-exported from the + * package entrypoint, so a backend must deep-import or structurally match its + * shape; that friction is by design and signals the API may change. Keeping + * the client's public surface slim avoids a second client family mirroring the + * whole public API. * - * @experimental - unstable API; it might be a subject to change in the - * future; please provide your feedback in the repository. + * @experimental - unstable API; used only for integrating with chDB. + * @see https://github.com/chdb-io/chdb-node/pull/52 * @default undefined */ connection?: Connection; }; diff --git a/packages/client-node/src/index.ts b/packages/client-node/src/index.ts index 10072b699..ba04008b0 100644 --- a/packages/client-node/src/index.ts +++ b/packages/client-node/src/index.ts @@ -61,22 +61,6 @@ export { type ClickHouseSpanAttributes, type ClickHouseSpanStatus, type ClickHouseSpanName, - // Pluggable Connection contract — re-exported for third-party backends - // (e.g. chdb-node) implementing `createClient({ connection })`. - type Connection, - type ConnBaseQueryParams, - type ConnBaseResult, - type ConnQueryResult, - type ConnInsertParams, - type ConnInsertResult, - type ConnExecParams, - type ConnExecResult, - type ConnCommandResult, - type ConnPingParams, - type ConnPingResult, - type ConnOperation, - type ClickHouseSummary, - type WithClickHouseSummary, } from "./common/index"; /** From 3351cb63683cf57c7e3e4ba831430af37bd95111 Mon Sep 17 00:00:00 2001 From: Peter Leonov Date: Wed, 24 Jun 2026 12:39:57 +0200 Subject: [PATCH 5/5] =?UTF-8?q?fix(ci):=20lockfile-age-audit=20=E2=80=94?= =?UTF-8?q?=20fail=20closed=20on=20non-registry=20sources=20+=20prettify?= =?UTF-8?q?=20(#886)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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) --- .claude/skills/fix-release-pr/SKILL.md | 174 ++++++++++++++++ scripts/ci/lockfile-age-audit.mjs | 267 +++++++++++++++---------- 2 files changed, 339 insertions(+), 102 deletions(-) create mode 100644 .claude/skills/fix-release-pr/SKILL.md diff --git a/.claude/skills/fix-release-pr/SKILL.md b/.claude/skills/fix-release-pr/SKILL.md new file mode 100644 index 000000000..1a154b338 --- /dev/null +++ b/.claude/skills/fix-release-pr/SKILL.md @@ -0,0 +1,174 @@ +--- +name: fix-release-pr +description: > + Fix CI failures and address code-review comments on a pull request that + targets the protected `release` branch of `ClickHouse/clickhouse-js`. Release + PRs are snapshots of `main` (their head branch is usually `main` itself) and + cannot be edited directly — branch protection blocks pushing fixes onto them. + Use this skill whenever the work is "fix the CI / address the review comments + on PR #N" and that PR's base branch is `release`: it routes the fix through a + separate PR to `main`, then closes the loop on the original release PR by + replying to and resolving the review threads. Triggers on phrasing like "fix + this release PR", "the release PR is failing CI", "address the review comments + but it's a release branch", or after checking out a PR whose base is `release`. + Do NOT use this for ordinary feature/fix PRs that target `main` — for those, + just push to the PR's own branch. +--- + +# Fixing PRs to the `release` branch + +## Why this skill exists + +In `clickhouse-js`, the `release` branch receives **release PRs** — snapshots of +`main` opened to cut a version (e.g. titled "1.23 beta2"). Two properties make +them special: + +- The PR's **head branch is usually `main` itself** (base `release`, head `main`). + `gh pr checkout ` is therefore a no-op that leaves you on `main` — do **not** + commit fixes there. +- `release` is **protected**: you cannot push commits onto the release PR to fix + CI or review feedback. + +So fixes never go onto the release PR. They go to **`main` via a separate PR**; +once that merges, the release branch is re-synced from `main` and the release PR +picks the fix up. This skill is that workflow. + +## Step 1 — Confirm it's a release PR + +```bash +gh pr view --json title,baseRefName,headRefName,headRefOid +``` + +If `baseRefName` is `release`, proceed. (If it's `main`, this skill does not +apply — push to the PR's branch normally.) + +## Step 2 — Gather what needs fixing + +**CI failures.** List checks and find the `fail` rows: + +```bash +gh pr checks +``` + +- The job named **`success`** is an _aggregate gate_ ("Fail if any needed job + failed") — it only fails _because_ a real job failed. Ignore it as a root cause + and find the actual failing job. +- Open the real failure log: + +```bash +gh run view --job --log-failed | tail -50 +``` + +**Review comments.** Inline review comments with their REST IDs and the GraphQL +thread node IDs (you need both: REST `id` to reply, thread node `id` to resolve): + +```bash +# Inline comments: REST id + location + author + body +gh api repos/ClickHouse/clickhouse-js/pulls//comments \ + -q '.[] | "\(.id)\t\(.path):\(.line)\t\(.user.login)\n\(.body)\n---"' + +# Review threads: node id (PRRT_…), resolved state, and first comment's databaseId +gh api graphql -f query=' +{ repository(owner:"ClickHouse", name:"clickhouse-js") { + pullRequest(number: ) { + reviewThreads(first: 50) { nodes { + id isResolved + comments(first: 1) { nodes { databaseId path body } } + } } + } } }' \ + -q '.data.repository.pullRequest.reviewThreads.nodes[] + | "\(.id)\tresolved=\(.isResolved)\tdbId=\(.comments.nodes[0].databaseId)\t\(.comments.nodes[0].path)"' +``` + +Match each thread (`PRRT_…` node id) to its first comment's `databaseId` — that +`databaseId` is the REST comment id you reply to in Step 5. + +## Step 3 — Branch off the latest `main` + +Never branch off the release PR head. Start from up-to-date `main`: + +```bash +git fetch origin +git checkout -b fix/ origin/main +``` + +## Step 4 — Make the fix and verify + +Apply the fixes. Then verify with the repo's own tooling (run the `setup` skill +first if `node_modules` isn't populated): + +- **Prettier** is the most common release-PR CI failure. House style is the + Prettier defaults (`.prettierrc` is `{}` → double quotes + semicolons). + Fix and check: + ```bash + node_modules/.bin/prettier --write + npm run -s prettier:check + ``` + `prettier:check` runs on the whole repo and may flag **untracked local scratch + dirs** (e.g. a `type-parser/` working directory). Those aren't part of the PR + and CI never sees them — only tracked files matter. Confirm no _tracked_ file + is flagged: + ```bash + npm run -s prettier:check 2>&1 \ + | grep -oE '\[(warn|error)\] [^ ]+\.(mjs|ts|js|json|ya?ml|md)' \ + | grep -v '/' | sort -u # empty = clean + ``` +- For standalone scripts: `node --check `. +- For library code: `npm run typecheck`, `npm run lint`, and the relevant + `npm run test:*` target (see the `setup` skill for what each needs). +- When the fix is logic (not just formatting), exercise it directly — e.g. drive + the script against synthetic inputs in the scratchpad dir and assert the + exit code / output, rather than trusting it by inspection. + +## Step 5 — Commit, push, open the PR to `main` + +```bash +git commit -m "(): + +Addresses CI failure / review feedback on # (release PR). The fix lands +on \`main\` separately because # targets the protected \`release\` branch. + +Co-Authored-By: Claude Opus 4.8 (1M context) " + +git push -u origin fix/ +gh pr create --base main --head fix/ --title "…" --body "…" +``` + +In the new PR body, explicitly state it addresses #N and why it's a separate PR +(release branch is protected). Map each fix back to the specific CI failure or +review comment it resolves. + +## Step 6 — Close the loop on the release PR + +For **each** original review comment, reply pointing to the new PR, then resolve +the thread. + +```bash +# Reply (use the REST comment id from Step 2) +gh api repos/ClickHouse/clickhouse-js/pulls//comments//replies \ + -f body='Fixed in #. . (Lands on `main` separately since this PR targets the protected `release` branch.)' \ + -q '.html_url' + +# Resolve the thread (use the PRRT_… node id from Step 2) +gh api graphql \ + -f query='mutation($id:ID!){resolveReviewThread(input:{threadId:$id}){thread{id isResolved}}}' \ + -f id='' \ + -q '.data.resolveReviewThread.thread | "\(.id) resolved=\(.isResolved)"' +``` + +## Step 7 — Flag the re-sync + +The fix is on `main`, not on `release`. Remind the maintainer that once the new +PR merges, the `release` branch / the release PR must be re-synced from `main` +to pick the fix up. Do not attempt to push to `release` yourself. + +## Quick reference — the whole flow + +1. `gh pr view --json baseRefName` → confirm base is `release`. +2. `gh pr checks ` + `gh run view --job --log-failed` → real CI failure (ignore the `success` gate). +3. `gh api …/pulls//comments` + GraphQL `reviewThreads` → review comments + thread ids. +4. `git checkout -b fix/… origin/main` → branch off latest `main`. +5. Fix → verify (prettier / typecheck / lint / tests / `node --check`). +6. Commit → push → `gh pr create --base main`. +7. Reply to each review comment (REST `…/comments//replies`) → resolve each thread (GraphQL `resolveReviewThread`). +8. Remind: re-sync `release` from `main` after merge. diff --git a/scripts/ci/lockfile-age-audit.mjs b/scripts/ci/lockfile-age-audit.mjs index 7b73c74ae..67fa125f0 100644 --- a/scripts/ci/lockfile-age-audit.mjs +++ b/scripts/ci/lockfile-age-audit.mjs @@ -3,38 +3,65 @@ // True new entries only — compares base vs head lockfile resolutions, not just diff `+` lines, // so lockfile reorders don't trigger false positives. // Skips first-party @clickhouse/* packages (no upstream-compromise risk). -// Fails closed on registry errors (skip via the 'lockfile-age-skip' PR label). -import { execSync } from 'node:child_process' -import { readFileSync } from 'node:fs' +// Fails closed on registry errors and on new deps resolved from a non-registry source +// (skip via the 'lockfile-age-skip' PR label). +import { execSync } from "node:child_process"; +import { readFileSync } from "node:fs"; -const MIN_AGE_DAYS = Number.parseInt(process.env.MIN_AGE_DAYS ?? '7', 10) +const MIN_AGE_DAYS = Number.parseInt(process.env.MIN_AGE_DAYS ?? "7", 10); if (!Number.isFinite(MIN_AGE_DAYS) || MIN_AGE_DAYS <= 0) { console.error( - `MIN_AGE_DAYS must be a positive integer (got: ${process.env.MIN_AGE_DAYS ?? '7'})`, - ) - process.exit(2) + `MIN_AGE_DAYS must be a positive integer (got: ${process.env.MIN_AGE_DAYS ?? "7"})`, + ); + process.exit(2); } -const BASE_REF = process.env.GITHUB_BASE_REF || 'main' -const cutoffMs = Date.now() - MIN_AGE_DAYS * 86400_000 +const BASE_REF = process.env.GITHUB_BASE_REF || "main"; +const cutoffMs = Date.now() - MIN_AGE_DAYS * 86400_000; // First-party scopes — npm has no native equivalent to yarn's npmPreapprovedPackages, // so hardcoded here. Mirrors the Dependabot cooldown.exclude list. -const PREAPPROVED_SCOPES = ['@clickhouse/'] +const PREAPPROVED_SCOPES = ["@clickhouse/"]; -const REGISTRY_HOSTS = ['registry.npmjs.org', 'registry.npmmirror.com'] +const REGISTRY_HOSTS = ["registry.npmjs.org", "registry.npmmirror.com"]; -function isRegistryEntry(resolved) { - if (!resolved || typeof resolved !== 'string') return false +// Classify an entry's `resolved` field: +// 'registry' — an allowed HTTPS registry tarball; audited against the age gate. +// 'foreign' — present but not an allowed HTTPS registry tarball (alternative +// registry, plain http, git+https, or a non-URL string from a +// hand-edited lockfile). A bypass vector for the age gate, so we fail +// closed on newly-added foreign entries (escape hatch: the +// 'lockfile-age-skip' PR label). +// 'local' — no resolved URL at all (workspace source dirs) or a file: dependency; +// not a registry download, so there is nothing to age-check. +function classifyResolved(resolved) { + if (!resolved || typeof resolved !== "string") return "local"; + let u; try { - const u = new URL(resolved) - return REGISTRY_HOSTS.includes(u.host) + u = new URL(resolved); } catch { - return false + // A present-but-unparseable resolved string (e.g. a hand-edited lockfile) + // must not slip through the gate — fail closed. + return "foreign"; + } + if (u.protocol === "file:") return "local"; + if (u.protocol === "https:" && REGISTRY_HOSTS.includes(u.host)) + return "registry"; + return "foreign"; +} + +// Render a `resolved` value for CI logs without leaking any embedded credentials +// (e.g. https://user:token@host/...). `URL.host` excludes userinfo, query, and path. +function redactResolved(resolved) { + try { + const u = new URL(resolved); + return `${u.protocol}//${u.host}`; + } catch { + return ""; } } function isPreapproved(name) { - return PREAPPROVED_SCOPES.some((scope) => name.startsWith(scope)) + return PREAPPROVED_SCOPES.some((scope) => name.startsWith(scope)); } // node_modules/foo -> foo @@ -42,167 +69,203 @@ function isPreapproved(name) { // node_modules/foo/node_modules/baz -> baz // node_modules/foo/node_modules/@scope/baz -> @scope/baz function packageNameFromPath(path) { - const idx = path.lastIndexOf('node_modules/') - if (idx === -1) return null - return path.slice(idx + 'node_modules/'.length) + const idx = path.lastIndexOf("node_modules/"); + if (idx === -1) return null; + return path.slice(idx + "node_modules/".length); } function extractNpmResolutions(text) { - const out = new Map() - let parsed + const out = new Map(); + let parsed; try { - parsed = JSON.parse(text) + parsed = JSON.parse(text); } catch (err) { - throw new Error(`Invalid JSON in lockfile: ${err.message}`) + throw new Error(`Invalid JSON in lockfile: ${err.message}`); } - const version = parsed.lockfileVersion + const version = parsed.lockfileVersion; if (version !== 2 && version !== 3) { throw new Error( `Unsupported lockfileVersion ${version}. This audit handles package-lock.json v2/v3 (npm 7+).`, - ) + ); } - const packages = parsed.packages - if (!packages || typeof packages !== 'object') return out + const packages = parsed.packages; + if (!packages || typeof packages !== "object") return out; for (const [path, entry] of Object.entries(packages)) { - if (path === '') continue // root project - if (!entry || typeof entry !== 'object') continue - if (entry.link === true) continue // workspace symlinks - if (!entry.version) continue - if (!isRegistryEntry(entry.resolved)) continue - const name = packageNameFromPath(path) - if (!name) continue - // Key by name@version so multiple paths resolving to the same registry entry collapse. - const key = `${name}@${entry.version}` - out.set(key, { name, version: entry.version }) + if (path === "") continue; // root project + if (!entry || typeof entry !== "object") continue; + if (entry.link === true) continue; // workspace symlinks + if (!entry.version) continue; + const kind = classifyResolved(entry.resolved); + if (kind === "local") continue; + const name = packageNameFromPath(path); + if (!name) continue; + // Key by name@version so multiple paths resolving to the same entry collapse. + const key = `${name}@${entry.version}`; + out.set(key, { + name, + version: entry.version, + kind, + resolved: entry.resolved, + }); } - return out + return out; } // Use the merge-base, not the branch tip — what the PR *actually introduced* // is what's in head but not in the common ancestor with main. Comparing // against the branch tip would flag stale pins as "new" when they pre-date // the PR. -let mergeBase +let mergeBase; try { mergeBase = execSync(`git merge-base "origin/${BASE_REF}" HEAD`, { - encoding: 'utf8', - }).trim() + encoding: "utf8", + }).trim(); } catch (err) { - console.error(`Could not find merge-base of HEAD and origin/${BASE_REF}: ${err.message}`) - process.exit(2) + console.error( + `Could not find merge-base of HEAD and origin/${BASE_REF}: ${err.message}`, + ); + process.exit(2); } -let baseLockfile +let baseLockfile; try { baseLockfile = execSync(`git show "${mergeBase}:package-lock.json"`, { - encoding: 'utf8', + encoding: "utf8", maxBuffer: 256 * 1024 * 1024, - }) + }); } catch (err) { console.error( `Could not read package-lock.json at merge-base ${mergeBase.slice(0, 10)}: ${err.message}`, - ) + ); console.error( `If package-lock.json is being introduced for the first time, use the 'lockfile-age-skip' label.`, - ) - process.exit(2) + ); + process.exit(2); } -const headLockfile = readFileSync('package-lock.json', 'utf8') +const headLockfile = readFileSync("package-lock.json", "utf8"); -let baseResolutions, headResolutions +let baseResolutions, headResolutions; try { - baseResolutions = extractNpmResolutions(baseLockfile) - headResolutions = extractNpmResolutions(headLockfile) + baseResolutions = extractNpmResolutions(baseLockfile); + headResolutions = extractNpmResolutions(headLockfile); } catch (err) { - console.error(err.message) - process.exit(2) + console.error(err.message); + process.exit(2); } -const added = new Map() +const added = new Map(); for (const [key, value] of headResolutions) { - if (baseResolutions.has(key)) continue - if (isPreapproved(value.name)) continue - added.set(key, value) + if (baseResolutions.has(key)) continue; + if (isPreapproved(value.name)) continue; + added.set(key, value); } if (added.size === 0) { - console.log('No new npm resolutions to audit.') - process.exit(0) + console.log("No new npm resolutions to audit."); + process.exit(0); } -console.log(`Auditing ${added.size} new lockfile entries against ${MIN_AGE_DAYS}-day age gate.`) +console.log( + `Auditing ${added.size} new lockfile entries against ${MIN_AGE_DAYS}-day age gate.`, +); -const violations = [] -const errors = [] +const violations = []; +const errors = []; + +// Newly-added entries resolved from a non-registry source bypass the age gate entirely, +// so fail closed on them rather than silently ignoring (escape hatch: 'lockfile-age-skip'). +const registryEntries = []; +for (const value of added.values()) { + if (value.kind === "foreign") { + errors.push( + `${value.name}@${value.version}: resolved from a non-registry source (${redactResolved(value.resolved)}) — not covered by the age gate`, + ); + } else { + registryEntries.push(value); + } +} async function checkOne({ name, version }) { // Scoped names contain '/' which must be percent-encoded for the registry URL. // encodeURIComponent handles all unsafe chars (replace('/', ...) only hits the first). - const url = `https://registry.npmjs.org/${encodeURIComponent(name)}` - let res + const url = `https://registry.npmjs.org/${encodeURIComponent(name)}`; + let res; try { - res = await fetch(url, { headers: { Accept: 'application/vnd.npm.install-v1+json' } }) + res = await fetch(url, { + headers: { Accept: "application/vnd.npm.install-v1+json" }, + }); } catch (err) { - errors.push(`${name}: fetch failed (${err.message})`) - return + errors.push(`${name}: fetch failed (${err.message})`); + return; } if (!res.ok) { - errors.push(`${name}: registry returned ${res.status}`) - return + errors.push(`${name}: registry returned ${res.status}`); + return; } - let data + let data; try { - data = await res.json() + data = await res.json(); } catch { - errors.push(`${name}: invalid JSON from registry`) - return + errors.push(`${name}: invalid JSON from registry`); + return; } - const publishedAt = data.time?.[version] + const publishedAt = data.time?.[version]; if (!publishedAt) { - errors.push(`${name}@${version}: missing publish time in registry response`) - return + errors.push( + `${name}@${version}: missing publish time in registry response`, + ); + return; } - const publishedMs = new Date(publishedAt).getTime() + const publishedMs = new Date(publishedAt).getTime(); if (publishedMs > cutoffMs) { - const ageDays = Math.floor((Date.now() - publishedMs) / 86400_000) - const mergeAfter = new Date(publishedMs + MIN_AGE_DAYS * 86400_000).toISOString() - violations.push({ name, version, publishedAt, ageDays, mergeAfter }) + const ageDays = Math.floor((Date.now() - publishedMs) / 86400_000); + const mergeAfter = new Date( + publishedMs + MIN_AGE_DAYS * 86400_000, + ).toISOString(); + violations.push({ name, version, publishedAt, ageDays, mergeAfter }); } } -const queue = [...added.values()] -const CONCURRENCY = 8 +const queue = [...registryEntries]; +const CONCURRENCY = 8; async function worker() { while (queue.length) { - const next = queue.shift() - if (next) await checkOne(next) + const next = queue.shift(); + if (next) await checkOne(next); } } -await Promise.all(Array.from({ length: CONCURRENCY }, worker)) +await Promise.all(Array.from({ length: CONCURRENCY }, worker)); -let failed = false +let failed = false; if (errors.length > 0) { - console.error(`\n✗ ${errors.length} registry lookup error(s) — failing closed:`) - for (const e of errors) console.error(` - ${e}`) - failed = true + console.error(`\n✗ ${errors.length} issue(s) — failing closed:`); + for (const e of errors) console.error(` - ${e}`); + failed = true; } if (violations.length > 0) { - console.error(`\n✗ ${violations.length} entries younger than ${MIN_AGE_DAYS} days:`) + console.error( + `\n✗ ${violations.length} entries younger than ${MIN_AGE_DAYS} days:`, + ); for (const v of violations) { - console.error(` ${v.name}@${v.version}`) - console.error(` published: ${v.publishedAt} (${v.ageDays} days ago)`) - console.error(` mergeable after: ${v.mergeAfter}`) + console.error(` ${v.name}@${v.version}`); + console.error(` published: ${v.publishedAt} (${v.ageDays} days ago)`); + console.error(` mergeable after: ${v.mergeAfter}`); } - const latestMergeAfter = violations.map((v) => v.mergeAfter).sort().at(-1) - console.error(`\nEarliest this PR can merge: ${latestMergeAfter}`) - failed = true + const latestMergeAfter = violations + .map((v) => v.mergeAfter) + .sort() + .at(-1); + console.error(`\nEarliest this PR can merge: ${latestMergeAfter}`); + failed = true; } if (failed) { - console.error(`\nTo bypass for an urgent security fix, add the 'lockfile-age-skip' label to the PR.`) - process.exit(1) + console.error( + `\nTo bypass for an urgent security fix, add the 'lockfile-age-skip' label to the PR.`, + ); + process.exit(1); } -console.log(`✓ All ${added.size} new entries ≥ ${MIN_AGE_DAYS} days old.`) +console.log(`✓ All ${added.size} new entries ≥ ${MIN_AGE_DAYS} days old.`);