fix(cli): route HeyGen API calls through canary - #3201
Conversation
somanshreddy
left a comment
There was a problem hiding this comment.
Independent review at d6a8ed5e (post-merge — raising as fast-follow, not blocking a merged change; tagged by name so doing my own pass). The helper refactor is clean — it centralizes the previously-scattered heygen_route:"canary" from publish/feedback, the spread preserves other headers, and every path is test-pinned. Three risks, all in the scope of routing you asked reviewers to scrutinize:
1. No escape hatch — canary is unconditional, no prod override. withHeygenCanaryRoute sets heygen_route: canary after the spread, so it overwrites any route a caller passes and there is no env/flag to route back to prod. If canary ships a bad deploy, the fleet can't be fallen back to prod without cutting a new CLI release (publish + user upgrade). Cheapest hardening: const route = process.env.HEYGEN_ROUTE ?? "canary" and set the header from that — keep canary as the default if that's the intent, but keep a prod escape hatch. Note HEYGEN_API_URL overrides the base URL but not this header, so today nothing can opt a run out of canary.
2. Login + token lifecycle now depend on canary health (new in this PR). Per the summary, device auth / token exchange / refresh / revoke previously went to prod; this PR moves them to canary. So a canary regression now breaks login and silent-refresh for every CLI user even when stable prod is healthy — the exact failure mode this stack of work just lived through on the EF side. Worth decoupling auth-infra routing (keep on stable prod) from HyperFrames feature routing (canary), rather than routing the OAuth grants to canary too.
3. API-key (billing) traffic now routes to canary (new in this PR). The buildAuthHeaders API-key branch — whose own comment says it "keeps the normal billing path" — now carries heygen_route: canary, so a customer scripting the CLI with their API key has their billed production calls served by the pre-release deployment. Bigger blast radius than OAuth CLI usage; confirm that's intended for paid/customer traffic (mild contradiction with the "normal billing path" comment right above it).
Scope note. The stated goal — "a normal CLI E2E can validate an EF canary release" — is a testing capability that finding #1's opt-in override satisfies directly: the E2E sets HEYGEN_ROUTE=canary, real users default to prod. That gets you canary-in-E2E without every released-CLI user (and their paid API calls) permanently living on canary. If HyperFrames-on-canary is instead a deliberate product posture (publish/feedback were already canary pre-PR), #2 and #3 still deserve an explicit sign-off since they extend that posture to auth + billing.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at d6a8ed5e (post-merge — fast-follow, matches Somu's independent read at the same SHA).
On the scope question. Somu already named the three concerns I would raise at this head, at the right resolution — no escape hatch, auth-lifecycle now depends on canary health, and API-key/billing traffic contradicts the "normal billing path" comment right above the branch. I agree with all three and won't restate them; treat this as a second confirming voice for the fast-follow. The HEYGEN_ROUTE ?? "canary" shape he sketched is the cheapest fix and lets the stated goal (E2E-in-canary) survive without every released-CLI user living permanently on canary.
Two things I did verify that complement his review, in case they help the follow-up scope:
Coverage completeness — the "consistent" claim holds. Full audit of the CLI's HeyGen-API-touching call sites at this head:
- Covered via
withHeygenCanaryRoutedirectly:oauth.ts:257(device issuance),:305(device polling),:396(refresh),:448(revoke),:511(auth-code exchange),publishProject.ts:558/621/650(direct + presign + complete),submitFeedback.ts:34. - Covered transitively via
buildAuthHeaders:AuthClient.fetchUserfor/v3/users/me(client.ts:134), and every cloud call routed throughcloud/_gen/client.ts:100(which pulls headers fromcloud/auth.ts:30'sbuildAuthHeaders(credential)). - Correctly excluded:
cloud/upload.ts:96(presigned S3 PUT — extra headers invalidate the signature, called out in the file's own comment at:80-82),cloud/download.ts:45(presigned GET, same reason),registry/remote.ts+registry/localSemantic.ts:112(fetchesraw.githubusercontent.com/heygen-com/hyperframes/main/registry, not HeyGen API),telemetry/transport.ts(PostHog),updateCheck.ts:71(npm registry),capture/contentExtractor.ts:303(OpenRouter),studioSelectionClient.ts(localhost). No missed HF-API paths on this repo scan.
TDD claim holds. The 6 RED tests at the pre-implementation snapshot were: client.test.ts:87 (OAuth headers), :95 (API-key headers), oauth.test.ts:197 (refresh), :312 (revoke), :346-359 (auth-code exchange), :543-548 (device issuance + 3 polls). All six read the heygen_route: "canary" expectation via toEqual or toMatchObject, so a revert of the withHeygenCanaryRoute calls would fail each one. Test-name / expectation alignment with the surfaces the PR claims to add is 1:1 — no test-name-vs-fix drift.
Small drafting nit for the follow-up. In publishProject.ts:558, :621, :650 and cloud call sites that go through buildAuthHeaders, heygen_route: "canary" ends up being set twice — once by the shared helper wrapping buildAuthHeaders's return, once by the outer withHeygenCanaryRoute(authHeaders) at the callsite. Idempotent (both write the same value), so no functional bug — but if the follow-up introduces an env-override, the double-wrap will silently mask an override written at the outer callsite when the auth-headers wrap overrides it back to "canary". Cleaner to have one canonical spot per request (the outer wrap is enough; the auth-headers helper wouldn't need to also wrap).
Otherwise, the shared helper is the right shape and the surface consolidation was cleanly done. Merge-decision context leaves this as fast-follow input, per the standing rule. Stamp routing per standing rule.
Summary
HyperFrames now consistently exercises EF canary for CLI-owned HeyGen API traffic. Previously publish and feedback requests carried the canary route header, while device authorization, token exchange/refresh/revoke, user verification, and cloud calls silently went to prod—so a normal CLI E2E could not validate an EF canary release.
A shared route helper now covers every relevant request path, with regressions that pin issuance and every poll plus the other OAuth grants and both credential types. Browser navigation and third-party presigned uploads remain untouched.
Test plan
bun run --cwd packages/cli test— 2,582 passed, 3 skippedbun run --cwd packages/cli typecheckbunx oxlint <changed files>bunx oxfmt --check <changed files>bun run build