Repository navigation
fix(cli): two CLI fixes from Wenbo's reverse test (paywall link, plan label) - #7099
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 66d1793a45a33c8055bee8e1b462241a4bb24707.
The Pro/credits refusal now carries the same page URL through Refused → Fix → runMeasure JSON while retaining the existing fix text (packages/cli/src/heygen/v3.ts:104–119; measure/command.ts:21–30). The auth JSON notice explicitly distinguishes the HyperFrames plan from the HeyGen app subscription when the HyperFrames OAuth plan is shown (commands/auth/status.ts:227–230). The update change reuses the existing due-check/cache and opt-outs with a one-hour interval (utils/updateCheck.ts:17), without adding another cache or scheduler.
Validation: full root bun run build passed. bun run --cwd packages/cli test src/commands/auth src/heygen/v3.test.ts src/measure/command.test.ts src/utils/updateCheck.test.ts reports 122 passed / 7 files. Three separate offline control groups execute the generated client → runJob → refusal serializer, real auth store/resolver/status across four credential types (including zero credits), and cache behavior at 59.99/60/60.01 minutes. Supplied/fallback URLs still match the legacy fix text; a non-paywall 401 omits url. Opt-out sends no registry fetch; HTTP failure, invalid JSON, unsafe versions and network failure leave the cache unstamped; successful writes preserve concurrently changed preferences. I inspected and reran these independent controls with bun review-artifacts/7099/adversarial/controls.ts in the review workspace. Network/DNS and update-config I/O are mocked; credential files are isolated fixtures.
Limits: no live subscription, paywall UI, registry freshness or end-to-end reverse run was exercised. This improves machine-readable guidance; it does not prove how an agent will relay it. The auth clarification is scoped to the existing HyperFrames OAuth notice branch. Hourly cache expiry permits up to an hour of cached metadata; it does not guarantee real-time registry freshness.
Need: yes · Size: bounded nine-file change · Reuse: existing Fix serializer, auth notice and update cache · Simpler: already minimal · Structure: fine
Risk: low — additive guidance fields; normal metadata refresh cadence rises from daily to hourly with the existing opt-outs.
Verdict: APPROVE
Reasoning: The three fixes fit the existing consumers and preserve their contracts; no blocking regression found in the reviewed change.
— hf-review
Edit accuracy: accurate 267 (base branch 267), smooth 81 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (1)
|
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 3ee1e9f7f09137a4ec4881f0996a39f2824c6a43, delta from my approved 66d1793a4.
The revert is exact and limited to packages/cli/src/utils/updateCheck.ts and its test: both files are byte-identical to the PR diff's merge-base 12ce8c2ffd37757204afa0ab170df27e0a384162. All seven remaining PR files—including the refusal URL producer/serializer, auth notice and their tests—are byte-identical to my approved head. The update-interval change is no longer part of this PR.
Validation: git diff 66d1793a4 HEAD, git diff --check and per-path Git blob comparisons verify those identities; the full remaining PR diff contains exactly the seven retained files. My prior build, 122 focused tests and three offline control groups ran at 66d1793a4, not this head. They are not represented as a new execution. This re-review uses exact source/test parity for the retained fixes and an exact restoration of the update files; no new build, tests, live auth or paywall run.
Need: yes · Size: narrowed seven-file change · Reuse: existing refusal serializer and auth notice · Simpler: removes the unrelated cadence change · Structure: fine
Risk: low — retained fixes unchanged; update files restored exactly to base.
Verdict: APPROVE
Reasoning: The revert removes only the update-check change and preserves both previously reviewed fixes without modification.
— hf-review
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Reviewed the retained 3ee1e9f delta after the hourly update-check change was reverted. The auth-status notice correctly distinguishes the HyperFrames account from the separate HeyGen subscription, and the relevant hosted CLI tests executed. The new paywall URL field still conflates an explicit server null with an omitted URL and fabricates a purchase link, contrary to the merged #7031 no-link precedent; see inline for the concrete JSON path and regression. No local payment test or merge. — Jerrai
| // The fix is the one line the agent relays as printed; the problem says how to wait for the person. | ||
| if (/hf_subscription_required/i.test(code)) | ||
| if (/hf_subscription_required/i.test(code)) { | ||
| const link = url ?? pageUrl("plans", { needed }); |
There was a problem hiding this comment.
The new Refused.url surfaces url ?? pageUrl(...) to agents, but the parser at line 98 collapses explicit error.url: null into missing. If the server withholds a purchase URL (e.g. purchasing is closed to this account), hf_subscription_required or hf_usage_exhausted now emits a fabricated plans/credits link in the JSON url and fix. The merged reverse upgrade_url:null contract in #7031 deliberately distinguishes null from an absent older-server field and names no purchase link/command in that case. Please preserve that distinction for these v3 refusals and add string/missing/null cases; v3.test.ts currently covers only string and missing. runMeasure forwards this URL into command JSON, so a consuming agent will see the fabricated link.
|
Thanks Jerrai, fixed in d88ec54. An explicit
|
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at d88ec54663889d5d64facc59f6e1565b5864eb50, delta from my approved 3ee1e9f7f.
Jerrai's explicit-null finding is fixed. The two HyperFrames paywall branches now return before constructing a fallback page when the server gives url: null (packages/cli/src/heygen/v3.ts:99–124): neither the JSON URL, message nor fix offers a purchase link or billing command. An omitted older-server field still gets the fallback; a supplied string remains unchanged. Refusal status and retry-key handling still pass through the existing logic. No new blocker found.
Validation: full root bun run build passed. bun run --cwd packages/cli test src/commands/auth src/heygen/v3.test.ts src/measure/command.test.ts — 83 passed / 6 files. Replacing only v3.ts with the prior approved version makes both new null-URL cases fail; source restored. Independent generated-client → runJob → runMeasure controls, inspected and rerun by me: 19 groups / 38 mocked requests, covering nested/flat errors, string/missing/null URL, suppressed server message links, zero balance, no credentials and start/poll retry-key behavior.
No live payment, purchase UI or spawned-CLI acceptance run performed. The retained auth-status notice is unchanged from the earlier review.
Need: yes · Size: two-file incremental change · Reuse: existing refusal producer and JSON serializer · Simpler: early no-purchase returns · Structure: fine
Risk: low — null-aware guidance without changing charge execution.
Verdict: APPROVE
Reasoning: Explicit server refusal to provide a purchase URL is now preserved through the actual command serializer, while string and older-server behavior remain intact.
— hf-review
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Approved at d88ec54663889d5d64facc59f6e1565b5864eb50. Explicit error.url:null now returns a no-purchase-link refusal for both subscription and credits; supplied links remain intact, omitted older-server fields retain their fallback, and the CLI JSON omits absent URLs. The new null tests cover the refusal property, message, and serialization; the auth-status notice remains additive. Relevant hosted CLI/Windows tests and lint ran green. The required aggregate Test is currently red because an unrelated packages/engine/src/services/browserManager.test.ts 5-second timing test failed under the same head (1 failed, 2,779 passed in the engine shard); this PR changes CLI files only, not engine behavior. That test still needs a successful rerun before merge. No payment, merge, or release by me.
— Jerrai
An agent hit the Pro paywall, found the link only inside the fix sentence, and handed the person a shell command instead of the page. The link now also comes as url.
An agent read user.subscription.plan (the HeyGen app's pro) as the HyperFrames plan and ran past Free. With the HyperFrames plan read, billing_notice now names which is which.
HeyGen sends url: null when it can't sell to this account right now. The refusal now says so and carries no link, as reverse start does since #7031; a missing url (older server) still gets the plans or credits page.
d88ec54 to
88d2f7e
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 88d2f7ebd9c0008f8a0f841c465a6e7784dbf4a5, rebase delta from my approved d88ec54663889d5d64facc59f6e1565b5864eb50.
Code-clear on the reviewed rebase. git range-diff 12ce8c2ffd..d88ec5466 ac1948a88..88d2f7ebd confirms the auth-status patch is unchanged, the update-check change/revert disappear, and the refusal patch preserves main's asynchronous pageUrl and “Buy add-on credits” / billing add-credits wording. A repository-wide rg census and current-head source audit confirm all five production billing pageUrl call sites await their results (billing.ts:105, auth/status.ts:258, heygen/reverse.ts:63, heygen/v3.ts:112,125), as do both v3 refusal callers (v3.ts:194,317). Jerrai's prior explicit-null finding remains fixed: the two paywall branches (packages/cli/src/heygen/v3.ts:106–130) return without a purchase link or billing command when url is null; missing fields retain the fallback and supplied strings remain intact.
Integration hold: this head conflicts with current main f3e5b81dfebb2460338dfefd31d4e3509b05ca08 in packages/cli/src/heygen/v3.ts and v3.test.ts, independently reproduced by git merge-tree --write-tree HEAD origin/main. Main added skill-aware checkout links in the same branches. Resolve by retaining that attribution plus this PR's null early returns and the same resolved link in both fix and url, then re-pin for review. This is an integration condition, not a new code defect.
Current tests positively pin the auth notice (commands/auth/status-user.test.ts:152), supplied URL fields and explicit-null branches (heygen/v3.test.ts:161–199), and JSON URL serialization (measure/command.test.ts:17). These were inspected, not executed at this head.
Hosted status: the only checks attached to this head are successful WIP and Graphite checks; no GitHub Actions build/test execution is attached. The Miao handoff for this thread explicitly asks for approval only once hosted CI is green or clearly passing; these two checks do not establish that. No new full build or native tests: the focused test attempt could not load Vitest because dependencies are absent. Prior executed tests belong to the prior approved head and are not claimed for this rebase. No paid calls or live auth exercised.
Need: yes · Size: ok · Reuse: existing Fix serializer and plan helper · Simpler: already minimal · Structure: fine
Risk: low — bounded refusal serialization and auth guidance; current integration resolution is unreviewed.
Verdict: COMMENT
Reasoning: The reviewed rebase preserves the prior fixes, but a conflict resolution and the explicitly requested hosted validation are still needed before restoring approval.
— Rames (hf-review)
# Conflicts: # packages/cli/src/heygen/v3.test.ts # packages/cli/src/heygen/v3.ts
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at b13c33e74ab35c1d873c90c410bd9b6874717c81, including the merge of main f3e5b81df into the previously reviewed head.
The conflict resolution preserves main's CLI/media-use checkout attribution and awaited pageUrl, while retaining the same resolved link in fix and url (packages/cli/src/heygen/v3.ts:114–136). Jerrai's prior explicit-null concern remains fixed: both paywall branches return before building a purchase link, and omit the JSON url field, with message/fix containing no purchase URL or billing command (v3.ts:107–113,122–128). The five non-conflict files are unchanged from the previous head; the full main-relative patch still contains only the seven intended files. The caller audit covers billing.ts:133, auth/status.ts:279 and v3.ts:116,131; reverse.ts:71 returns the promise from async reverseUpgradeUrl and every caller awaits that helper. Both refusal callers await it. No new blocker found.
Validation at this exact head: frozen install and full root build pass; focused HeyGen/auth/billing/measure tests report 207 passed / 12 files. I inspected and replayed the independent generated-client → runJob → runMeasure controls pass 16 offline groups, covering both refusal codes, nested/flat errors, missing/null/own/external URLs, workspace hints, attribution and identical JSON/fix links. Every network request in those controls was mocked; no paid call or live auth/paywall run.
Hosted snapshot: Build, Typecheck, both CLI shards, required CLI smoke and Linux/macOS/Windows CLI shims pass. All hosted checks are now successful, skipped or neutral at the reviewed head, including both Windows engine/CLI shards and their runtime-contract step (Windows run). The explicit hosted-validation condition and the prior merge-conflict hold are both resolved.
Need: yes · Size: bounded seven-file change · Reuse: existing plan helper and Fix serializer · Simpler: early no-link returns and one resolved link · Structure: fine
Risk: low — additive refusal JSON and plan-label guidance; no live purchase flow exercised.
Verdict: APPROVE
Reasoning: The merge resolution preserves both parents' contracts and resolves my previous integration hold, and passes current-head build, focused tests and offline serializer controls.
— Rames (hf-review)
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Approved the exact resolved-rebase head. The seven-file authored patch retains the no-link refusal for explicit error.url:null, preserves supplied checkout URLs, and uses the same skill-attributed resolved link in both fix and url while awaiting main’s async pageUrl. The auth-status notice and JSON serializer remain additive. Hosted CLI and Windows jobs, typecheck and build are now terminal green; Rames independently approved the same head. I did not run a checkout, pay, merge or change auto-merge.
— Jerrai
Two CLI fixes from Wenbo's end-to-end reverse test on 0.9.8 (findings F1, F2): https://www.heygenverse.com/a/reverse-style-sampling-findings-45a4fd9e-fb0a-4484-b12e-9289afa6868f
F1: the paywall link has its own field. A Pro or credits refusal had its page link only inside the
fixsentence (Subscribe: https://…). Wenbo's agent didn't show it and handed him a shell command instead.Fixnow takes an optionalurl, the two paywall refusals fill it, andrunMeasure's JSON prints it next tofix.fixis unchanged.F2:
auth statussays which plan is which.--jsonprints the HeyGen app'suser.subscription(pro) besidehyperframes(none). The agent read the first as the HyperFrames plan and ran past Free. With the HyperFrames plan read,billing_noticenow sayshyperframesis the plan anduser.subscriptiondoesn't unlock HyperFrames features. Before, that case had no notice.The update-check interval change (F14) was reverted in 3ee1e9f. #7094 changes the same interval as part of updating the CLI and skills together, so it's left to that PR.
Tests:
v3.test.ts(url on both refusals),measure/command.test.ts(url printed),auth/status-user.test.ts(the notice).