feat(#630): create @altinity/clickhouse-http, move low-level request/URL (phase 2)

[BorisTyshkevich]

What & why

Part of #630 — replacing the rejected @clickhouse/client-web adoption path (#585) with
a first-party, Fetch-native @altinity/clickhouse-http package extracted from generic
protocol code SQL Browser already maintains.

This is Phase 2 of 8. It creates the repository's first npm workspace,
packages/clickhouse-http, and moves chUrl()/URL serialization plus the low-level
Fetch request into it. src/net/clickhouse-http-transport.ts's send() becomes a thin
delegating compatibility adapter; stream reading (streamLines()) and all SQL Browser
auth/epoch/lifecycle/retry policy stay local until later phases.

Claims: A2 (@altinity/clickhouse-http exists in-repo with no SQL Browser source
imports), A4 (SQL Browser consumes the package through its public export), A5 (URL
serialization has exactly one implementation, owned by the package). A6–A18 remain
deferred to later phases.

Approved plan: see the ship-log comment on #630. Plan review took an unusual path this
round — Agent Chrome disconnected mid-loop during the automated ChatGPT-author/Fable
loop; the recovery (reading the completed ChatGPT response directly from the DOM and
invoking Fable directly, bypassing the chatgpt-review skill/Playwright) is documented
in full in the ship-log's Phase 2 handoff. Fable/high approved on the 3rd real review
pass after 3 findings were incorporated (a Docker build break, a zero-runtime-deps
enforcement gap, and an async/sync settlement drift — all three verified fixed by an
independent Fable re-review).

Contract coverage

Invariant	Enforcement	Proof	Sabotage
Package→root-src ban	check-boundaries.mjs Rule A	policy mirror test	client.ts importing ../../../src/net/ch-client.js → caught
Root→package-internals ban	check-boundaries.mjs Rule C	policy mirror test	root file deep-importing package internals → caught
Zero bare specifiers in package source	dedicated scan (closes the generic RULES loop's bare-specifier blind spot)	policy mirror test	url.ts importing @preact/signals-core → caught by check:arch (verified independently)
Bare @altinity/clickhouse-http restricted to src/net/**	dedicated scan	policy mirror test	bare import from src/core/** → caught
Docker workspace reproducibility	Dockerfile COPY packages ./packages before npm ci	real docker build	removing the line → esbuild "Could not resolve @altinity/clickhouse-http" (verified independently, legacy builder — buildx blocked in this sandbox)
Nine Phase-1 transport invariants, direct	contract suite registered against package's own request()	tests/unit/clickhouse-http-package.test.ts	(Phase 1's existing sabotage cases, now proven twice: package + adapter)
Single serializer (A5)	one function chUrl( definition, in the package	grep across src/**+packages/**	—
Promise settlement	request()/send() both stay async, preserving today's rejected-promise contract for malformed URLs	focused test in both files	—

Tests

• npm run check:types && npm run check:arch && npm run check:schemas && npm run check:examples && npm test && npm run build — green (100%/97.14%/100%/100% coverage, including the 3 new package files, verified via lcov).
• npm ci --no-audit --no-fund — green; node_modules/@altinity/clickhouse-http resolves as a real local workspace symlink, not a registry package.
• npm run test:e2e -- tests/e2e/clickhouse-http-transport.spec.js --project=chromium --project=webkit — 18/18 passed, now through the package-backed compatibility adapter.
• Full root e2e regression, --project=chromium --project=webkit — 432 passed, 4 skipped (pre-existing, unrelated).
• Real docker build (legacy non-buildx driver — buildx blocked in this sandbox) — succeeds; independently re-verified it fails exactly as expected when the COPY packages ./packages fix is removed.
• 8 raw-ESM e2e fixtures reaching the transport gained an import-map entry for the new bare specifier; the full fixture graph was audited twice independently (once via an esbuild-transform-accurate script correcting for import type erasure, once by a separate reviewer) confirming no additional fixture needs one.

Build

npm run build succeeds — single self-contained dist/sql.html, no new runtime
dependency (the workspace package is first-party project code, not a third-party
dependency — CLAUDE.md hard rule 4 clarified accordingly).

Invariant/sabotage verification

Verified three times: once by the implementer, once by the coordinator directly
(diff footprint, npm ci linkage, full gate, lcov coverage, the bare-specifier and
Docker sabotages, e2e), and once by a separate targeted read-only review covering the
remaining invariant-map items (import-ban sabotages, move-not-duplicate test claims,
ch-client.ts composition, streamLines() locality, single-serializer invariant,
direct-package contract registration) — no defects found in any pass.

Checklist

• npm test passes (the per-file coverage gate is non-negotiable)
• Tests added/updated in the same change as the code
• npm run build succeeds (single-file dist/sql.html)
• Layers kept honest: package boundary enforced by 4 new architecture rules, each with a sabotage-tested mirror
• No new runtime dependency (first-party workspace package; CLAUDE.md hard rule 4 clarified)
• CHANGELOG.md ([Unreleased]) updated
• Reconciled affected tracked work — ship-log comment on Epic: extract the Fetch-native ClickHouse client into a reusable package #630 updated; CLAUDE.md, docs/ARCHITECTURE.md, ADR-0005 addendum, and .wiki pages updated per the plan's Checkpoint 3; the issue's own ## Phases checklist is ticked only after merge is verified on origin/main, per its explicit stated convention

Part of #630.

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: 62f018365f10d194e4565c625e02b1afa215510b

Required change — Major: the architecture scanner does not fully enforce the claimed dynamic-import boundary

build/check-boundaries.mjs uses one regex-based extractSpecifiers() for Rules A–D. Its dynamic-import pattern accepts only single- or double-quoted arguments (import('...') / import("...")). A valid no-substitution template-literal import such as:

import(`@preact/signals-core`)

produces no extracted specifier, so Rule B does not report the bare package import. The same bypass applies to the other rules when the template-literal target is a forbidden relative root/deep path or @altinity/clickhouse-http outside src/net/**. I reproduced this directly against the exact regex in this head: static imports, export * from, named re-exports, and quoted dynamic imports are detected; template-literal dynamic imports (and dynamic imports with a comment before the literal) are not.

The policy mirror copies the same four regexes, so it shares the blind spot rather than independently catching it. Its sabotage probes currently use static imports; they do not pin the dynamic/re-export forms named by the acceptance question.

Action: make dynamic-import handling fail closed. At minimum recognize no-substitution template literals; stronger is to reject any import(...) in guarded package source unless its argument is a statically provable relative literal. Add explicit sabotage cases for import('@preact/signals-core'), import(`@preact/signals-core`), export * from '@preact/signals-core', and named/namespace re-exports. Do the equivalent location/deep-import probes for Rule D.

This is an acceptance blocker even though the current package tree is clean: the PR claims the zero-bare-specifier/package-boundary rules are mechanically enforced and sabotage-tested. A valid JavaScript spelling that bypasses both the production gate and its mirror makes that enforcement claim false.

Other adversarial checks

No additional blocking defect found. The new chUrl() body is behaviorally identical to the base implementation, including zero/empty values, reserved-character encoding, settings-before-params ordering, and insertion order; the exact-literal suite is moved into the package test rather than duplicated. request() and compatibility send() both remain async, both have focused malformed-URL rejected-promise tests, and the production transport.send(...) path is awaited inside authedFetch; eager chUrl() validation still happens before getToken().

Checkpoint 1 (5c21cb770bfa23d3b6c33839b3f454923e67fa7c) is structurally isolated: the workspace package is statement-free, production does not import it until Checkpoint 2, and the Dockerfile already copies packages/ before npm ci. I found no code-level reason that checkpoint should fail the stated gate, but GitHub has no Actions run recorded for that intermediate SHA, so I cannot independently certify the historical “full gate + real docker build” claim from CI evidence.

The raw-ESM footprint is consistent with the stated eight fixtures: seven fixtures load /src/ui/app.js, and the transport-specific fixture imports clickhouse-http-transport.js directly; all eight are patched. Current canonical head CI is green, including the PR Chromium e2e job (218/218), docker-smoke, unit/coverage, build, bundle, and size jobs. The extra local WebKit claims in the PR description were not independently rerun in this sandbox.

streamLines() remains SQL-Browser-local and the auth/epoch/lifecycle/retry policy remains in ch-client.ts; the package owns only URL serialization, request types/deps, and the low-level injected Fetch call. I found no duplicate chUrl() implementation or moved responsibility still executing in its former owner.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: 62f018365f10d194e4565c625e02b1afa215510b
Current canonical head: 62f018365f10d194e4565c625e02b1afa215510b

There is no code delta between review passes: GitHub reports the two SHAs as identical (0 commits, 0 changed files). This pass therefore reassesses the pass-1 finding against repository history/scope and re-checks the complete unchanged PR for regressions.

Pass-1 finding reassessment

Dynamic-import scanner gap — real, but not a merge blocker for this PR. I re-confirmed the gap against the exact current regex: static imports, export * from, named re-exports, and quoted import('...') are detected; no-substitution template-literal dynamic imports and comment-prefixed dynamic-import arguments are not. However, commit f16e4a1a10a603b188453f8612a4b192ad58a6de introduced build/check-boundaries.mjs with this same regex and the explicit design assumption that the codebase has no exotic import syntax. Comparing PR base 20f2b5979544e05e8f9c02c8e40b8ec048562fce to the reviewed head shows SPECIFIER_PATTERNS is unchanged byte-for-byte; #641 adds Rules A/C and the Rule B/D consumers, not the parser limitation itself. The three package source files use only relative static imports/re-exports and contain no live bypass. Under the repository's existing checker contract, this is a cross-cutting follow-up rather than a regression introduced by #641.

The strongest objection is that Rule B's prospective "zero bare specifiers" promise is new even if its parser helper is old. If the DoD required language-complete parsing of every valid JavaScript import spelling, the pass-1 blocker would stand. I do not read A2/A4/A5 or this repository's explicitly regex-scoped checker contract that way: the required current package boundary is clean, and this PR does not introduce or rely on an unsupported dynamic-import spelling.

Checkpoint-1 CI evidence — not a defect. Checkpoint 1 (5c21cb770bfa23d3b6c33839b3f454923e67fa7c) is structurally inert: its package entry point is export {}, production does not consume the package until Checkpoint 2, and its Dockerfile already copies packages/ before npm ci. The absence of an isolated GitHub Actions run for an intermediate commit in a multi-commit PR is not evidence of a failing checkpoint.

WebKit reproduction scope — not a defect. The repository's PR workflow explicitly runs Chromium-only Playwright on pull requests; WebKit is part of nightly/manual/release coverage, not the PR gate. The required head CI run is green.

Complete PR regression re-check

No new regression was introduced between passes because the head did not move. Re-inspection of the full 34-file PR still finds no additional blocker:

• packages/clickhouse-http has no runtime dependencies and exposes only .; its source currently contains only relative imports/re-exports.
• chUrl() remains the single serializer implementation and is mechanically identical to the base behavior for zero/empty values, reserved-character encoding, settings-before-params ordering, and insertion order. The exact-literal suite is moved to the package test rather than duplicated.
• Package request() and compatibility send() remain async; focused tests prove malformed URL serialization rejects a returned promise without synchronous throw. Production authedFetch() awaits transport.send(...), while eager chUrl() validation still executes before getToken().
• ChCtx -> transportFor -> createHttpTransport -> send remains structurally intact; no credential/refresh hook or new ChCtx package field appears.
• streamLines() remains SQL-Browser-local; auth/epoch/lifecycle/retry policy remains in ch-client.ts.
• The eight raw-ESM transport-reaching fixtures are the same eight patched fixtures reviewed in pass 1.
• Canonical head CI is green across unit/coverage+build, bundle, size, docker-smoke, e2e, and the aggregate CI gate.

No merge-blocking finding remains for the claimed Phase-2 acceptance subset.

VERDICT: SHIP