Repository navigation
feat(cli): accept host-managed HeyGen access tokens - #5177
Conversation
Edit accuracy: accurate 2059 (base branch 2059), smooth 1570 of thoseThe gate passes. Quarantined, measured but not gated (0) |
There was a problem hiding this comment.
Review at 1d557064. COMMENT, not an approval. This head can't merge: GitHub reports mergeable: CONFLICTING. Main took #5181 (21d14b25b, "reach HeyGen through a host app's gateway (HEYGEN_API_BASE)") about an hour ago, and it rewrites the same media-use host/credential code. git merge-tree shows conflicts in skills/media-use/audio/scripts/lib/heygen.mjs, heygen.test.mjs and skills-manifest.json. The rebase will produce a new head, and the media-use half needs a real reconciliation rather than a mechanical one.
Correction (after tai's CR 5448413550): I first called the CLI half clean, and I missed one finding. tai's item 1 is real; I confirmed it in the code. After a browser login, runOAuthLogin → reportIdentity() (commands/auth/login.ts:231) re-resolves through tryResolveCredential(). With HEYGEN_ACCESS_TOKEN for account A still in the environment, it identifies A, then persistUserInfo writes A's user block next to the account-B tokens that were just saved, and both the login-completed telemetry event and the user identity it attributes record A. Nothing in auth login checks for an env credential first.
- The same pitfall already existed with
HEYGEN_API_KEY, but this PR makes it reachable from inside Desktop, where the agent's environment carries the token. - Fix: identify the account from the tokens the flow just returned, or refuse or warn in
auth loginwhile an env credential is set. Add a dual-account test.
tai's item 2 does not reproduce on Linux. packages/cli/src/audio/scripts is a tracked symlink (mode 120000) to ../../../../skills/media-use/audio/scripts, and a fresh worktree at this head passes heygen.test.mjs 11/11. But it does mean the "both distributed helpers" loop imports the same source file twice, so it doesn't prove a second, distributed copy. A checkout without symlinks (Windows core.symlinks=false) would fail that import.
CLI half (packages/cli): resolver and refresh are correct; see the correction above for login.
HEYGEN_ACCESS_TOKENresolves after the two API-key env vars and before the store. It isrefreshable: false, sorefreshIfNeededandforceRefreshCredentialsboth leave it alone: there's norefresh_token, and nothing writes it to the store.auth statuslabels itenv (HEYGEN_ACCESS_TOKEN), andisFileSourcekeeps it out of the file-only rows.headerSafeEnvkeeps the control-character refusal for all three env vars.- Tests: vitest
resolver.test.tsandstatus-user.test.tspass, 22 tests. - Mutants, all caught (4/4):
refreshable: true;- the token moved ahead of the API keys;
- the token ignored;
- the header-safety check dropped.
Media-use half: must be redone on top of #5181.
- On main,
heygenBase()is evaluated per request fromHEYGEN_API_BASE, and #5181 added two guards around it:HOST_ONLYstops a project.envfrom setting the base;- a base outside
heygen.comgets no stored or host OAuth credential.
- This PR's
HEYGEN_API_URLpath has neither guard. At this head it's safe only by accident:HEYGEN_BASEis computed at import, beforeloadEnvFromDirruns. I probed it: a.envwithHEYGEN_API_URL=https://attacker.exampleleaves the base atapi.heygen.com. - If the rebase folds
HEYGEN_API_URLinto main's lazyheygenBase(), a cloned project's.envcould point the person's~/.heygenOAuth token or shell key at any host. That is the exact hole #5181's last commit closed forHEYGEN_API_BASE. - So either:
- (a) drop the media-use host change and have Desktop #3056 set
HEYGEN_API_BASE, the variable main already honors; or - (b) route
HEYGEN_API_URLthrough the same rules: add it toHOST_ONLY, apply the heygen.com-only credential check and theHEYGEN_ALLOW_HTTPrule, and add a test that a.envvalue is ignored.
- (a) drop the media-use host change and have Desktop #3056 set
- Option (a) also avoids two env vars that mean "media host".
- At this head, the media-use tests pass (11/11), and mutants hard-coding the host or dropping the trailing-slash strip are caught (2/2).
Nits
- Precedence differs between the two halves. With both
HEYGEN_API_KEYandHEYGEN_ACCESS_TOKENset, the CLI uses the key while media-use uses the token, so the two halves of one Desktop agent run can bill different accounts. The body says this is deliberate. If Desktop can inherit a shellHEYGEN_API_KEY, the signed-in user's token loses in the CLI. - Docs.
skills/hyperframes-cli/references/cloud.md:33and thehyperframes authhelp's ENV VARS list don't mentionHEYGEN_ACCESS_TOKEN.
CI: 92 passing, 2 skipped, none red, at this head. The required checks will need to re-run after the rebase.
Verdict: I can't approve. The login identity mismatch above needs a fix, the head conflicts with main, and the media-use half has to be reconciled with #5181's host guards. Re-ping me with the rebased head and I'll check the conflict resolution.
— Somu
jrusso1020
left a comment
There was a problem hiding this comment.
Not approving 1d5570646f. GitHub reports this head as conflicting with main, so it can't merge as it stands, and the rebase will need a new head. This adds to somanshreddy's review and agrees with it.
The media-use change should be dropped, not reconciled
main already covers this half. Since #5181 (21d14b25b), skills/media-use/audio/scripts/lib/heygen.mjs sends a host-injected HEYGEN_ACCESS_TOKEN as Bearer and moves every request to $HEYGEN_API_BASE. That host override comes with the guards this PR's HEYGEN_API_URL lacks:
HOST_ONLYstops a project.envfrom setting the base.heygenOwnBase()keeps~/.heygencredentials onheygen.comhosts.- Plain HTTP is refused unless
HEYGEN_ALLOW_HTTP=1.
If HEYGEN_API_URL is folded into main's per-request heygenBase(), it becomes a second, unguarded way to point the shared credentials somewhere else. Dropping the heygen.mjs hunk and its manifest hash, and having the host set HEYGEN_API_BASE, gets the same result with no new surface. It also fixes the header comment, which this PR changes from "matches the hyperframes CLI auth" because the two orders differ: media-use tries the access token before the API-key variables, and the CLI tries it after them.
CLI half: fine as written
HEYGEN_ACCESS_TOKENresolves after both API-key variables and before the store. It is header-checked through the newheaderSafeEnv, and an empty value falls through as before.- Callers of
resolveCredentialarecloud/auth.ts,commands/cloud.ts,commands/publish.tsandutils/publishProject.ts.refreshIfNeededreturns early without arefresh_token, and the forcedrefreshable: trueon a 401 inpublishProject.ts:193also has no refresh token, so an env token is never refreshed or written to the store. - Behavior only changes for someone who already exports
HEYGEN_ACCESS_TOKENin their shell. That token will now win over a valid~/.heygen/credentials. That fits the documented order, but the release note should mention it. - Nothing here is on the render path. No file under
packages/coreorpackages/producerchanges, and neither package imports the resolver.
A question for the host app
The CLI already works through a host gateway as it stands. auth/client.ts honours HEYGEN_API_URL, and an API-key credential goes out as x-api-key, which a gateway can check against its own per-run token. If the embedding app gives the agent its gateway base and a gateway token instead of the account's access token, this PR isn't needed for that app, and the access token never reaches the agent's environment. Please confirm the host-managed token is the intended design before this ships in a release.
CI. 85 checks passed and 2 were skipped at this head. Those results predate the conflict, so they need a fresh run after the rebase. I didn't run the CLI suites locally; somanshreddy's review covers them.
— Rames
There was a problem hiding this comment.
UPDATE: This initial COMMENT is superseded by my changes-requested review for the CLI login identity mismatch. I retracted the later test-path claim after verifying a tracked symlink. The earlier host/conflict analysis remains applicable.
Review at 1d5570646f74b2a487c4fc140b836d7ed90a63ee — not approving this head.
I checked the live PR and the credential/host patch. GitHub reports mergeable: CONFLICTING against current main, which includes #5181. This head cannot merge; the reconciliation will change the reviewed code. The CLI resolver adds HEYGEN_ACCESS_TOKEN after the two API-key environment sources and before the credential store, with refreshable: false. That changes existing caller precedence if the environment already supplies this token; it is not a blanket compatibility guarantee from green tests.
The media-use hunk adds HEYGEN_API_URL as a module-time host without main's #5181 HEYGEN_API_BASE guards for project .env provenance, credential destination, and HTTP opt-in. I did not establish that a project .env can redirect credentials at this head: the direct entrypoints import the helper before loading their project .env. But folding this extra host variable into main's per-request host selection during conflict resolution without those guards would weaken its credential boundary. The safer resolution is to drop the media-use host hunk and have Desktop use main's existing HEYGEN_API_BASE; otherwise apply the same guards and test them. Somu and Rames have detailed at-head reviews of the overlapping issue.
All 11 required checks pass at this head, but they predate the necessary conflict resolution. I did not run the local suites or inspect production request traffic; this is a client-side environment/credential path, and those checks cannot establish how a future conflict resolution treats a project .env. Please re-ping with the rebased head for a fresh verdict.
— tai
There was a problem hiding this comment.
CORRECTION: I retract the earlier test-path finding in this review. packages/cli/src/audio/scripts is a tracked symlink to skills/media-use/audio/scripts, so the import resolves in a checkout that preserves symlinks. Somu reports the 11 media-use tests pass. My ERR_MODULE_NOT_FOUND reproduction used a checkout without that symlink; treating the unexpanded Git tree path as proof of absence was wrong. The login identity mismatch below remains a changes-requested issue.
Review at 1d5570646f74b2a487c4fc140b836d7ed90a63ee — requesting changes after a follow-up source audit. This supersedes my earlier COMMENT review and adds a CLI login defect beyond the already reported merge conflict and host-guard reconciliation.
packages/cli/src/auth/resolver.ts:62-65 introduces an environment OAuth credential ahead of the file credential. Browser auth login persists its newly issued OAuth token to the file (auth/oauth.ts:208-209), then commands/auth/login.ts:228-253 calls tryResolveCredential() to report the identity. With HEYGEN_ACCESS_TOKEN for account A still set while someone completes browser login for account B, the resolver selects A, /v3/users/me returns A, and saveUserInfo writes A's user block alongside B's newly saved OAuth token (auth/user.ts:73-81). The command and telemetry report A as the completed login; after the environment token is removed, the credential and cached identity disagree. A pre-existing API-key environment source had a similar precedence pitfall, but this PR makes the host-managed token a new way to trigger it. Identify the credential from the just-completed login rather than re-resolving the global priority list, and cover the dual-account case.
As detailed in my earlier review and by Somu and Rames, GitHub still reports this head CONFLICTING with main/#5181. A rebased media-use host selection must retain main's project-.env provenance and credential-destination restrictions; I have not found a project-.env redirect through the current direct entrypoints at this unreconciled head. The CLI's API-key precedence and non-refreshable host token are otherwise as intended. All 11 required checks report green on this old head, but they do not validate the rebased resolution. I did not run the full local suites or inspect production request traffic.
— tai
Identify the account from the tokens the login issued, not a fresh credential lookup that an environment token or API key outranks.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
Publish told a host-token user to log in again, which cannot help while the token outranks the login.
One map of environment credential variables, owned by the resolver, now feeds the logout warning and both publish messages.
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at af4eaae4. Approving. Both findings from the 1d557064 round are fixed. I read 0d31f991 in full and the five commits after it as deltas.
Prior findings
- Media-use host hunk: dropped. The PR no longer changes anything under
skills/media-use. Media-use stays on main'sHEYGEN_API_BASEpath, with itsHOST_ONLY,heygenOwnBase()and HTTP opt-in guards, so there's no second, unguarded host variable. GitHub reports the head as mergeable, andgit merge-treeagainst current main is clean. - Login identity mismatch: fixed.
runOAuthLoginnow keeps thetokensthatstartAuthorizationCodeFlow()returns (oauth.ts:210) and identifies the account fromissuedCredential(tokens). It no longer re-resolves, so an envHEYGEN_ACCESS_TOKEN(orHEYGEN_API_KEY) can't outrank the tokens this login just issued.- The device path uses the same helper.
- The 401 refresh hook still works because
AuthClientgates refresh onrefresh_token(client.ts:139), not onrefreshable, and the issued credential carries the refresh token.
- Docs: updated.
authhelp,cli.mdxandskills/hyperframes-cli/references/cloud.mdall listHEYGEN_ACCESS_TOKENin the resolution order.
Since 0d31f991
-
auth logoutnow names whichever env credential is still active, includingHEYGEN_ACCESS_TOKEN. To do that,ENV_CREDENTIAL_VARmoved intoresolver.tsand is shared by logout, publish and the 401 message, so the three can't drift apart. There's a new test, and dropping theenv_oauthentry fails it. -
af4eaae4is a pure refactor. The resolver,auth statuslabels, logout, publish and the 401 message now all read the variable names fromENV_CREDENTIAL_VAR, which is typedRecord<EnvSource, string>, throughenvCredentialVar(source). No user-facing string changes. -
The auth guide now says the media order applies to the bundled audio scripts, and that
media-use resolvegoes through the separateheygenCLI. -
When publish gets a 401 with a host token, it now says
HEYGEN_ACCESS_TOKEN was rejected. Fix or unset itinstead of asking for a login that would never be used.ENV_CREDENTIAL_VARcovers all three env sources, and there's a test for it. Inpublish.ts, the--update/--spacebranch runs only for non-OAuth credentials, so theenv_oauthentry there is never used and does no harm. -
docs/guides/authentication.mdxnow gives media-use's real order:HEYGEN_API_BASE+ key, then the access token, then the API keys, then the store. That matches main'sheygen.mjsheader. It also says a project.envnever setsHEYGEN_API_BASE, and that the CLI checks the keys first.
Verification
- Ran locally at
af4eaae4:resolver.test.ts,login.test.tsandlogout.test.tspass, 43 tests in total. - I couldn't run
status-user.test.tsorpublishProject.test.tswith the dependencies on hand: they failed to import@hyperframes/engine/system-memoryandignore, and instatus-user.test.tsthat includes the tests that predate this PR. CI covers both files. - Mutants, all caught (3/3):
- reverting
login.tsto re-resolve the credential fails the dual-account test; - reading
HEYGEN_ACCESS_TOKENwithoutheaderSafeEnvfails the control-character test; - moving the token ahead of the API keys fails both precedence tests.
- reverting
- CI: all 11 required checks are green at
af4eaae4. They were also all green at55b48e58and4fc98c03. At0d31f991, theTestaggregator was red only because the newer push cancelled its studio shard (studio=cancelled).
Non-blocking
- Dropping
no_credentialfromAuthLoginFailureReasonis correct, because the login no longer re-reads the store. Any dashboard that groupsauth_login_failedby reason will just stop seeing that value. - The precedence difference is now documented, but it still exists, and it lives in main's media-use code rather than in this PR. If a host sets both a key and the token, the CLI and media-use can bill different accounts. Worth a line in the release note alongside "an exported
HEYGEN_ACCESS_TOKENnow wins over~/.heygen/credentials". - My earlier question still applies to the embedding app's design rather than to this PR: should it hand the agent a gateway base and a per-run token, or the account's access token? The CLI change is sound either way.
terencecho's CHANGES_REQUESTED at 1d557064 is still live, so this approval does not open the merge gate on its own.
— Rames
|
New head Conflict with main / #5181 (tai, Somu, Rames). I merged main in without a force-push ( Login reports the wrong account (tai; confirmed by Somu). Fixed at the root in Test import path (tai, retracted). Agreed it resolves through the tracked symlink. The test is removed along with the media-use change. Nits (Somu).
Three more fixes from my own review of the new head.
Release note (Rames). Added to the body: a shell that already exports Whether the host-managed token is the intended design (Rames). Yes. Miguel confirmed on 2026-10-08: keep the host-managed access token as built, with no gateway token. |
somanshreddy
left a comment
There was a problem hiding this comment.
Re-review at af4eaae4, covering the merge of main (6e2f8f48) plus the 6 commits after it. Every point from my review 5447470999 at 1d557064 is resolved.
- Conflict / media-use
HEYGEN_API_URL: media-use is now exactly main's.git diffagainst current main is empty for media-use, andgit merge-treeagainst current main is clean, even though main is 4 commits ahead. That means #5181's HOST_ONLY.envskip and its heygen.com-only credential guard are the only path, so the lazy-rebase concern is gone. - tai's
reportIdentitybug (which I confirmed): fixed. Both browser and device login now verifyissuedCredential(tokens), the tokens that login just received, so an envHEYGEN_ACCESS_TOKENcan't be cached as the new account's identity.- The refresh hook still works.
AuthClientrefreshes on 401 whenever the credential has arefresh_token, regardless ofrefreshable.
- The refresh hook still works.
- Host token is named, not "login expired":
envCredentialVarnow coversenv_oauth, sopublish --update/--spaceand the publish rejection message nameHEYGEN_ACCESS_TOKEN. Logout's warning lists every active env name from the resolver's single map.
Tests: CLI resolver, login, logout, status-user and publishProject pass 101/101 locally.
Mutants, 3/3 caught:
reportIdentityre-resolving instead of using the issued tokens;- logout's warning missing
HEYGEN_ACCESS_TOKEN; - the publish rejection naming only API-key env vars.
CI: still running at the time of review (24 checks pending, 117 green so far). tai's CHANGES_REQUESTED at 1d557064 is still open; it's tai's to clear.
— Somu
Requested on an older commit; later commits answer it (see the thread). Current head is af4eaae.
What
Accept host-managed OAuth access tokens in the CLI, and report the right account after a browser sign-in while one is set.
Why
An embedding application can own sign-in and token refresh without writing its credentials into the shared CLI store. Previously the CLI ignored that token.
Related work
#5181 already lets media-use send a host-injected
HEYGEN_ACCESS_TOKENand move its requests withHEYGEN_API_BASE, behind its project-.envand destination guards. This PR leaves media-use exactly as main has it; a host that needs a non-production API setsHEYGEN_API_BASEfor media-use. The embedding application owns whether to supply the token alongside existing shared credentials.How
HEYGEN_ACCESS_TOKENafter the API-key environment variables and before the shared store. It is never refreshed or persisted, andauth statusreports its environment source.auth loginnow identifies the account from the tokens the login just issued, in both the browser and device flows. Before, the browser flow resolved credentials again, so an environment token or API key for account A outranked the new account B login: A's identity was saved next to B's tokens and reported in telemetry. That lookup could no longer come back empty, so itsno_credentialfailure reason is removed.publishnames a rejectedHEYGEN_ACCESS_TOKENinstead of asking for a login it would not use, andauth logoutwarns that the token still signs commands. One map of environment credential variables in the resolver feeds both.authhelp,docs/packages/cli.mdxand the CLI skill's cloud reference list the token in the resolution order.docs/guides/authentication.mdxnow shows the media workflows' real order from main, which checks the host token before the API keys, and says thatmedia-use resolvesearches through the separateheygenCLI.Release note: someone who already exports
HEYGEN_ACCESS_TOKENin their shell will now have it used ahead of~/.heygen/credentials.Test plan
On Linux, at this head:
b@example.com, gota@example.com) and passes with it.Earlier on this branch: the resolver and auth-status tests passed three consecutive runs, and deliberate mutations of the resolver order, refresh flag and header check each failed their tests. No real service or published package was exercised.