feat(#630): add consuming query APIs, ClickHouseError, stateless KILL QUERY (phase 4)

[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 4 of 8. It's purely additive to packages/clickhouse-http:
ensureClickHouseSuccess, consumeJsonResponse/consumeTextResponse/
consumeProgressResponse, queryJson/queryText/queryProgress convenience methods,
a minimal ClickHouseError, and a stateless killQuery. Zero SQL Browser production
files changed — nothing consumes these new APIs yet; that cutover is Phase 7.

Claims: A8 (explicit non-consuming/consuming response APIs with a minimal
ClickHouse error model), A9 (stateless package KILL QUERY without credential
lookup/retry/registry ownership). A10–A18 remain deferred.

Approved plan: see the ship-log comment on #630. Fable approved on the first review
pass — zero findings — the plan explicitly reasoned through and avoided repeating Phase
3's hand-rolled-scanner mistake (no new architecture rule was needed; the existing
package-policy rules automatically cover the new module).

Contract coverage

Invariant	Enforcement	Proof	Sabotage
Success never consumed/cloned	returns response by strict identity on response.ok, no .text()/.json()/clone	clickhouse-http-response.test.ts	.clone().text() bypass → 2 tests fail (verified independently by the coordinator)
Non-2xx → ClickHouseError once	reads error text exactly once, throws minimal error	response test suite	text-call-count/instanceof checks fail if bypassed
One request + one consumer per convenience method	queryJson/queryText/queryProgress each exactly one request() + one consumer	Fetch-count assertions	injected duplicate request() → 3 tests fail
Native errors never wrapped	no catch/wrap around .json()/.text()/streamLines()	identity-preserving rejection tests	wrapping a reader error → identity test fails
Stateless killQuery	one queryText() call, no retained state, no credential/retry/registry	statelessness + isolation tests	registry+retry sabotage → 7 tests fail (composition verified independently by the coordinator)
Target/HTTP query-id isolation	killQuery never writes its SQL target into params.query_id	dedicated isolation test	auto-writing target into params → 3 tests fail

Tests

• npm run check:types && npm run check:arch && npm run check:schemas && npm run check:examples && npm test && npm run build — green (7301 tests, 100%/97.14%/100%/100% coverage).
• Full root e2e regression, --project=chromium --project=webkit — 219/215 passed respectively (4 pre-existing skips), including new Scenario 9 proving queryProgress() preserves the same native post-header-cancellation semantics through the new API.

Build

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

Invariant/sabotage verification

Verified twice: once by the implementer (whose own sabotage checks caught a real gap
in its first test draft — a .clone().text() bypass a bodyUsed-only check wouldn't
catch — fixed before reporting done), and once by the coordinator (diff footprint
confirming zero src/** changes, full six-command gate, direct reproduction of the
clone-bypass sabotage, and direct code review confirming killQuery's genuine
statelessness).

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: purely additive to the package; zero src/** changes; no new hand-rolled text/regex scanner
• 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, ADR-0005 addendum, 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: 2d251368964f294551cbec6f62f66fcf68eadf92

Major — queryJson's empty-string format contract is false end-to-end, and the test deliberately stops before the serializer

ClickHouseJsonRequest / queryJson document that an explicit defaultFormat: '' is preserved and transmitted unchanged, with ?? 'JSON' distinguishing omission from an explicit empty string. queryJson does preserve '' when it constructs the ClickHouseHttpRequest, but request() then passes that value to chUrl, whose existing serializer uses:

const format = opts.format || 'JSONStringsEachRowWithProgress';

So the actual wire request for queryJson({ defaultFormat: '' }) is not empty-format and is not JSON; it silently becomes default_format=JSONStringsEachRowWithProgress. The new test acknowledges this exact behavior and spies on client.request() instead of asserting the Fetch URL, so it proves only the intermediate object and cannot catch the public/wire mismatch.

This also makes the CHANGELOG/API wording about omission-only defaulting incomplete: there is a second falsy default at the serializer boundary. Because no src/** caller consumes these APIs yet, this is not an existing SQL Browser production regression, but it is a package contract defect in the new public API and in the acceptance proof.

Required action: settle the empty-string contract and test it at the actual Fetch URL. If '' is meant to be a distinct explicit format, preserve it through serialization (noting that changing chUrl would need to be reconciled with Phase 4's “low-level request() unchanged” constraint). If empty format is intentionally unsupported, reject/disallow it in the new convenience API and remove the claim/test that it is transmitted unchanged. The current state cannot simultaneously claim preservation and rely on the existing falsy serializer fallback.

Other adversarial checks

• ensureClickHouseSuccess returns successful responses by identity and consumes non-2xx text once; consumers do not catch/wrap native reader/JSON/text failures.
• queryJson/queryText/queryProgress each have one request() plus one matching consumer; no SQL Browser view/result policy leaked into package production code.
• killQuery is stateless, one queryText() call, keeps target queryId separate from HTTP params.query_id, and its private quoter matches the existing backslash-then-single-quote escaping convention.
• The 14-file diff contains no src/** or architecture-checker change.
• Scenario 9 exercises client.queryProgress() using the real browser Fetch path and the caller's original AbortSignal.
• I found no realistic ClickHouse-wire failure for consumeProgressResponse's success.body!: ClickHouse successful HTTP query responses are 200; an empty 200 Fetch response still exposes a body stream. A synthetic 204 Response can have a null body, but that is outside the actual ClickHouse POST success path under review.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previous reviewed head: 2d251368964f294551cbec6f62f66fcf68eadf92
Reviewed head: 090dfa25c7492450e808ddaf11d15b08b6c3b40b

Pass-1 finding reassessment

Resolved — queryJson empty-string defaultFormat contract/test mismatch.

The old head claimed an explicit defaultFormat: '' was transmitted unchanged even though the unchanged Phase-1/2 chUrl serializer uses a falsy fallback and therefore emits default_format=JSONStringsEachRowWithProgress. The new head does not change request() or chUrl; instead it narrows the public contract accurately: queryJson preserves '' only into the composed ClickHouseHttpRequest, while explicitly documenting the existing serializer fallback at the wire boundary.

The test now proves both layers independently: it asserts that client.request() receives defaultFormat === '' (so queryJson really uses ??, not ||) and also asserts that the final Fetch URL contains default_format=JSONStringsEachRowWithProgress. That closes the acceptance-proof gap from pass 1.

I consider this resolution scope-correct for Phase 4. The strongest objection is that allowing queryJson({defaultFormat: ''}) can lead to a non-JSON response and then a native .json() failure. But the API already deliberately treats an explicit format as opaque caller-supplied wire data and permits arbitrary non-JSON format strings; validation or semantic format restriction is not part of A8/A9, while changing chUrl would alter the low-level request behavior this phase is required to preserve.

Complete updated-PR regression review

No new actionable findings.

• Old→new fix delta is one commit and changes only packages/clickhouse-http/src/client.ts comments plus tests/unit/clickhouse-http-package.test.ts; no runtime statement changed.
• The complete PR remains 14 files with zero src/** production changes and no architecture-enforcement change.
• ensureClickHouseSuccess still returns successful responses by identity, reads non-2xx text exactly once, and the JSON/text/progress consumers still add no catch/wrapping around native reader failures.
• queryJson/queryText/queryProgress remain exactly one request() plus one matching consumer; no SQL Browser Table/KPI/row-cap/result-presentation policy appears in package production code.
• killQuery remains stateless, one queryText() call, keeps SQL target queryId separate from HTTP params.query_id, and uses only the narrow private unexported backslash-then-single-quote quoter required for this phase.
• consumeProgressResponse's success.body! remains acceptable for the actual ClickHouse POST success path under review. A synthetic bodyless 204 can violate the generic Fetch type assumption, but ClickHouse successful query responses use the body-bearing 200 path; this is not a realistic A8/A9 defect here.
• Scenario 9 still invokes the package's real client.queryProgress() with the caller's original AbortSignal and real browser Fetch path. The fix commit does not touch runtime/e2e code.
• CHANGELOG.md is factually consistent with the current diff: it describes queryJson's own ?? 'JSON' defaulting without claiming that an explicit empty string survives the pre-existing chUrl wire fallback.
• New-head GitHub Actions are green: the test job reports 7301/7301 tests, package source at 100% statement/branch/function/line coverage, and the PR Chromium e2e job reports 219/219 passed. PR CI is Chromium-only; the fix changes no runtime or e2e statement relative to the previously reviewed head, so it does not introduce a new WebKit cancellation path.

VERDICT: SHIP