feat(#585 phase 1): establish the transport seam without behavior change

[BorisTyshkevich]

Summary

• Defines a narrow SQL Browser ClickHouse-transport contract (src/net/clickhouse-transport.types.ts: ClickHouseTransport, TransportDeps, TransportRequest, StreamCallbacks) and puts the current custom HTTP implementation behind it (src/net/clickhouse-http-transport.ts: createHttpTransport, plus chUrl/ChUrlOpts moved verbatim and re-exported unchanged).
• ch-client.ts's auth/epoch/retry policy, product operations, and ChCtx are otherwise untouched — no new ChCtx field, no runtime transport switch. authedFetch is the one exported-signature change (its only importers are ch-client.ts internals and its own unit test); it now takes a structured request and centralizes a synchronous settings/params snapshot at entry (before its first await) as a single aliasing defense, replacing what would otherwise be a fragile per-caller responsibility.
• Adds a reusable contract-test-suite factory (tests/unit/clickhouse-transport-contract.ts), registered against the current implementation only. build/check-boundaries.mjs gains a bare-specifier ban on @clickhouse/client-web anywhere under src/** (mirrored as a coverage-gated unit test, since ignore-scripts=true means pretest/check:arch don't run under a bare npm test).
• No user-visible or production-behavior change. Bundle size delta ≈ 0 (pure code movement). Phases 2–4 (an official-client cutover) remain out of scope and do not proceed without a new decision — ADR-0005 is Rejected (Phase 0, PR refactor(#585): Phase 0 — @clickhouse/client-web validation spike, ADR-0005  #619).

Contract coverage

Per the approved plan (skills/ship plan-review-loop, 5 ChatGPT passes + 1 human-approved fix-in pass — see ship-log): all 8 Phase 1 implement-list bullets claimed and delivered; the "Architecture" acceptance-criteria subset claimed in its enforceable-now form (transport contract exists, only-transport-imports-the-official-client rule active even before that file exists, OAuth/epoch/lifecycle/retry/session/cancellation/caps/exports/domain-queries remain SQL-Browser-owned, product SQL not moved into the transport). Issue "Integration"/"Browser E2E" test lists and adapter-level error translation are explicitly deferred to Phase 2+ (no second implementation exists yet to compare against).

Invariant verification

All 20 issue Hard Invariants apply as "stays exactly as before" (pure refactor); the plan's own 12+-row invariant map was implemented with named enforcement + a compile-time or test proof for each (epoch-fence adjacency, frozen-lease auth isolation, live origin/fetch accessors vs. snapshot, entry-point settings/params snapshot, no @clickhouse/client-web under src/**, no dual transport, no product SQL in the transport, authored FORMAT never duplicated, raw bytes never forced through text). Independently re-verified by a targeted internal review pass (read-only, Medium-risk budget) — no findings.

Sabotage cases

All plan-mandated mutations were performed and independently confirmed to fail a named test/check: epoch fence moved after send; createHttpTransport snapshotting origin at construction; pre-consuming the response body in send; appending a FORMAT clause in send; adding an @clickhouse/client-web import to ch-client.ts/the transport files (five per-category checks against the new arch rule); removing/moving the entry-point settings/params snapshot.

Tests

• npm run check:types && npm run check:arch && npm run check:schemas && npm run check:examples && npm test && npm run build — all green (re-run twice by the coordinator: once on the worker's output, once after syncing with origin/main). 219 test files / 7151 tests passed; coverage 100% statements / 97.14% branches / 100% functions / 100% lines, no per-file floor violations (the two new transport files don't appear in the coverage table — fully covered at 100/100/100/100).
• npm run test:client-spike: 91 passed, 19 skipped.
• npm run test:e2e (Chromium + WebKit): all green except one WebKit flake (dashboard-tree.spec.js, a page.goto timeout under full-suite resource contention) that passed in isolation on retry and doesn't touch any transport/ch-client code.
• npm run size-report: bundle delta ≈ +167 B gzip (+0.0%) — effectively zero, pure code movement.

Plan review

--planner default (Fable/high authors, ChatGPT reviews). 5 passes run (the loop's cap) without a clean VERDICT: APPROVED — genuine, evidence-backed findings kept surfacing each round (7+2+4+3 accepted across passes 1–4, folded in each time), then 2 more accepted findings on pass 5 (a coverage-claim contradiction re: a nonexistent null-body guard, and a design improvement centralizing a request-snapshot inside authedFetch) went unresolved when the loop hit its cap. Per the ship skill's FULL STOP protocol, the owner reviewed the outstanding findings directly and approved folding both into the plan without a 6th ChatGPT pass (both were narrow, well-evidenced, non-controversial). Conversation: https://chatgpt.com/c/6a7449f6-893c-83ed-b5f0-e534681aa009

Process fix landed separately, in parallel: while reviewing pass 5's rejected findings, we found the plan-mode chatgpt-review prompt had no pass-aware branching (unlike pr mode), so ChatGPT was re-litigating findings already rejected with cited evidence in the plan's own ## Review responses section. Fixed and merged as PR #620 (skills/chatgpt-review only — no product code).

Non-goals confirmed

No @clickhouse/client-web dependency change (package.json/lockfile untouched — the package remains a dev-only Phase-0-spike dependency); no persisted-state/schema/deploy-surface change; no auth/OAuth semantics change; src/application/*, src/ui/*, src/core/*, src/main.ts untouched entirely.

Part of #585 (Phase 1 of 4 remaining; Phase 0 already shipped via #619). Phases 2–4 do not proceed without a new decision per ADR-0005 (Rejected).

🤖 Generated with Claude Code

https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: cb939f9c35973e004065a53149920e676b3fc2dd

P1 — Pin the request origin at authedFetch entry

The refactor changes which cluster a single logical request can target. Before this PR, each caller built chUrl(ctx.origin, ...) synchronously before entering authedFetch; that URL was reused unchanged for the initial attempt and the refresh retry. The new code snapshots settings and params, but createHttpTransport.send() reads ctx.origin after the token await and again on every retry.

The new test named reads ctx.origin live per send within one authedFetch retry cycle explicitly codifies the changed behavior: attempt 1 goes to the old origin and attempt 2 goes to the new origin. Under an unchanged epoch, a request started for cluster A can therefore send its SQL, query ID/session parameters, and resolved Authorization header to cluster B. This contradicts the PR's zero-production-behavior-change contract and leaves the entry-point request snapshot incomplete.

The strongest counterargument is that today's ConnectionSession normally changes origin together with an epoch transition, which the fence catches. That coupling is not part of the exported ChCtx contract, however, and the new test deliberately permits a same-epoch origin change. A behavior-preserving refactor should not replace the old request-target snapshot with an undocumented lifecycle assumption.

Action: snapshot ctx.origin synchronously at authedFetch entry and reuse it for both attempts, while still reading the current origin for each independent authedFetch invocation. The cleanest shape is likely to make the request origin explicit in TransportRequest (or otherwise bind a request-scoped immutable origin), rather than treating the mutable connection origin as transport-construction state. Add regressions for origin mutation while getToken() is pending and while refresh is pending: both sends for that invocation must remain on the original origin; the next invocation must use the new origin.

P2 — The client-web boundary checks pre-authorize the rejected Phase 2 path

Both build/check-boundaries.mjs and the mirrored unit test skip src/net/clickhouse-web-transport.ts via CLIENT_WEB_ALLOWLIST. Consequently, adding that exact file with import '@clickhouse/client-web' passes both checks. The sabotage cases against ch-client.ts and the current transport files do not exercise this bypass.

That does not enforce the stated Phase 1 invariant "no @clickhouse/client-web under src/**" and weakens the ADR's requirement that Phases 2–4 require a new decision. If the intended policy is instead "the future file may import it at any time," the PR description, changelog, test name, and invariant claim are materially overstated.

Action: remove the allowlist in this Phase 1 PR. Introduce the one-file exception only in a separately approved Phase 2/new-decision PR, in the same change that adds the implementation. Add a sabotage test using the exact path src/net/clickhouse-web-transport.ts so the current gate proves it rejects that import.

The URL construction and moved streamLines loop otherwise have focused coverage using real ReadableStream/Response objects, including exact URL ordering, split UTF-8, malformed/empty/trailing lines, in-band exceptions, body preservation, and callback ordering. Canonical CI for the reviewed head is green.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: cb939f9c35973e004065a53149920e676b3fc2dd

Current reviewed head: cb939f9c35973e004065a53149920e676b3fc2dd

The canonical branch is still identical to the pass-1 SHA (0 commits and 0 files differ), so this pass reassesses the existing PR using the supplied rebuttals rather than reviewing a code delta.

Pass-1 finding reassessment

1. Live origin across retries — withdrawn. The repository-wide production wiring supports the rebuttal. chCtx.origin is written only in ConnectionSession: clearing credentials is reached after an epoch-advancing signed-out/auth-required transition, and successful Basic connection assigns the new origin immediately adjacent to the epoch-advancing credentials-installed transition, with no asynchronous or observable operation between those statements. The final epoch fence therefore prevents the cross-cluster retry described in pass 1 in the actual application wiring. A hand-built ChCtx can violate that coupling, but that is not a reachable production regression in this PR.

2. Future client-web allowlist — withdrawn. The exact allowlisted file does not exist, no current src/** file imports the package, and the acceptance contract explicitly permits only the designated transport implementation to import it. The source scanner cannot enforce the separate governance decision that would authorize creating and wiring that future implementation; that belongs to review of that future change.

P1 — URL-construction failures now perform auth work and falsely mark the connection offline

The entry snapshot preserves values, but it does not preserve the old request-preparation boundary.

Before this PR, every affected caller evaluated chUrl(...) before invoking authedFetch. Therefore URL serialization failure occurred before ctx.getToken(), outside the try that reports rejected fetches through onTransportOffline.

At the current head, authedFetch first awaits ctx.getToken(), then calls transport.send(...) inside that transport-failure try; createHttpTransport.send() performs chUrl(...) before it invokes fetch. A serialization exception is consequently treated as a network rejection even though no fetch occurred.

This is reachable with the declared string value type: encodeURIComponent('\ud800') throws URIError: URI malformed for an unpaired UTF-16 surrogate. A focused reproduction using the base and PR control-flow shapes produced:

base: URIError, tokenReads=0, fetches=0, offline=0
pr:   URIError, tokenReads=1, fetches=0, offline=1

In production, onTransportOffline transitions the connection lifecycle to offline with the display detail Network unavailable. With an expired token, the new ordering can also initiate refresh work for a request that the base implementation rejected before entering the auth path. This contradicts both the zero-behavior-change claim and authedFetch's own rule that only a rejected fetch is a transport failure.

Action: preserve request serialization before the first auth await and outside the fetch-rejection classification boundary, or give the transport a preparation/error distinction that lets authedFetch report offline only after the fetch seam was actually invoked. Add a regression using a lone-surrogate setting or parameter and assert: the same URIError is returned, getToken/refresh are not called, fetch is not called, and onTransportOffline is not called.

The complete 14-file PR was rechecked; no dependency, application/UI/core/main, persisted-state, or deployment-surface changes were introduced. URL ordering and the moved stream parser remain covered for their normal inputs, and canonical CI for this unchanged head is green, but it does not cover this preparation-error boundary.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed head: cb939f9c35973e004065a53149920e676b3fc2dd

Current reviewed head: 7cfa1ed82bd50ed01a56bb14bbd714ed3d646ee8

The current head is one commit ahead of the previous review. The delta is limited to src/net/ch-client.ts (+15) and tests/unit/ch-client.test.ts (+12).

Earlier findings reassessed

1. Pass 1: live origin across retries — remains withdrawn. The only production chCtx.origin writers are synchronously coupled to epoch-advancing lifecycle transitions, and the epoch fence remains immediately before transport.send(...). No reachable production cross-cluster retry was found.

2. Pass 1: future client-web allowlist — remains withdrawn. The allowlisted file does not exist, no current src/** file imports @clickhouse/client-web, and governance approval for a future implementation is not a property this source scanner can adjudicate.

3. Pass 2: URL-construction failures perform auth work and signal offline — fixed. authedFetch now snapshots settings/params, eagerly calls chUrl(...) before its first await, and only then reads the token. The actual send still rebuilds the URL with the live origin after the epoch fence. The added lone-surrogate regression verifies the original failure shape: URIError, no token read, no fetch, and no onTransportOffline call. A focused reproduction confirmed those side-effect counts.

Complete updated PR review

No new actionable findings. The fix does not weaken request snapshots, retry reuse, live-origin semantics, epoch-fence adjacency, cancellation behavior, response-body ownership, stream parsing, URL ordering/encoding for valid inputs, or the architecture boundaries. The full PR still changes only the same 14 files; package/lock files and src/application/**, src/ui/**, src/core/**, and src/main.ts remain untouched. Canonical CI for 7cfa1ed82bd50ed01a56bb14bbd714ed3d646ee8 is green.

Direct cloning remained unavailable in the review environment because GitHub DNS resolution failed, so canonical metadata, commit comparisons, source files, patches, comments, and CI were inspected through the GitHub connector; the focused failure-path behavior was also reproduced independently.

VERDICT: SHIP