refactor(cli/http): replace ky with a self-contained HTTP client#36711
Open
lin-snow wants to merge 10 commits into
Open
refactor(cli/http): replace ky with a self-contained HTTP client#36711lin-snow wants to merge 10 commits into
lin-snow wants to merge 10 commits into
Conversation
fbc4317 to
2704784
Compare
2704784 to
9e3e455
Compare
9e3e455 to
2fd72b4
Compare
4fe5cd9 to
033b0f5
Compare
GareArc
reviewed
May 29, 2026
First of three commits replacing the ky-based http client with a hand-written
ofetch-shaped implementation. This commit is purely additive — nothing imports
the new files yet; the existing createClient/KyInstance path is untouched and
all 854 tests pass.
New files (cli/src/http/):
README.md design contract: §1-11 covering surface, dispatch lifecycle,
12-row decisions log, ofetch comparison, migration plan
types.ts additions: HttpClient, RequestOptions, ClientOptions,
ResolvedOptions, FetchContext, Hook, Hooks, HttpMethod,
SearchParamValue, local BodyInit/HeadersInit aliases
(DOM-named unions missing from @types/node 25)
url.ts joinURL (ported from ofetch/utils.url.ts:15-35),
appendSearchParams (undefined-skip + primitive coercion)
body.ts isJSONSerializable (ported from ofetch/utils.ts:17-43),
buildBody (json vs raw body, Content-Type injection,
GET/HEAD body suppression)
retry.ts RETRY_METHODS, RETRY_STATUS_CODES, shouldRetry, backoffDelay
(300ms base, exponential, capped at 30s)
hooks.ts setBearer, setUserAgent, logRequest, logResponse,
classifyTransport — replace middleware/* one-for-one and
use ctx.meta instead of casting onto Request
*.test.ts focused unit tests for url/body/retry (no integration)
Toolchain:
eslint.config.mjs ignore cli/src/http/tmp/** (working clone of ofetch
may live there during refactor; not committed)
cli/tsconfig.json exclude src/http/tmp from tsc
cli/vite.config.ts exclude src/http/tmp/** from vitest
Verification:
pnpm test 81 files, 854 tests pass
pnpm lint clean
pnpm type-check clean
Commit 2 swaps createClient → createHttpClient and migrates ~30 call sites.
Commit 3 drops ky.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Second of three commits. Replaces the ky-based client.ts with the new
ofetch-shaped dispatch + factory and migrates every consumer to the new
HttpClient surface. The previous middleware/ directory is deleted.
cli/src/http/:
client.ts full rewrite. createHttpClient(opts) returns HttpClient
(get/post/put/patch/delete<T> + fetch + stream + extend).
Internal dispatch(state, path, opts, attempt) is
recursive (matches ofetch's onError shape). Built-in
hooks compiled from bearer/userAgent/logger fields,
user hooks appended via opts.hooks.
Error.captureStackTrace(err, dispatch) clips dispatch
frame from user-facing stacks (per ofetch trick).
client.test.ts rewritten. Ports 13 existing smoke tests + adds:
extend() inherits convenience fields; logger: undefined
drops log hooks; fetch() returns Response without
throwing; stream() ignores retry; onRequest mutation
observed by subsequent hooks; onResponse throw
propagates without entering onResponseError chain.
middleware/ deleted (auth.ts, request-logger.ts, user-agent.ts).
Replaced by hooks.ts compiled into the client at
construction time.
body.ts body return type narrowed: BodyInit | undefined
(was | null) to match Node's RequestInit shape.
types.ts HeadersInit narrowed to [string,string][] tuple pairs.
BodyInit narrowed: drop ArrayBufferView (Node wants
DataView specifically; Uint8Array covers byte buffers).
ResolvedOptions.body: BodyInit | undefined.
cli/src/util/host.ts: + openAPIBase(host): trims trailing slash, appends
/openapi/v1/. Used at 5 createHttpClient sites instead
of inlining at each.
Pattern A migrations (typed JSON, 14 sites): this.http.get(p).json<T>()
→ this.http.get<T>(p). Applied across api/account.ts, account-sessions.ts,
apps.ts, file-upload.ts, members.ts, meta.ts, workspaces.ts, and
commands/resume/app/run.ts. apps.ts also rewrites URLSearchParams-building
to plain Record (our searchParams typing skips undefined).
Pattern B migrations (raw Response, 2 sites in api/oauth-device.ts):
this.http.post(p, { json, throwHttpErrors: false, context: { skipClassify: true } })
→ this.http.fetch(p, { method: 'POST', json }).
fetch() defaults throwOnError: false (per README D5), so the OAuth flow
reads res.status directly with the same semantics as before.
Pattern B-stream migrations (api/app-run.ts:49,86):
this.http.post(p, { json, headers: { Accept: SSE }, retry: 0, timeout: false })
→ this.http.stream(p, { method: 'POST', json, headers, throwOnError: true }).
Stream callers opt into throwOnError so non-2xx open still throws BaseError;
retry/timeout disabled internally by stream().
Type swaps (KyInstance → HttpClient): 9 API classes + 11 command classes +
their factory params + test fixtures (~25 files). Bulk perl rewrite for
mechanical sites, manual edits for the OAuth/streaming/probe semantics.
createClient() → createHttpClient() at the 5 production sites:
version/probe.ts:53, commands/auth/login/login.ts:39,
commands/auth/logout/index.ts:27, commands/_shared/authed-command.ts:56,78.
Each now passes { baseURL: openAPIBase(host) } instead of { host }.
Test fixture migrations (12 files): perl rewrite of createClient(...) calls
to createHttpClient + openAPIBase(host).
Verification:
pnpm test 81 files, 862 tests pass (no regressions vs commit 1's 854)
pnpm lint clean
pnpm type-check clean
pnpm build succeeds
Commit 3 drops ky from package.json and removes cli/src/http/tmp/.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Third and final commit of the http client refactor. With every CLI call site migrated to createHttpClient in commit 2, ky is no longer reachable from any production or test code in cli/. cli/package.json: remove "ky": "catalog:" from dependencies pnpm-lock.yaml: regenerated (ky entry removed from cli's importer) cli/src/http/tmp: deleted (ofetch reference clone, untracked from commit 1) Verification after removal: grep -r "from 'ky'" cli/src/ → no matches grep -r "\bKyInstance\b" cli/src/ → no matches test ! -d cli/src/http/tmp → directory absent pnpm test → 81 files, 862 tests pass pnpm lint → clean pnpm type-check → clean pnpm build → succeeds The web/ workspace continues to depend on ky from the same catalog entry — this commit affects only cli's importer in the lockfile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…etup
- streamFetch now passes timeoutMs: 0 so SSE bodies aren't aborted at the
30s client default; dispatch already treats 0 as "disabled".
- Snapshot userAborted before onRequestError rewrites ctx.error so user
ctrl+C never retries; drop the causedByTimeout short-circuit so POST
timeouts respect the GET/PUT/DELETE method allowlist.
- Simplify shouldRetry: the AbortError name check was dead after
classifyTransport. User aborts are filtered earlier now.
- Add integration tests for SSE no-timeout, POST timeout no-retry,
GET timeout retries, and GET user-abort no-retry.
- Drop unused HttpFactoryOptions, redundant empty-token guard in setBearer,
and stale cli/src/http/tmp exclusions across tsconfig / vite / eslint.
- Introduce test/fixtures/http-client.ts and migrate ~80 test call sites
from createHttpClient({ baseURL: openAPIBase(...), ... }) to
testHttpClient(host, bearerOrOpts?).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add unit tests for the API clients that the ky→HttpClient migration left without coverage: apps, account, account-sessions, workspaces.list, and file-upload. Tests target the refactor-risk surfaces — searchParams defaults/omission, fields comma-join, void-DELETE consuming a JSON body, and the multipart upload path keeping its boundary instead of being coerced to application/json. Extract the stub-server helper previously inlined in members.test.ts into test/fixtures/stub-server.ts so the new suites share one source of truth. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
F-2 (regression): the new createHttpClient dropped the implicit UA default
that the old ky-based client carried, and every production call site (5
sites: authed-command x2, version/probe, login, logout) had relied on that
default. Server logs / WAF rules / telemetry were quietly losing the
difyctl/<ver> (<platform>; <arch>; <channel>) signal. Pin the default at
the client layer (compileState) instead of plumbing userAgent through
every call site, so a new caller can't reintroduce the regression. The
explicit-override path is preserved. Add a test that asserts the default
header shape when no userAgent is supplied.
Tidy-ups in the same change:
F-1 src/commands/get/member/run.test.ts:73 referenced an unimported
KyInstance type. Only escaped tsc because tests are excluded from
the project's include list. Replace with HttpClient (matches :89).
F-3 src/api/members.test.ts still carried an inlined stub-server with
the older `lastRequest` shape, even though e666e8c claimed the
helper was extracted to test/fixtures/stub-server.ts. Migrate the
file onto the shared CapturedRequest fixture; WorkspacesClient
import switched from dynamic to static now that no cycle risk
exists. All 11 tests preserved.
F-4 src/http/url.ts:appendSearchParams only skips `undefined` — empty
strings, 0, false coerce through and land on the wire. Document
the convention in a header comment and pin it with a unit test in
url.test.ts ("keeps empty-string values on the wire").
F-5 Symmetric to the existing "onResponse throw propagates" test, add
one for the onResponseError branch: a hook throw in the !res.ok
path must replace the classified BaseError on the way out of
dispatch.
Verification (cli/):
npx tsc --noEmit -p tsconfig.json clean
npx vitest run 87 files, 902 tests pass (+3)
npx eslint . clean
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
typedCall unconditionally called res.json(), which throws an unclassified SyntaxError on 204/empty 2xx bodies — breaking void-returning callers like revoke/stopTask. Return undefined for 204/205/empty bodies instead, and add regression tests. Also drop external project names from a few comments.
The SSO login test read the token via the old two-arg store.get(host, account) signature; the Store interface takes a single Key, so it resolved to undefined. Use store.get(tokenKey(...)) like the happy-path test. Also drop a padded blank line that the rebased flag-parsing errors.test.ts left failing eslint.
…yAgent Node's global fetch ignores proxy env vars (review feedback). Resolve an undici EnvHttpProxyAgent once per process when a proxy var is present and pass it as the per-request dispatcher; when none is set, leave fetch on its default dispatcher. Verified routing is honored on both Node (via dispatcher) and the bun-compiled binary (native proxy support + dispatcher, no regression). Adds undici, pinned to the already-resolved 7.25.0, and externalized at build.
a951c06 to
2faf9dc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace the CLI's
ky-based HTTP layer with a small, self-contained client and migrate every call site to it. Internal transport refactor — no behavioral change to CLI commands.Dependency-wise this swaps the
kyHTTP wrapper forundici(the engine that already powers Node'sfetch), pulled in for one reason: honoringHTTP_PROXY/HTTPS_PROXY/NO_PROXYviaEnvHttpProxyAgent(Node's globalfetchsilently ignores them). It is externalized at build, not bundled.flowchart LR subgraph Before A["~30 call sites"] -->|"KyInstance leak"| K["ky + middleware/"] end subgraph After B["~30 call sites"] --> H["createHttpClient → HttpClient"] H --> D["recursive dispatch + fetch + hooks"] D --> P["proxy dispatcher (EnvHttpProxyAgent)"] endHttpClientsurfaceget/post/put/patch/delete<T>Promise<T>204/empty body →undefinedfetchPromise<Response>streamPromise<Response>extendHttpClientcli/src/http/ships focused, unit-tested modules:url(query building),body(encoding),retry(policy),hooks(lifecycle),proxy(env-driven proxy dispatcher),types(contract). Auth / UA / logging / error-classification are composable hooks compiled in at construction;AbortSignal.any([user, timeout])replaces the old timing cast and the{ throwHttpErrors: false, skipClassify }escape hatch. Themiddleware/layer and thekydependency are removed;testHttpClientdedupes client setup across the suite.Proxy support
proxyDispatcher()resolves a singleEnvHttpProxyAgentonce per process when a proxy env var is present and passes it as the per-requestdispatcher; with no proxy var set,fetchkeeps its default dispatcher untouched. Routing was verified end-to-end (dead-proxy → connection refused at the proxy) on both Node (via the dispatcher) and thebun build --compilebinary (native proxy support + dispatcher, no regression).Verification
pnpm testpnpm type-check+pnpm lintky/KyInstancerefs incli/srcScreenshots
N/A — internal refactor, no user-facing UI change.
Checklist
cd cli && pnpm type-check && pnpm lint && pnpm testto appease the lint gods