feat(#630): extract progress-stream and late-exception primitives (phase 3)

[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 3 of 8. It moves streamLines(), parseExceptionText(),
findExceptionFrame(), and the StreamLine/StreamCallbacks/ProgressMetaColumn wire
types out of src/net/clickhouse-http-transport.ts/src/core/stream.ts into
packages/clickhouse-http/src/{progress-stream,exceptions}.ts — a real move+delete,
not an additive compatibility layer. StreamResult, row caps, progress percentages,
raw/result presentation, editor-caret positioning, and auth-expiry UI policy all remain
in SQL Browser, unmoved.

Claims: A6 (progress-stream decoding has one package implementation while SQL
Browser retains result/view state), A7 (ClickHouse HTTP exception parsing/late-exception
framing are package-owned and byte-safe). A8–A18 remain deferred.

Approved plan: see the ship-log comment on #630. Plan review again needed manual
recovery — the automated ChatGPT-author/Fable loop hit repeated timeouts on a very long
generation (not a Chrome crash this time); the completed response was read directly
from the DOM and Fable invoked directly, same recovery pattern as Phase 2. Fable
approved on the 2nd real review pass after 2 findings (a mis-cited enforcement
mechanism; a missed test-migration site) were incorporated and re-verified.

Contract coverage

Invariant	Enforcement	Proof	Sabotage
One production streamLines/parseExceptionText/findExceptionFrame owner	package-only declarations; no forwarding wrapper anywhere in the old transport	check-boundaries.mjs legacy-owner rule + mirror	reintroduced export interface StreamLine in core/stream.ts → caught by check:arch and the mirror (verified independently by both the coordinator and the pre-PR reviewer)
All compile-time wire-type consumers migrated	root StreamLine export removed	check:types over tests/unit/** + tests/spike/clickhouse-client/**	reverted one test's import → check:types fails (TS2305)
Byte-accurate exception boundary	findExceptionFrame computes offsets via a lossless latin1 byte↔char view, never TextDecoder, over the clean prefix	tests/unit/clickhouse-http-exceptions.test.ts invalid-UTF-8 + multibyte-message boundary cases	swapping to a real TextDecoder → 2 tests fail (garbled message, off-by-2 byte count)
Result-policy boundary held	StreamResult/row caps/percentages/parseErrorPos/auth-expiry policy stay in core/stream.ts, logic byte-for-byte unchanged	existing tests + diff inspection	—

Tests

• npm run check:types && npm run check:arch && npm run check:schemas && npm run check:examples && npm test && npm run build — green (7232 tests, 100%/97.14%/100%/100% coverage).
• npm run test:e2e -- tests/e2e/clickhouse-http-transport.spec.js --project=chromium --project=webkit — 18/18 passed; Scenario 6 now exercises the package's streamLines() through the real production call path.
• Full root e2e regression, --project=chromium --project=webkit — 432 passed, 4 skipped (pre-existing, unrelated).

Build

npm run build succeeds — single self-contained dist/sql.html, no new runtime dependency.

Invariant/sabotage verification

Verified three times: once by the implementer (who found and fixed a real bug in its
own sabotage-check tooling — a two-pass comment-stripping regex that let a /*-shaped
substring inside an unstripped // comment silently swallow real code, masking a
sabotage; fixed with a single alternation-based regex, commit 9ff449e), once by the
coordinator (full gate, direct reproduction of the StreamLine-reintroduction sabotage
post-fix, e2e), and once by a separate high-effort pre-PR review confirming the
production cutover is real (not forwarding), the result-policy boundary held with
unchanged logic, the byte-accurate boundary is genuinely byte-safe, the comment-stripper
fix is correct and applied identically in both files, and every compile-fallout site was
genuinely retargeted.

Two out-of-scope, pre-existing gaps surfaced during review were filed separately rather
than folded into this PR: #642 (a dynamic-import regex gap in check-boundaries.mjs,
found during Phase 2's review) and #643 (two other test files carrying the same unsound
two-pass comment-strip pattern this phase's own bug demonstrated, found during this
phase's pre-PR review).

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: no duplicate stream/exception implementation; result-policy boundary enforced by a sabotage-tested architecture rule
• No new runtime dependency
• 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, and .wiki pages updated; 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: 9ff449ebb15c9a95590697b96268a7c476f226b6

[P2 / acceptance blocker] The new legacy-owner guard still has false negatives around string literals

build/check-boundaries.mjs lines 366–368 strips comments with:

source.replace(/\/\*[\s\S]*?\*\/|\/\/.*/g, '')

The Phase 3 mirror in tests/unit/clickhouse-http-package-policy.test.js lines 299–301 uses the same algorithm. The single alternation fixes the specific two-pass // ... /* failure from 9ff449e, but it still cannot distinguish real comments from comment-shaped text inside JavaScript/TypeScript string literals.

I reproduced two false negatives with the exact regex:

const marker = "/*";
export interface StreamLine {}
const end = "*/";

The stripper reduces that to const marker = "";, swallowing the forbidden StreamLine declaration across lines. Likewise:

const u = "https://example"; export function streamLines() {}

is reduced to const u = "https:, swallowing the forbidden streamLines on the same line. In both cases the subsequent word-boundary test reports no violation. Because the mirror copies the same stripping algorithm, both enforcement paths miss the sabotage.

This is not #643's deferred pre-existing scope: that issue covers two other untouched two-pass helpers. This false-negative class is in the new Phase 3 guard itself, whose stated purpose is to mechanically enforce the one-production-owner invariant. The current production code does satisfy the invariant, but the PR's claimed regression proof does not.

Action: make this narrow rule token-aware (for example, a TypeScript scanner or a small lexical scanner that recognizes comments only outside strings/templates/regex literals), and add sabotage probes for at least the multiline "/*" ... "*/" case and the "https://..." same-line case so the production checker and its mirror cannot regress together.

Other adversarial checks were clean at this head: the transport has no stream forwarding member; runQuery calls the package streamLines directly; streamToFile passes retained bytes directly to package-owned findExceptionFrame; applyStreamLine's change is type-boundary-only with the same runtime branch/fold logic; the latin1 byte view keeps cleanBytes byte-exact even with multibyte UTF-8 immediately before the frame; the Chromium Scenario 6/8 fixture exercises the real package function; and the CHANGELOG entry matches the actual cutover. Canonical CI for the exact PR merge ref is green (including 7,232 unit tests and 218 Chromium e2e tests). I could not run the repository locally because this review runtime could not resolve github.com for a clone, but I independently executed the regex reproductions above in Node.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: 9ff449ebb15c9a95590697b96268a7c476f226b6
Reviewed head: ad792f39f5d615591ca1a759a3596ede75d07158

[P2 / acceptance blocker] The legacy-owner scanner still has a regex-literal false negative

The pass-1 string-literal bypasses are fixed: the new hand-rolled scanner correctly preserves /*, */, and // when they occur inside single/double-quoted strings and template literals, and the two reproduced probes were added to the mirror test.

However, the earlier finding is not fully resolved because the scanner still has no JavaScript/TypeScript regex-literal state. This was part of the pass-1 requested fix: comments must only be recognized outside strings/templates/regex literals.

I reproduced a remaining false negative against the exact new stripComments implementation with valid JavaScript/TypeScript:

const re = /\/\//; export function streamLines() {}

The regex literal /\/\// matches //. Character-by-character, its second escaped slash is immediately followed by the regex literal's closing slash. The new scanner sees those two slash characters as a // line-comment opener because it does not know it is inside a regex literal. It therefore reduces the probe to:

const re = /\/\

and the subsequent \bstreamLines\b check reports no violation. The test-file mirror copies the same scanner, so it misses the same sabotage.

This is acceptance-relevant for the same reason as pass 1: the current production tree still satisfies the one-owner invariant, but the new Phase 3 architecture gate that claims to mechanically enforce that invariant remains bypassable by syntactically valid source. The fix commit only changed build/check-boundaries.mjs and tests/unit/clickhouse-http-package-policy.test.js; all runtime extraction files are unchanged from the prior reviewed head.

Action: use a token-aware scanner/parser that understands regex literals (the TypeScript scanner is the simplest robust option here), or complete the lexical scanner with correct regex-vs-division recognition. Add the probe above to both the production-checker sabotage coverage and the mirror's regression cases.

Reassessment of pass-1 checks

• Pass-1 blocker: partially fixed, still open due to the regex-literal bypass above.
• Runtime ownership/cutover: unchanged from 9ff449e; no new forwarding wrapper or moved-symbol re-declaration was introduced.
• applyStreamLine behavior: unchanged from the prior reviewed head; the Record<string, unknown> change remains type-boundary-only.
• Byte-accurate exception framing: unchanged from the prior reviewed head; no new encoding/offset regression introduced.
• Browser Scenario 6/8 production path: unchanged from the prior reviewed head.
• CHANGELOG/runtime documentation: unchanged from the prior reviewed head and still matches the runtime cutover.

Canonical CI for the new head's merge ref is green: check:arch, check:types, build, and 7,234 unit tests passed with 100%/97.14%/100%/100% coverage; Chromium e2e also passed. Those green results do not cover the regex-literal sabotage above.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed head: ad792f39f5d615591ca1a759a3596ede75d07158
Reviewed head: 8e9af149a494425fd58611b6e2d2694e74ae3d8f

[P2 / acceptance blocker] The regex-aware legacy-owner scanner is still bypassable after control-flow )

