feat(#630): simplify SQL Browser authentication integration around @altinity/clickhouse-http (phase 6)

[BorisTyshkevich]

Summary

Phase 6 of #630 ("Epic: extract the Fetch-native ClickHouse client"). Moves SQL Browser's
authenticated ClickHouse HTTP request composition — credential lookup, epoch capture/fencing,
refresh-retry, Authorization construction, lifecycle (connected/offline/sign-out) classification —
out of src/net/ch-client.ts's authedFetch()/transportFor() and into a new,
narrowly-scoped module, src/net/authenticated-clickhouse-request.ts, which composes
directly over @altinity/clickhouse-http's request() and its Phase-4 response consumers
(consumeJsonResponse/consumeTextResponse/consumeProgressResponse). This is the first
real production consumer of the Phase-4 response layer.

authedFetch() and transportFor(ctx) are fully deleted — no forwarding alias, no second
retry loop, no second Authorization constructor. queryJson(), runQuery(), and
exportQuery() all now traverse the new authenticatedRequest()/authenticatedJson() entry
points. ChCtx stays SQL-Browser-owned (now extends AuthenticatedRequestCtx), and the
frozen-lease cancellation bypass (killQueryWithLease) is untouched — it still never reads
mutable credential/refresh/lifecycle state.

Query-result/export cutover to the package's convenience query APIs, and deletion of the
remaining generic transport mechanics, stay Phase 7's scope per the issue's own phase split —
not attempted here.

Claims A12 (authentication composed through one authenticated request layer over package
request()) and A13 (epoch/refresh/lifecycle/cancellation/frozen-lease invariants remain
regression-tested and unchanged). A1–A11 already shipped; A14–A18 remain deferred to Phase 7/8.

Invariant map (plan §8) — all 13 behavioral invariants + architecture guards, sabotage-proven

#	Invariant	Sabotage result
1	Epoch captured before first await	Moved capture below await getToken() → replacement epoch gained authority instead of AbortError. Failed as required.
2	Settings/params snapshot before credential await	Removed defensive copy → mutated caller values reached the wire. Failed.
3	Preparation failure precedes token lookup	Removed eager chUrl preflight → token read became observable before the URIError. Failed.
4	Fence after every credential/body await	Removed a post-await fence → a stale request wrongly signed out. Failed.
5	Final epoch fence immediately before request	Moved fence above authHeader() → a Fetch occurred despite the flip. Failed.
6	Complete Authorization computed per attempt	Hoisted Authorization outside the loop → retry carried the stale header. Failed.
7	At most one auth-refresh retry	Widened the bound → refresh/fetch both exceeded the maxima. Failed.
8	Post-confirmation 401/403 remain query outcomes	Moved the confirmed-check after refresh/sign-out → forced sign-out on both. Failed.
9	Only current 2xx reports connected	Moved the connected callback before the epoch check → a stale 2xx mutated state. Failed.
10	Only current non-abort rejection reports offline	Removed the epoch guard → a stale rejection reported offline. Failed.
11	Stale work never mutates replacement lifecycle	Dropped the captured epoch from onSignedOut → call observed unlabeled. Failed.
12	Original AbortSignal reaches Fetch, no bridge	Substituted a derived controller signal → strict identity assertion failed. Failed.
13	Frozen lease never reads mutable auth/lifecycle	Widened the lease type to ChCtx → tsc failed with 9 type errors. Failed.

Architecture sabotages (build/check-boundaries.mjs): importing the new module from
clickhouse-http-transport.ts fails check:arch; type-only importing it from
clickhouse-transport.types.ts fails check:arch (proving the type-only leaf boundary too);
introducing serverVersion in the new module fails the #512 connection-authority guard. All
16 sabotages: bytes saved → mutation introduced → smallest proof run and confirmed to fail for
the stated reason → exact bytes restored; verified clean afterward (git status/git diff
empty at HEAD).

Tests

• tests/unit/authenticated-clickhouse-request.test.ts (new, 627 lines) — the moved
  low-level auth/epoch/refresh/lifecycle characterization suite, retargeted to
  authenticatedRequest()/authenticatedJson()/authenticatedText()/authenticatedProgress(),
  plus package-consumer composition proofs.
• tests/unit/ch-client.test.ts — pared to caller-level proofs: queryJson()'s
  ClickHouseError→plain-Error compatibility translation, runQuery()/exportQuery() result
  shapes, cancellation propagation.
• tests/unit/connection-session.test.ts — unchanged behavior, still green (independent
  proof layer: refresh single-flight, replacement-epoch authority, immutable lease capture).
• tests/e2e/clickhouse-http-transport.{html,spec.js} — added authenticated-path variants of
  post-header cancellation scenarios 5–9, run through the real authenticatedRequest()/
  authenticatedProgress() composition (not just the raw package client). 30/30 passing on
  both Chromium and WebKit locally.

Gate

npm run check:types   ✅
npm run check:arch     ✅ (902 files, 13 rules, 0 violations)
npm run check:schemas  ✅
npm run check:examples ✅
npm test               ✅ 225 files / 7362 tests, 100% statements/functions/lines, 97.14% branches
npm run build           ✅ dist/sql.html built
npm run test:e2e (transport spec) ✅ 30/30 Chromium + WebKit

Review

• One targeted High-risk internal read-only review over the complete branch diff: no
  concrete defects found; confirmed the move is byte-for-byte algorithmically identical to
  the deleted authedFetch() apart from the client-construction line, confirmed no owner
  remnants, confirmed the frozen-lease bypass untouched, confirmed the architecture-guard
  change is a pure data extension.
• One focused security-review pass (this repo's security-review skill isn't installed in
  the current environment, so an equivalent adversarial pass was run against its exact
  checklist): no vulnerabilities found across all 10 checked items (epoch supersession,
  Authorization reuse, missing fences, stale sign-out, credential observability, package
  boundary, AbortSignal identity, frozen-lease isolation, misclassification of prep/abort/HTTP
  failures).

Documentation reconciliation

CHANGELOG.md [Unreleased], docs/ARCHITECTURE.md, CLAUDE.md, .wiki/Architecture.md,
.wiki/Source-Map.md, .wiki/Decisions-and-Roadmap.md, and a narrow current-state addendum
to docs/ADR-0005-clickhouse-web-client.md (preserving all historical content/dates
unchanged) all updated. A repository-wide authedFetch/transportFor reference sweep
reclassified every hit: current-owner comments updated, genuine historical references
preserved, executable references eliminated.

Part of #630.

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

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: 039f1c3c7bcf6fc9bfe34445d10e49812fd901c9

Finding

P2 — docs/ARCHITECTURE.md still has stale present-tense lifecycle/auth ownership claims. The new Phase-6 section correctly says src/net/authenticated-clickhouse-request.ts is the normal-request auth/epoch/refresh/lifecycle owner, but two earlier current-state statements still contradict that move:

• the connection-session row says the live chCtx is mutated with “authConfirmed by net/ch-client”;
• the connection-lifecycle section says “net/ch-client reports only successful 2xx transport settlement as connected and rejected, non-aborted fetch as offline.”

At this head, authenticatedRequest() is the code that sets ctx.authConfirmed and invokes onTransportConnected / onTransportOffline. Please retarget those two present-tense statements to net/authenticated-clickhouse-request (or describe the behavior without assigning it to ch-client). This is especially worth fixing in this trust-boundary phase because the same document’s new Phase-6 section already declares the opposite ownership.

What I verified

• Reviewed the complete 22-file, 4-commit PR diff against base 882b78643622d3c44ce196cd60d261a6b4af29cc; no packages/clickhouse-http/** file is changed.
• Compared the old authedFetch() loop with authenticatedRequest(): epoch capture, request snapshot/preflight, post-await fences, final pre-side-effect fence, per-attempt Authorization construction, one-refresh bound, post-confirmation 401/403 behavior, and lifecycle classification retain the same relative ordering.
• Confirmed no authedFetch or transportFor(ctx) declaration/forwarding alias remains in ch-client.ts; the new module has the single normal-request retry/Authorization loop.
• Confirmed the caller’s exact AbortSignal is forwarded by identity through authenticatedRequest() to package request() and that package request() passes request.signal directly to native fetch(); no derived AbortController/listener bridge exists in that path.
• Confirmed queryJson() translates package ClickHouseError back to a plain Error with the same parsed message; native JSON/body/network/abort failures remain unwrapped.
• Confirmed killQueryWithLease() still receives only the frozen lease/query id/quoter and the captured lease contains only epoch/origin/complete Authorization/fetch, bypassing mutable credential/refresh/lifecycle state.
• Confirmed authenticated E2E scenarios 5–9 import and call authenticatedRequest() / authenticatedProgress() directly rather than only exercising the raw package client.
• Canonical GitHub Actions for this head is green; PR E2E ran 224 Chromium tests and passed 224/224. The PR workflow is explicitly Chromium-only (PR_ONLY_CHROMIUM=true), so the reported separate WebKit 30/30 run is not independently evidenced by this PR CI run.
• A local clone/test run was not feasible in this review environment because outbound DNS could not resolve GitHub; canonical source, patch, history, tests, and Actions evidence were fetched read-only through GitHub instead.

No other concrete defect found in the reviewed Phase-6 scope.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: 039f1c3c7bcf6fc9bfe34445d10e49812fd901c9
Reviewed head: b6fbf1a59d770e6fed56b34adaf786ea5c363339

Pass-1 finding status

RESOLVED. The one fix commit between the two SHAs changes only docs/ARCHITECTURE.md (3 additions / 3 deletions) and retargets both stale current-state ownership statements from net/ch-client to net/authenticated-clickhouse-request, exactly addressing pass 1. No production, package, test, or boundary-checker code changed after the prior review.

Finding

P2 — two remaining current-path descriptions still call the compatibility transport the normal production query path.

1. .wiki/Architecture.md Query path step 2 still says src/net/ch-client.js delegates generic request construction/stream mechanics through clickhouse-transport.types.js + clickhouse-http-transport.js. At this Phase-6 head, ordinary queryJson/runQuery/exportQuery instead reach authenticated-clickhouse-request.ts, whose authenticatedRequest() constructs the package client directly and calls package request(); .wiki/Source-Map.md correctly says the compatibility transport's one remaining production caller is killQueryWithLease.
2. tests/e2e/clickhouse-http-transport.html still says raw Scenarios 1–8 exercise the “still-live SQL Browser production composition (createHttpTransport().send() -> package streamLines)” and that it “remains the production path until Phase 7's cutover.” That ceased to be the ordinary production path in this Phase 6. The new authenticated variants correctly exercise the current production-shaped path; the stale header comment mislabels what the raw scenarios prove.

Please rewrite those two descriptions so the ordinary path is ch-client caller → authenticatedRequest() → package request() (and package consumer/streamLines as applicable), while createHttpTransport is described as the temporary compatibility/frozen-lease path that remains through Phase 6.

Revalidation

• The old→new delta is docs-only, so the trust-boundary implementation is byte-identical to pass 1.
• Rechecked all 13 invariants against the current head: epoch capture/snapshots/preflight/post-await fences/final pre-send fence/per-attempt Authorization/one-refresh bound/post-confirmation 401/403/current-only lifecycle/offline classification/captured-epoch sign-out/original AbortSignal/frozen-lease isolation remain enforced in the same locations.
• authedFetch() and transportFor(ctx) declarations remain absent from ch-client.ts; there is still one normal-request auth/retry loop.
• The package remains untouched by the complete PR.
• queryJson() still restores the prior plain-Error message/shape on package ClickHouseError; native JSON/body/network/abort errors remain unwrapped.
• killQueryWithLease() remains isolated behind the frozen lease.
• Authenticated E2E scenarios 5–9 do call authenticatedRequest() / authenticatedProgress() directly; this finding is only about the stale comments labeling the raw path.
• Canonical Actions for the new head is green across test/build, E2E, bundle, docker-smoke, and size; PR E2E again ran 224 Chromium tests and passed 224/224. As before, PR CI is Chromium-only, so the separate WebKit claim is not independently represented by this PR workflow.

No additional runtime/security/compatibility defect found.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed baseline: 039f1c3c7bcf6fc9bfe34445d10e49812fd901c9
Pass-2 reviewed head: b6fbf1a59d770e6fed56b34adaf786ea5c363339
Reviewed head: 39a6616763429bd4a6e62e82580bc46271fe4161

Earlier finding status

• Pass 1: RESOLVED. docs/ARCHITECTURE.md now attributes authConfirmed mutation and connected/offline classification to net/authenticated-clickhouse-request, not net/ch-client.
• Pass 2: RESOLVED at the two locations raised. .wiki/Architecture.md now describes the ordinary Phase-6 path as ch-client caller -> authenticated-clickhouse-request -> package client, and the E2E harness's top-level Scenario 1-8 header now explicitly labels the raw compatibility path as lower-layer regression coverage rather than the current ordinary path.

Finding

P2 — the E2E harness still contains older present-tense comments that contradict its newly corrected Phase-6 header.

tests/e2e/clickhouse-http-transport.html still says, in its older Phase-3/Scenario-6 commentary, that the raw createHttpTransport().send() -> package streamLines composition is the “SAME real production call path” / “exact call path ch-client.ts's runQuery uses in production.” That stopped being true in Phase 6: current runQuery() obtains its Response through authenticatedRequest() -> package request(), then streams that Response with package streamLines; createHttpTransport() remains in production only for the frozen-lease killQueryWithLease() bypass. The corrected top-of-file header already states exactly that, so the file now contradicts itself.

Please retarget those older Scenario-6/Phase-3 comments to historical/lower-layer wording (and sweep the immediately adjacent raw-scenario wording for the same obsolete “production path” label). The authenticated Scenario-6 comment is already accurate: it identifies authenticatedRequest() -> package streamLines as the current Phase-6 runQuery() path.

Revalidation

• From the original reviewed 039f1c3c7bcf6fc9bfe34445d10e49812fd901c9 to this head there are exactly two follow-up commits, touching only docs/ARCHITECTURE.md, .wiki/Architecture.md, and E2E HTML comments. No production source, package source, boundary-checker logic, or test behavior changed after the original review.
• Rechecked all 13 trust-boundary invariants against current source: epoch capture, request snapshots/preflight, every post-await fence, final pre-send fence, per-attempt Authorization, one-refresh bound, post-confirmation 401/403 handling, current-only connected/offline effects, captured-epoch sign-out, exact AbortSignal identity, and frozen-lease isolation remain enforced in the same locations.
• authedFetch() and transportFor(ctx) declarations remain absent from ch-client.ts; there is still one normal-request auth/retry loop.
• The complete PR still changes no packages/clickhouse-http/** file. Package request() still passes request.signal directly to Fetch with no derived controller/listener bridge.
• queryJson() still translates package ClickHouseError back to a plain Error with the exact parsed message; native JSON/body/network/abort errors remain unwrapped.
• killQueryWithLease() still accepts only the frozen lease/query id/quoter and builds its one-shot compatibility transport from the lease's origin/Authorization/fetch, without mutable credential/refresh/lifecycle state.
• Authenticated E2E variants 5-9 genuinely call authenticatedRequest() / authenticatedProgress(); the finding is only stale explanatory text around the raw lower-layer scenarios.
• Canonical Actions for 39a6616763429bd4a6e62e82580bc46271fe4161 is green across test/build, E2E, bundle, docker-smoke, and size. PR E2E ran 224 Chromium tests and passed 224/224; the PR workflow is explicitly Chromium-only, so the reported separate WebKit run is still not independently represented by this CI run.
• main has advanced since this PR's merge base, but the intervening commits are confined to skills/chatgpt-review/** / skills/ship/**, do not overlap this PR's files, and GitHub currently reports the PR mergeable.
• A local clone/test run remains infeasible in this review environment because outbound DNS cannot resolve GitHub; canonical source, patch, history, and Actions evidence were used read-only instead.

No runtime/security/compatibility defect found beyond the documentation/comment reconciliation issue above.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review — final certification (manual continuation, same conversation)

Session 226cff42's own pr-mode pass cap (3) was spent after pass 3's fix, so this
certification and the prior manual-continuation exchange were carried out by driving the
existing conversation tab directly (https://chatgpt.com/c/6a76eb99-d178-83eb-a700-e00af0e4eb64)
rather than opening a new session, per this repo's "one unit, one ChatGPT conversation" rule.

Reviewed head: acd64200d6a43bed25b447153b5aec208bece3e6

Reviewed canonical PR head acd6420. GitHub confirms that
exact SHA is current, the PR is open and mergeable, with 22 changed files overall.

The delta from the previous reviewed head is one comment-only commit in
tests/e2e/clickhouse-http-transport.html. Both outstanding findings are fixed correctly:
the Phase-3 description now explicitly says the raw streamLines composition was the
runQuery path through Phase 5 and was superseded in Phase 6, while Scenario 9 now calls
Scenario 6 the lower-layer compatibility/package path.

I swept the full current file rather than just the patch. The remaining current-state
statements are internally consistent: queryJson/runQuery/exportQuery are identified as
using authenticatedRequest()/authenticatedProgress(), createHttpTransport is identified
as remaining live only for the frozen-lease killQueryWithLease compatibility route, and
raw Scenarios 1–8 are explicitly classified as lower-layer regression coverage rather
than the current ordinary production path. Raw Scenarios 6 and 8 both carry the same
explicit disclaimer. The authenticated Scenario 6 correctly identifies itself as the
current runQuery() production path.

No remaining stale present-tense production-path claim was found in the requested sweep.
Because this latest delta is comments only, it introduces no runtime change relative to
the implementation already reviewed in the preceding passes. The GitHub Actions run for
this exact head also completed successfully across all applicable jobs.

VERDICT: SHIP