Repository navigation
fix(skills): reverse hands billing commands to the person instead of running them - #7314
Conversation
…running them The Snyk audit on skills.sh still fails reverse-any-video on E004 because the skill has the agent run `billing subscribe` and `billing add-credits` itself. media-use, which hands the person the billing command instead, passes the same audit. reverse now does the same: on Free it gives the upgrade link and the subscribe command for the person to run, and mid-run it gives one add-credits command for the run's total. It waits until they say they're done, then starts again or relaunches the refused steps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HebQXQjEiSeVmhvTbJvSAw
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at ed948a8
The Free and exhausted-credit sections explicitly forbid agent-run billing and hand the person the purchase link/command (skills/reverse-any-video/references/start.md:41–52,65–73). The subscribe command stays verbatim with --feature reverse --skill reverse-any-video --session <id>; the combined top-up retains --skill reverse-any-video. Waiting for all launched answers and relaunching only refused steps leaves successful shots alone (start.md:66–70); the unchanged generation path derives its retry key from request/media (packages/cli/src/heygen/generate.ts:687), and reverse start reads the saved session (packages/cli/src/commands/reverse/start.ts:117–119).
Nit — strengthen the regression pins: scripts/check-core-skills.test.mjs:51–55 still accepts the old and run ... once for that total. I mutated just the new total handoff back to that wording: all 11 tests still passed; appending Execute ... billing subscribe ... yourself also passed. The base reference does fail, but these guards do not establish the claimed no-agent-billing invariant. Require the positive person-handoff wording and explicit prohibition in both SKILL.md and start.md, remove the run alternative, and add mutation fixtures covering each payment section. The total-to-per-step mutation correctly fails today.
Nit — clarify the opening: references/start.md:4–6 still says “They type nothing: you run every command” and implies payment pages have already opened. Qualify that as non-billing commands and person-opened payment links so it agrees with the specific exceptions below. I treat this as nonblocking because those specific billing prohibitions are explicit and repeated.
Verification: node --test scripts/check-core-skills.test.mjs 11/11; base start.md 10 pass/1 fail; per-step mutant 10 pass/1 fail; restored head 11/11. node scripts/check-core-skills.mjs passes, SKILL.md is 5275/5312 bytes, and bun packages/cli/scripts/gen-skills-manifest.ts --check reports all 9 skills in sync. Read the entry and all four reference files plus the unchanged refusal, attribution and generation retry paths. No live payment/generation, agent execution trial, or new Snyk audit was run.
This narrows #7218’s concern by removing the skill’s instruction to initiate subscriptions/top-ups. It does not close the broader no-up-front-cost concern: paid generation still starts under the existing no-volunteered-prices policy, and this is an instruction-level safeguard rather than a CLI enforcement boundary.
Need: yes, move purchases to the person · Size: ok, four focused files · Reuse: existing media-use handoff and CLI attribution/resume contracts · Simpler: already minimal · Structure: fine
Risk: low — purchases move behind an explicit person-run handoff; the paid-generation policy and CLI behavior do not change.
Verdict: APPROVE
Reasoning: The current specific payment instructions hand billing to the person, keep the whole-run top-up and refused-only retry flow, and introduce no verified behavioral blocker. The regression-test blind spots and generic opening wording are small follow-ups.
— Rames (hf-review)
start.md's opening said the agent runs every command, which contradicts the billing sections below it. It now excludes `billing` and says the person opens and pays on the upgrade and add-on credits pages. The test now checks the handoff itself. Both payment sections must give the person the command and forbid running billing. Neither may tell the agent to run, execute or launch a billing command, and SKILL.md must keep its prohibition. Each of those mutations, and main's start.md, fails the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HebQXQjEiSeVmhvTbJvSAw
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 1afa8cd
Re-reviewed the delta from my approval at ed948a8. Both prior nits are resolved:
skills/reverse-any-video/references/start.md:4–6explicitly excludes billing from agent-run commands and puts opening/paying on the upgrade and add-on pages with the person.scripts/check-core-skills.test.mjs:51,58–81removes the agent-run alternative and positively pins the entry prohibition, qualified opening, and prohibition/person-handoff in each payment section. Negative checks now cover run/execute/launch in those sections.
Independent verification: node --test scripts/check-core-skills.test.mjs passes 12/12. Seven mutations each fail: base start.md, restoring the old agent-run combined top-up, Execute-subscribe in Free, launch-add-credits in the exhausted-credit section, removing the entry prohibition, reverting the opening, and replacing the run-total ask with per-step asks. Restoring this head returns 12/12. node scripts/check-core-skills.mjs passes; unchanged SKILL.md is 5275/5312 bytes. bun packages/cli/scripts/gen-skills-manifest.ts --check reports all 9 skills in sync.
Only the test, opening paragraph and manifest changed since my prior review; the payment-section commands, attribution, confirmation wait and refused-only retry instructions are unchanged. These text tests catch the tested regressions; they are not a semantic proof over arbitrary instruction rewordings. No live agent/payment/generation trial or new Snyk audit. The broader no-up-front-cost concern from #7218 remains as described in my previous review.
Need: yes, pin the person-run purchase boundary · Size: ok, three-file follow-up · Reuse: existing core-skill tests and media-use handoff · Simpler: already minimal · Structure: fine
Risk: low — clarifies the existing person-run billing exception and strengthens regression checks without changing CLI behavior.
Verdict: APPROVE
Reasoning: Both prior follow-ups are fixed and the tested unsafe wording mutations now fail. No new blocker in the delta.
— Rames (hf-review)
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Reviewed the current head 1afa8cd102d3f068e2a27c168abdd844450df50e. The new opening and payment sections now clearly prohibit the agent from executing billing commands and hand the upgrade/credit link and command to the person. The successor also tightens the positive and negative tests against the earlier agent-run wording. One billing-consent/whole-run blocker remains in the changed skills/reverse-any-video/references/start.md:68–69: after saying to aggregate all refused steps and hand over a single --needed <total> command, it says “If a refusal prints no amount, give plain billing add-credits for that one.” If two steps refuse without an amount (or one known and one unknown), that instruction still authorizes a separate purchase prompt for each unknown step, and potentially an aggregate purchase prompt plus one per-step prompt. This contradicts the stated requirement and the #7048 intent: one top-up handoff for the whole run, never one per refused step. It was inherited from base, but remains active in the very payment path this PR rewrites.
Please make the unknown-amount case one consolidated human handoff for all refused steps (e.g. a single plain add-credits link/command without a falsely computed --needed total), or stop for clarification when a whole-run total cannot be established. Do not issue one command per unknown step. Add a regression that rejects a per-step fallback even while the aggregate sentence remains; the current check-core-skills.test.mjs:49–78 matches the good aggregate wording but still passes with this exception present. The no-volunteered-prices wording and restart of refused steps otherwise look sound. Hosted support/skill checks executed green; they do not cover this gap. No live payment, purchase command, merge or enqueue by me. — Jerrai
…mount is unknown "If a refusal prints no amount, give plain `billing add-credits` for that one" let the agent hand over a purchase for each step with no amount, on top of the aggregate one. Now, if any refusal prints no amount, the total is unknown and the person gets one plain add-credits for the whole run, never one per step. The test now requires that sentence and rejects a per-step fallback in the out-of-credits section, even when the aggregate sentence is still there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HebQXQjEiSeVmhvTbJvSAw
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed at 73155b92d2106f61372f7d228da19c10d10ad450. The remaining whole-run blocker is fixed: skills/reverse-any-video/references/start.md now says that if any refusal lacks an amount, the total is unknown and the person gets one plain billing add-credits handoff for the whole run, never one per step. With all amounts known, it still waits for every launched step, aggregates --needed <total> once with run attribution, and relaunches only refused steps after the person confirms their balance. The opening and both payment sections explicitly exclude agent-run billing; Free hands over the upgrade link and verbatim attributed subscribe command. No prices or balances are volunteered outside the existing refusal context. The successor regression requires the unknown-total sentence and rejects a per-step fallback even beside the good aggregate wording. Other changed files are test and regenerated manifest; no CLI payment implementation changed.
Hosted skill/manifest checks were green while broader new-head CI was still running at stamp time, an independent merge gate. This is an instruction-level consent fix, not a live agent/payment trial or a new Snyk audit; the broader #7218 upfront-cost policy remains separate. I did not run a billing command, buy anything, merge or enqueue. — Jerrai
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 73155b9
Re-reviewed the three-file delta from my approved 1afa8cd. Jerrai identified an inherited unknown-amount exception that my prior read missed: “for that one” could produce separate purchase handoffs despite the aggregate rule. It is now removed.
skills/reverse-any-video/references/start.md:68–70 makes any missing amount mean an unknown total and gives one plain add-credits command instead, for the whole run. For all-known amounts the aggregate --needed <total> handoff remains; for either all-unknown or mixed known/unknown amounts the plain whole-run alternative applies, without a second per-step prompt. The person still runs billing and confirms completion before only refused steps resume. The reviewed delta changes no CLI source and retains the prior prohibition/opening fixes.
node --test scripts/check-core-skills.test.mjs: 12/12 pass. Independently, each of five mutations fails: previous-head start.md; appending the old “for that one” fallback beside the good aggregate; adding “for each refused step”; removing the any-unknown-total rule; reverting the combined total to agent-run. Restored head passes 12/12. Budget check passes (entry unchanged at 5275/5312 bytes), and the manifest verifier reports 9 skills in sync. The tests protect those concrete text regressions, not arbitrary semantic rewordings. No live agent, payment, generation, or new Snyk audit was run.
Need: yes, one handoff even when an amount is missing · Size: ok, three-file delta · Reuse: existing whole-run handoff and core-skill tests · Simpler: already minimal · Structure: fine
Risk: low — removes the per-step purchase exception while retaining person-run billing and existing retry behavior.
Verdict: APPROVE
Reasoning: The unknown-amount exception raised in Jerrai’s review is fixed, and both prior-text and contradictory-fallback mutations fail the regression. No new blocker found in this revision; the existing instruction not to volunteer prices before paid generation remains unchanged and is a separate concern from who initiates a purchase.
— Rames (hf-review)
…view to heygen-com#7250 Adopts heygen-com#7310 (MPEG-4 Part 2 / MJPEG get browser-playable copies), heygen-com#7316 (check_report counts the copies a check waited for) and heygen-com#7317 (render reads a composition src like the preview and refuses paths outside the project). Defers heygen-com#7309 (needs heygen-com#6912); heygen-com#7314 does not apply. PR axis advances to heygen-com#7250 (heygen-com#7243 closed unmerged, heygen-com#7249 open and pinned as monitored; monitored 96->97). Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
reverse-any-video stops running
billingcommands itself. When a person is on Free, the skill gives them the upgrade link and thereverse.subscribe_commandto run themselves. When a run is out of credits, it gives them onebilling add-credits --needed <total>command for the run's total, plus the refusal's link. The skill then waits until the person says they're done, and either starts again or relaunches every refused step.This is the pattern Higgsfield's skills use, and the one our own
media-usealready follows ("A refused call names abillingcommand: say why, ask them to run it").Why
#7218 reworded the billing lines. The Snyk audit on skills.sh re-ran on Nov 5 and E004 dropped from 1.00 to 0.30, and W012/W009 are gone. The skill still fails, because E004 always counts as CRITICAL. The only reason Snyk gives now is that the skill "instructs the agent to initiate paid billing subscriptions and credit purchases on the user's behalf through CLI commands".
media-useandstyle-samplingpass the same audit with the same no-volunteered-prices rule. Higgsfield's skills have no purchase step at all and get Warn, for W012 and W011 only. What sets reverse apart is that the agent runs the purchase command, and this change removes that.What doesn't change: the same commands and links (with
--skill/--sessionattribution), adding credits once for the run's total rather than per step, keeping the plan whole, and not volunteering prices.Prior test intent
scripts/check-core-skills.test.mjs› "reverse builds the project with init --video and asks for the run's total once" comes from #7048 ("docs(skills): reverse tops up once for the run's total and inits with --video"). That PR gives a reason: a run with nine refused steps "asked for one shot's credits nine times over instead of the sum". The regex still requires one command for the run's total. Its wording is now "give them… add-credits --needed <total> …to run once for that total", so a per-step ask fails, and so does going back to "run".A new test, "reverse hands every purchase to the person instead of running billing itself", requires:
billing)";billing;billingcommand yourself" and gives the person the command;billingcommand or thereverse.subscribe_command.billing add-creditsfor the whole run when any refusal prints no amount, and has no per-step fallback. docs(skills): reverse tops up once for the run's total and inits with --video #7048's old "for that one" let a purchase be handed over for each unknown step. This follows Jerrai's review.This follows Rames' review.
Testing
node --test scripts/check-core-skills.test.mjs: 12 pass. The1afa8cd1start.md ("for that one") fails, and so does a per-step fallback added beside the aggregate sentence.start.md; the total handed back to "run";Execute … billing subscribe … yourselfin the Free section;launch billing add-creditsin the out-of-credits section; the SKILL.md prohibition removed; the opening reverted.node scripts/check-core-skills.mjs: within budget (SKILL.md is 5275 B of 5312 B).gen-skills-manifest --check: in sync.oxfmt --check: clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01HebQXQjEiSeVmhvTbJvSAw