The pass-2 exact bypass is fixed: the new regexAllowedAfter/regex-literal consumer correctly handles the previously reported top-level const re = /\/\//; export function streamLines() {} case, and the mirror now carries that sabotage plus a normal read / total division case.

However, regexAllowedAfter unconditionally returns false after every closing ). That is not sufficient JavaScript/TypeScript lexical context: after a control-flow condition closes, a regex literal can legally begin the controlled expression statement.

I reproduced a remaining false negative against the exact new stripComments implementation with valid module syntax:

if (true) /\//.test("/"); export function streamLines() {}

node --check accepts that source. At the regex-opening /, the scanner sees the preceding ) and classifies the slash as division rather than a regex literal. It then reaches the escaped slash immediately followed by the regex closing slash, sees those two slash characters as a // line-comment opener, and reduces the probe to approximately:

if (true) /\

The subsequent \bstreamLines\b check therefore reports no violation. The same reproduction works after while (...). Because tests/unit/clickhouse-http-package-policy.test.js mirrors the same heuristic, both enforcement paths miss this sabotage.

This is the same acceptance-relevant class as passes 1 and 2: today's production tree still satisfies the one-owner invariant, but the Phase 3 gate advertised as its mechanical regression proof remains bypassable by syntactically valid source. The current fix commit only changes build/check-boundaries.mjs and tests/unit/clickhouse-http-package-policy.test.js; it introduces no runtime extraction regression.

