refactor(platform): one VeryfrontApiTransport under the canonical and token API clients - #3059
Conversation
… token API clients Introduce createVeryfrontApiTransport<T> in src/platform/adapters/veryfront-api-transport.ts. The transport centralises Bearer-token assembly, W3C trace-context injection (injectContext), per-attempt AbortController timeouts (via retryWithBackoff's timeoutMs), redacted retry/timeout logging, and terminal-error wrapping via a caller-supplied wrapFinalError factory. Response-handling policy is delegated via onResponse<T>: - Canonical (retry-handler.ts): throws API_CLIENT_ERROR on any !ok, returns parsed JSON/text. Per-attempt HTTP_CLIENT_FETCH spans, recordApiRequest/recordApiRetry metrics, and the 4xx-non-429 shouldRetry rule remain canonical-only via wrapFetch/onRetry/shouldRetry hooks. - Token (token/veryfront/api-client.ts): returns raw Response for 4xx non-429 (callers handle), throws TOKEN_STORAGE_ERROR for 5xx/429. No span wrapping; uses transport's default onRetry log. All existing tests pass unmodified (153 steps). Module-boundary lint: 0 new baseline entries (focused leaf imports throughout). Net src-line delta: retry-handler.ts -7, api-client.ts -44, new transport +279 (+228 total); structure overhead from JSDoc/types exceeds deduplication savings in raw lines, but the shared HTTP mechanics (auth, tracing, timeout, retry) now live in one place.
There was a problem hiding this comment.
Pull request overview
This PR consolidates duplicated HTTP/retry behavior for calls to api.veryfront.com by introducing a shared VeryfrontApiTransport and routing both the canonical Veryfront API client and the token-storage client through it, while keeping each client’s policy differences (response handling, retry semantics, error wrapping) configurable.
Changes:
- Added
createVeryfrontApiTransportplus a canonical wrapper (createCanonicalVeryfrontApiTransport) to centralize fetch, auth header assembly, trace-context injection, retry/backoff wiring, logging, and error mapping. - Replaced the canonical client’s
requestWithRetryimplementation with a small backward-compatible alias that delegates to the canonical transport. - Updated
VeryfrontAPIOperationsandTokenStorageApiClientto hold a transport instance and use it for requests; updated module-boundaries baseline accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/adapters/veryfront-api-transport.ts | New shared transport implementing the common HTTP mechanics (retry/backoff integration, headers, tracing/metrics hooks, default response handling). |
| src/platform/adapters/veryfront-api-client/retry-handler.ts | Shrinks to a backward-compatible alias delegating to the canonical transport and re-exports compatible types. |
| src/platform/adapters/veryfront-api-client/operations.ts | Switches operations to use a cached canonical transport instance rather than calling requestWithRetry directly. |
| src/platform/adapters/token/veryfront/api-client.ts | Switches token-storage client to a transport instance with token-client-specific response/retry/error policy. |
| scripts/lint/module-boundaries-baseline.json | Updates baseline to reflect the reduced import surface after refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90f28e713c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…rls; cancel body before retry throw
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/platform/adapters/veryfront-api-transport.ts:176
- logTimeout() only redacts the first
token=query parameter occurrence because the regex is missing the global flag. If the URL contains multipletokenparams (or repeated), later values will be logged unredacted.
function logTimeout(url: string, timeoutMs: number, attempt: number): void {
log.warn("Request timed out", {
url: url.replace(/token=[^&]+/, "token=***"),
timeoutMs,
attempt: attempt + 1,
});
src/platform/adapters/token/veryfront/api-client.ts:78
- On the 404 fast-path, the Response body is not consumed or canceled before returning null. This can keep connections/buffers open longer than needed.
This issue also appears in the following locations of the same file:
- line 99
- line 121
const response = await this.transport.request(url);
if (response.status === 404) {
return null;
}
src/platform/adapters/token/veryfront/api-client.ts:125
- The early return on successful/404 DELETE does not consume or cancel the Response body. Cancel it before returning to avoid leaking resources.
const response = await this.transport.request(url, { method: "DELETE" });
if (response.ok || response.status === 404) {
return;
}
src/platform/adapters/token/veryfront/api-client.ts:103
- set() returns void and the response body is never consumed on success. Cancel the body after a successful PUT so the underlying connection/buffer can be released promptly.
const response = await this.transport.request(url, {
method: "PUT",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ value }),
});
Summary
Candidate #5 from the architecture review — the last of the seven. Two adapter stacks talk to the same
api.veryfront.com, each privately owning URL-building, Bearer-auth headers, trace injection, per-attempt timeout, and error mapping. This names the seam: oneVeryfrontApiTransportowns the HTTP mechanics; both stacks become policy over it.Design (reworked per review)
The first attempt wrapped
requestWithRetryin a third layer (+228 lines) and was rejected. This version replaces it:veryfront-api-transport.ts(203 lines) absorbs the formerrequestWithRetrybody — span-wrapped fetch, header assembly,injectContext, metrics, redacted timeout/retry logging, terminal-error wrapping — parameterized only where the token client genuinely differs (returns rawResponse, 4xx pass-through,TOKEN_STORAGE_ERROR, no spans/metrics).retry-handler.ts: 165 lines → a 20-line backward-compat alias.retryWithBackoffprimitive (refactor(errors): unify five retry implementations onto one retryWithBackoff primitive #3046).Accounting
+46 net (+289/−243, 5 files) — near-neutral, with the two private copies of HTTP mechanics gone. Bonus: module-boundary debt decreased by one broad import; the baseline is regenerated to lock the ratchet at the lower count.
Verification
deno task verify:quickexit 0 ·deno task test:unitgreen · module-boundaries green at the tightened baseline · pre-push (fmt + full suite) passedcli/mcp/tools/deploy-tool.test.ts("triggerDeploy happy path") — it passes in isolation, in its suite (260 steps ×2), and in the full-suite rerun; treating as parallel-run flake, unrelated to this diffGitHub client deliberately untouched (different host; separateness is load-bearing).