feat(#630): make @altinity/clickhouse-http standalone-buildable, retire client-web vendor spike (phase 8)

[BorisTyshkevich]

Summary

Phase 8 of #630 — the final phase of this 8-phase epic ("Epic: extract the
Fetch-native ClickHouse client"). Makes packages/clickhouse-http independently
buildable, packable, and importable outside this repository with no fallback into
SQL Browser source, then retires the @clickhouse/client-web vendor dependency and
its executable spike-tooling wiring (the migration scaffolding this whole epic has
been working toward removing).

• packages/clickhouse-http now has its own build/test/check:types scripts,
  produces built browser-first ESM + TypeScript declarations into dist/, and its
  package.json carries publication-shaped exports/files/license/version
  metadata — exports["."] now points at dist/index.js, never TypeScript source.
• Root build/build.mjs (esbuild bundling src/main.ts) now consumes the package via
  its built dist/**, not source — verified via the real esbuild metafile (only
  dist/*.js inputs, zero src/**.ts/@clickhouse/client-web inputs) and
  build/size-report-lib.mjs's bucket attribution (still project, unchanged).
  build/bundle.sh and deploy/install.sh — both of which call node build/build.mjs
  directly, bypassing root npm scripts — now build the package first, verified from a
  clean dist/ state.
• A real isolated-package proof (npm pack → install into a temp fixture outside
  the workspace tree → import as ESM → compile a TypeScript consumer against the
  declarations → tsc --traceResolution proves no source fallback) is wired into
  npm run test:clickhouse-http:pack and CI.
• tests/spike/clickhouse-client/** (33 files, the @clickhouse/client-web
  vendor-comparison spike from ADR-0005: adopt @clickhouse/client-web behind the SQL Browser transport adapter #585) is retired: historical evidence
  (docs/evidence/585/**, ADR-0005's Rejected decision) is byte-untouched; reusable
  generic fault scenarios became the package's own first-party Chromium+WebKit
  regression suite (packages/clickhouse-http/test/browser/**); the rest is deleted
  with zero dangling references. @clickhouse/client-web (1.23.1) is removed from
  the root manifest and lockfile.
• 5 new/extended architecture guards (package→SQL-Browser imports; SQL-Browser
  package-deep-imports including dist/**; regrowth of a second generic
  request/URL implementation; regrowth of duplicate progress/late-exception parsers;
  @clickhouse/client-web reintroduction) — all built on the existing real-parser
  mechanism in build/lib/check-legacy-owners.mjs, no new hand-rolled scanner.
• .github/workflows/ci.yml: pack-test added to the test job; PR browser-install
  widened to Chromium+WebKit for the package's own new suite only (root e2e's
  Chromium-only-on-PR policy is unchanged); the retired fault-server.mjs path-filter
  entry removed.
• New docs/clickhouse-http-repository-extraction.md documents the tested mechanical
  handoff for follow-up: move @altinity/clickhouse-http into a dedicated Altinity repository #639 — every command it lists is a command this PR's own test suite
  actually runs.
• docs/ARCHITECTURE.md, CLAUDE.md, .wiki/*, package README, and a narrow
  ADR-0005 current-state addendum (historical content/dates preserved) all reconciled.

Claims A17 (package builds/packs/installs/imports/typechecks in isolation with no
root-source fallback) and A18 (final architecture guards/docs reconciled, obsolete
@clickhouse/client-web wiring removed, #639 has a tested extraction handoff). A1-A16
already shipped across Phases 1-7.

At Phase 8 completion, issue #630 itself is complete. The package still lives in
this repository — #639 (creating a separate repository, publishing, and the SQL
Browser consumer cutover to the released package) remains explicitly out of scope,
exactly as the issue separates the two.

Pre-PR review

One high-effort, read-only pass over the complete 86-file diff — every load-bearing
claim in the plan's A17/A18 definition of done was checked against actual execution
(clean-state builds, the real esbuild metafile, a genuine external-install proof, the
browser suite, byte-diffing historical evidence), not taken on faith. Found 2 minor
test-design gaps (both fixed): Guards 1/3/4 lacked the same production-drift sentinel
this repo's existing Rules A-D/Guard 5 already use, and Guard 5's structural
manifest/lock/script check had 3 sabotage tests re-deriving its own logic rather than
calling the real predicate — fixed by extracting that predicate into an exported,
shared function (build/lib/check-legacy-owners.mjs) that build/check-boundaries.mjs
now calls directly, eliminating the duplicate-logic risk entirely rather than just
detecting drift after the fact. Both fixes verified via real sabotage-of-production-code
probes, confirmed to fail for the right reason, then restored.

Gate

npm run check:types                     ✅
npm run check:arch                       ✅ (1670 files, 11 rules, 0 violations; sabotage-verified live)
npm run check:schemas                    ✅
npm run check:examples                   ✅
npm test                                  ✅ 100/97.05/100/100 root coverage; 100/100/100/100 package coverage
npm run build                              ✅ dist/sql.html built
npm run test:clickhouse-http:pack          ✅ isolated-package: OK
npm run test:clickhouse-http:browser       ✅ 20/20, Chromium + WebKit
npm run test:e2e (full suite)              ✅ 215/215 Chromium; 211/211 WebKit (4 pre-existing skips)

Closes #630.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz

[BorisTyshkevich]

ChatGPT review pass 2

Reviewed head: f73f59c7d19e80f1e5f61423bb6ab14b0268f20f
Previously reviewed head: d81a672d01e6dbea909ec8c5e6cfdbc5491e0d9f

The head-to-head delta is only the deploy/install.sh SC2155 cleanup (local tbl + separate assignment). I re-audited the complete Phase-8 surface rather than limiting the review to that one-line semantic delta.

Finding — P2: A18's new/extended guard set is not actually parser-authoritative in two places

1. Guard 2 (src/** must not relatively deep-import any packages/clickhouse-http/**, including generated dist/**) still runs through the legacy regex extractSpecifiers. build/check-boundaries.mjs broadens Rule C's forbidden target to the whole package, but the generic RULES loop obtains specifiers from extractSpecifiers, whose four regexes are explicitly the older lexical scanner. tests/unit/clickhouse-http-package-policy.test.js's Phase-8 Rule-C sabotage also goes through its own relativeViolations/extractSpecifiers regex mirror. So the new dist/** guard inherits the exact lexical-bypass class this issue repeatedly moved away from: e.g. a valid import such as import { chUrl } from/*trivia*/'../../packages/clickhouse-http/dist/index.js' (or an escaped string-literal segment) is parsed/resolved by TS/esbuild but is not extracted by that regex path. Both production and the mirror test miss the same case.

   Fix: make Phase-8 Guard 2 a parser-backed check using the existing findModuleSpecifiers(source, relFile) (preferably a dedicated shared helper/predicate rather than widening all legacy RULES at once), resolve the decoded spec, and remove the Phase-8 Rule-C test's dependence on the local regex mirror. Add sabotage for comment trivia and an escaped specifier into dist/**.

2. Guard 5 calls the real parser only after an unsound raw-substring prefilter, and its tests do not bind that production prefilter. Production currently does if (!source.includes(CLIENT_WEB_SPECIFIER)) continue; before findModuleSpecifiers. The same repository already documents in mightReferencePackage why an exact raw-substring gate is insufficient: string-literal escapes decode to the target specifier even when the raw source does not contain it. client-web-retirement-policy.test.js repeats the raw source.includes(...) gate for its real-tree scan, while its sabotage cases call findModuleSpecifiers directly and therefore skip the production prefilter entirely. Also, despite the Guard-1/3/4 drift-bind comment referring to a Guard-5 equivalent, the current retirement-policy file has no checkerSource binding for this production source-scan block.

   Fix: gate Guard 5 with the already-shared escape-aware mightReferencePackage(source, CLIENT_WEB_SPECIFIER) (or export one complete Guard-5 source predicate), and add a sabotage that exercises the real prefilter+parser composition plus a production-wiring sentinel if the checker remains a top-level script.

This is acceptance-relevant rather than stylistic: the PR explicitly claims all five new/extended Phase-8 architecture guards use the existing real-parser mechanism, and A18 is the final architecture-hardening criterion.

Earlier findings / regression re-check

• Guards 1/3/4 now have the production-text drift bind added in d81a672...; confirmed present.
• Guard 5's manifest/lock/script predicates are shared exports in build/lib/check-legacy-owners.mjs and production calls those shared predicates; the earlier duplicate-logic finding is fixed.
• The current f73f59c... SC2155 change is behavior-neutral and preserves deploy/install.sh's package-build-before-app-build ordering.
• Both bypass production paths (build/bundle.sh, deploy/install.sh) explicitly build @altinity/clickhouse-http before build/build.mjs.
• Package exports point to dist/index.js/dist/index.d.ts; root build/type/test scripts establish the package build prerequisite.
• test/isolated-package.mjs performs a real npm pack, installs under OS temp outside the workspace with NODE_PATH cleared, runs an ESM consumer with exact import.meta.resolve, compiles a NodeNext TypeScript consumer, checks resolution reaches installed dist/index.d.ts, and cleans up.
• The package browser harness serves/imports /dist/index.js directly and the regression suite owns scenarios 1–9 plus invalid-UTF8 fidelity on Chromium/WebKit; root auth/export suites consume the moved fault server from the package test tree.
• @clickhouse/client-web is absent from the current root manifest and removed from the lockfile; executable spike scripts/tree are retired. The ADR change is a narrow Phase-8 addendum rather than a rewrite of the historical decision block.
• CI keeps root PR e2e Chromium-only while installing WebKit for the package's C/W suite, adds the isolated-pack proof, and removes the old spike fixture path filter. The current-head CI run is green.
• build/size-report-lib.mjs continues to classify all packages/** inputs as first-party project, so package dist/** does not become an external bucket.

No other actionable regression found in the updated PR.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Reviewed head: 7f89ff879340e9b41f39de43f6db981accc574ff
Previously reviewed head: f73f59c7d19e80f1e5f61423bb6ab14b0268f20f

The new head is two commits ahead of the previous review. The delta is confined to four architecture-policy/test files: build/check-boundaries.mjs, build/lib/check-legacy-owners.mjs, tests/unit/clickhouse-http-package-policy.test.js, and tests/unit/client-web-retirement-policy.test.js. I re-used the prior full-PR audit for byte-unchanged surfaces and re-audited the complete current Phase-8 contract around the new guard changes. Current-head CI is green, including test/pack/build, package Chromium+WebKit, root e2e, bundle/shellcheck, size, docker-smoke, and the final gate.

Finding 1 — P2: pass-2 Guard 5 soundness finding is only partially fixed

The production-wiring/drift part is fixed: Guard 5 and its test now share mightReferenceRetiredTopLevelApi, the sabotage helper exercises prefilter → findModuleSpecifiers, and a checkerSource sentinel pins production wiring.

But the actual false-negative from pass 2 remains. mightReferenceRetiredTopLevelApi is a raw source.includes(name) prefilter and its own doc explicitly says it does not widen for backslash-escaped spellings. That convention is defensible for deliberately obfuscated identifiers, which is what the helper was designed for, but Guard 5 is feeding it a module-specifier string (@clickhouse/client-web). This repository already has mightReferencePackage precisely because raw-substring prefilters are unsound for module specifiers: valid string literals can encode a character with \xNN/\uNNNN, the TypeScript parser decodes node.text, but a raw source.includes(CLIENT_WEB_SPECIFIER)-equivalent gate never invokes the parser.

Concrete bypass:

import { createClient } from '@clickhouse/client-w\x65b';

The module specifier decodes to exactly @clickhouse/client-web; findModuleSpecifiers would report it if called, but the current Guard-5 prefilter returns false first. The new tests share the same prefilter and do not include an escaped-specifier sabotage, so they now eliminate drift without eliminating this correctness hole.

Fix: use the existing escape-aware mightReferencePackage(source, CLIENT_WEB_SPECIFIER) (or an equivalently sound shared package-specifier prefilter), keep the current production-wiring sentinel, and add a full-pipeline sabotage using an escaped vendor specifier.

Finding 2 — P2: Guard 2 can still deep-import the workspace package through its node_modules link

The regex/parser issue from pass 2 is fixed: Rule C/Guard 2 is now a dedicated findModuleSpecifiers path, with comment-trivia and escaped-path sabotage, and the follow-up prefilter preserves those cases while fixing the CI timeout.

However, the guard compares the lexical result of resolveRelative() only against packages/clickhouse-http/**. This repository's lockfile explicitly declares node_modules/@altinity/clickhouse-http as link: true, resolved to packages/clickhouse-http. A normal relative import can therefore bypass the public-export-only invariant through that workspace link:

import { chUrl } from '../../node_modules/@altinity/clickhouse-http/src/index.js';

From src/net/**, the new prefilter runs (the source contains clickhouse-http) and the real parser sees the relative specifier, but resolveRelative() returns a lexical node_modules/@altinity/clickhouse-http/... path. The subsequent check only accepts relResolved === 'packages/clickhouse-http' or that prefix, so it does not flag the import. Rule D does not catch it either because the specifier is relative rather than the bare @altinity/clickhouse-http/... form. esbuild/TypeScript can follow the workspace link, so this is a real deep import of package internals while bypassing the package's "." export.

Fix: make Guard 2 compare canonical package identity, not only lexical repo paths. For existing targets, resolve symlinks before the forbidden-prefix comparison; also explicitly treat relative resolution under node_modules/@altinity/clickhouse-http/** as the workspace package so the guard remains fail-closed even when generated dist/** has not been built yet. Add a sabotage import through the workspace link.

Earlier findings / full-PR regression re-check

• Pass-2 Guard 2 lexical-scanner finding: fixed; the dedicated Guard-2 path is parser-backed and has comment/escape sabotage plus production drift binding.
• Guard-2 follow-up CI timeout: fixed by the shared escape-aware forbidden-directory prefilter; current-head CI is green.
• Pass-2 Guard 5 production-drift finding: fixed; only the raw-substring soundness half above remains.
• Pre-PR Guards 1/3/4 drift sentinels and shared Guard-5 manifest/lock/script predicates remain present.
• The two new commits do not touch the package build/export surface, isolated-package proof, browser suite, root build wrappers, CI policy, vendor deletion, docs/evidence, or migration aliases. Those surfaces are byte-identical to the previously reviewed head and retain the prior audit result.
• docs/evidence/585/** is still absent from the PR changed-file set; historical evidence remains untouched.
• packages/clickhouse-http/package.json still targets built dist/index.js/dist/index.d.ts, retains private: true/0.0.0, and has no runtime dependency map.

No additional runtime/package regression found beyond the two architecture-boundary gaps above.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3 (manual continuation — CLI pass cap hit)

The CLI's own chatgpt-review pr call for pass 3 was hard-rejected before reaching
ChatGPT ("PR review sessions permit at most three total passes" — an earlier pass
needed an internal retry that consumed an extra session slot). Continued the same
conversation manually instead of starting a fresh session, per this session's own
"one unit, one ChatGPT conversation" convention.

Review scope: diff from 7f89ff8 (pass 2's reviewed head) to 2df6beb (current
head) — 3 changed files: build/check-boundaries.mjs,
tests/unit/clickhouse-http-package-policy.test.js,
tests/unit/client-web-retirement-policy.test.js.

Finding 1 (Guard 5 escaped-specifier bypass) — genuinely resolved. Production now
gates on the escape-aware mightReferencePackage; the new sabotage test proves the old
raw-substring false negative is gone; a drift sentinel pins the production wiring.

Finding 2 (Guard 2 node_modules workspace-symlink bypass) — genuinely resolved.
resolveRelative() now canonicalizes via fs.realpathSync() before the prefix
comparison; the sabotage test uses the real, already-installed symlink (not a virtual
fixture) and would fail against the old lexical-only implementation; a sentinel pins
fs.realpathSync in the production function.

2df6beb (unrelated hook-timeout widening) — confirmed timeout-only, no weakened
assertions or skipped work; current-head CI green. One non-blocking precision note:
fs.realpathSync() adds bounded filesystem work during cache warming, so "unchanged
work" wasn't perfectly literal — doesn't affect the conclusion.

No remaining actionable defect found in this delta.

VERDICT: SHIP

Conversation: https://chatgpt.com/c/6a775c43-16a8-83eb-98a2-105a5570df66