Action: stop approximating JavaScript regex-vs-division grammar with last-character heuristics. Use a real tokenizer/scanner (for example the TypeScript scanner) for this narrow check, or otherwise model control-flow statement context correctly. Add at least the if (true) /\//...streamLines sabotage above to regression coverage. A matching while (...) case is also useful because it exercises the same grammar class independently.

Reassessment of earlier/runtime findings

• Pass-2 blocker: partially fixed, still open through the control-flow regex case above.
• Ownership/cutover: unchanged from ad792f3; the old HTTP transport is still send-only, with no stream forwarding member.
• runQuery: still calls package streamLines directly.
• streamToFile: still passes retained Uint8Array bytes directly to package-owned findExceptionFrame through the zero-logic ch-client.ts re-export.
• applyStreamLine: runtime branch/fold logic remains unchanged; the Record<string, unknown> change is still only the type boundary.
• Byte-accurate exception framing: unchanged; the latin1 byte-view keeps string indexes byte-identical and only decodes the matched message after framing.
• Browser Scenarios 6/8: still exercise the real package streamLines with the production transport send path.
• CHANGELOG: still matches the actual runtime cutover.

Canonical CI for the exact new merge ref is green: check:arch, check:types, build, and 7,236 unit tests passed with 100%/97.14%/100%/100% coverage; the e2e job also passed. Those tests do not cover the control-flow regex bypass above.

VERDICT: REVISE

[BorisTyshkevich]

I think we should stop extending the hand-written JS lexer in build/check-boundaries.mjs and simplify the Phase 3 ownership guard.

The last three review passes all found valid bypasses in the same mechanism:

• strings/templates containing comment markers
• regex literals containing //
• regex literals after control-flow ) where / is not division

Each individual fix was reasonable, but together they show that the checker is gradually reimplementing JavaScript lexical grammar. That complexity is unrelated to @altinity/clickhouse-http itself; it exists only because the architecture check is trying to strip comments and then perform textual matching.

Please replace that mechanism with the following simpler approach.

Required implementation

1. Delete the hand-written source scanner
   Remove the comment/string/template/regex parsing machinery used by the Phase 3 legacy-owner rule, including things such as:

   • stripComments
   • regexAllowedAfter
   • regex-literal parsing
   • template/string state
   • parenthesis/control-flow tracking
   • any equivalent duplicated implementation in the unit test

2. Use the existing typescript devDependency in the build checker only
   Parse the relevant legacy-owner source files with ts.createSourceFile().

   This must remain build/test tooling only. Do not add typescript or any parser dependency to @altinity/clickhouse-http or its runtime dependency graph.

3. Scope the rule narrowly to the Phase 3 invariant
   We only need to detect whether the former SQL Browser owners regain the moved primitives.

   Walk the TypeScript AST and reject exact identifier/property declarations or references for the moved names where they are forbidden, including as applicable:

   • streamLines
   • parseExceptionText
   • findExceptionFrame
   • StreamLine
   • StreamCallbacks
   • ProgressMetaColumn
   • ExceptionFrame
   • any other symbol explicitly moved by Phase 3

   Keep the existing allowlist/file scope narrow. This is not intended to become a generic static-analysis framework.

4. Make forwarding wrappers fail too
   The rule should catch not only a second implementation, but also attempts to restore the old ownership surface through forwarding, for example:

   import { streamLines } from '@altinity/clickhouse-http';
   
   return { streamLines };
   
   async streamLines(...) {
     return packageStreamLines(...);
   }

   Exact AST identifier/property checks are sufficient for normal accidental regressions.

   Do not try to detect intentionally obfuscated constructs such as computed strings or dynamically constructed property names. That is outside the threat model for this architecture check.

5. Do not duplicate the checker implementation in tests
   Extract the ownership check into one pure helper, for example:

   build/lib/check-legacy-owners.mjs
     findLegacyOwnerViolations(source, filename)
   
   build/check-boundaries.mjs
     imports/calls helper
   
   tests/unit/clickhouse-http-package-policy.test.js
     imports/calls the same helper
   

   The tests should exercise the contract, not maintain a second copy of the parsing algorithm.

6. Keep all prior sabotage cases
   Preserve regression coverage for all three review findings so that the replacement is demonstrably immune to them:

   const marker = "/*"; export interface StreamLine {} const end = "*/";

   const u = "https://example"; export function streamLines() {}

   const re = /\/\//; export function streamLines() {}

   if (true) /\//.test("/"); export function streamLines() {}

   Also keep at least one ordinary division case to prove the new check does not depend on regex-vs-division classification.

7. Keep the existing production behavior unchanged
   This change is only to the architecture enforcement mechanism.

   Do not alter the already-verified Phase 3 runtime cutover:

   • transport remains send-only
   • runQuery calls package streamLines directly
   • export passes retained Uint8Array bytes directly to package findExceptionFrame
   • result policy remains local
   • applyStreamLine behavior remains unchanged
   • Browser Scenarios 6 and 8 continue using the actual package streamLines
   • no duplicate or forwarding implementation is introduced

Motivation

The invariant we need to enforce is simple: these moved primitives must have one owner and must not silently reappear in their previous SQL Browser files.

A custom lexer is a poor fit for that invariant. Correctly distinguishing comments, strings, templates, regex literals, division, and statement/expression contexts requires essentially reproducing part of the JavaScript grammar. The three review passes have already demonstrated that this produces an open-ended sequence of edge cases.

Using the repository's existing TypeScript parser gives us the lexical correctness for free and keeps the policy check small and reviewable. It does not make the network library depend on a parser: TypeScript is used only by development/CI tooling to inspect TypeScript source.

The desired outcome is therefore fewer lines of custom parsing code, one implementation of the ownership check, and a narrowly scoped mechanical guard that directly expresses the Phase 3 architectural invariant.