Skip to content

@remotion/cli: Respect --flag false for 7 boolean options - #12629

Open
DeryFerd wants to merge 1 commit into
remotion-dev:mainfrom
DeryFerd:fix/cli-boolean-flags
Open

DeryFerd wants to merge 1 commit into
remotion-dev:mainfrom
DeryFerd:fix/cli-boolean-flags

Conversation

@DeryFerd

Copy link
Copy Markdown
Contributor

Summary

packages/cli/src/parsed-cli.ts hands minimist a list of boolean flags (boolean: BooleanFlags), so that --overwrite, --sequence and friends parse into real booleans. Seven boolean-typed renderer options never made it into that list: --throw-if-site-exists, --cross-site-isolation, --prefer-lossless, --enable-multiprocess-on-linux, --enable-folder-expiry, --enable-lambda-insights and --disable-ask-ai. Because those flags are unknown to minimist's boolean parser, passing them with an explicit value parses false into the five-character string "false", and a truthy string in JavaScript is not a request to turn something off.

That string then flows into each option's getValue(). throwIfSiteExistsOption and the prefer-lossless option use a truthy check on the CLI value, so --throw-if-site-exists false and --prefer-lossless false return {source: 'cli', value: true}: the user asks to switch the option off and the option comes back on. The other five (cross-site-isolation.tsx, enable-multiprocess-on-linux.tsx, folder-expiry.tsx, enable-lambda-insights.tsx and ask-ai.tsx) distinguish "flag provided" from "flag absent" with !== undefined, so the literal string "false" is passed down where a boolean is expected. The worst case is --enable-folder-expiry false: the request to switch the S3 lifecycle policy off arrives as a truthy string, and the deploy keeps the auto-delete behavior the user just asked to disable.

This PR adds the seven flags to BooleanFlags. That alone is not enough for five of them: minimist parses an absent boolean-listed flag as false rather than undefined, so a getValue() written as !== undefined would read "flag not provided" as "flag provided, false" and the config file could no longer win. The repo already solved this for disableWebSecurityOption, and this PR copies that exact shape: the five flags get a null entry in the minimist default map, and their getValue() guards with !== undefined && !== null and wraps the CLI value in Boolean(). throw-if-site-exists and prefer-lossless use truthy checks, so they need no default entry, exactly like enforce-audio and skip-skills.

After this change, --flag false parses as a boolean false for all seven. For the five guarded options, an explicit --flag false now genuinely disables the option and wins over the config file, which it never managed before. For the two truthy-check options, --flag false falls through to the config value, the same precedence every other flag in the list already has. A bare --flag still parses as true, and leaving the flag out still reads the config file and then the default, unchanged.

Validation

Following the repository rule about not adding new tests, I verified this with a throwaway script that imports the real BooleanFlags array and runs the repo's own minimist install with the same configuration parsed-cli.ts uses, including the default map. The script was removed before committing, so the PR is source-only. Happy to add a permanent regression test if you would like one.

  • Red and green. Before the change, the script reported --prefer-lossless false and --enable-folder-expiry false parsing to the string "false" with truthiness true, and --cross-site-isolation false to the literal string. After the change, all seven parse --flag false into boolean false, a bare --flag into true, and an absent flag into null for the five with a default entry. Reverting the BooleanFlags edit brings the string behavior back.
  • Why the diff touches packages/renderer at all: with the flags in the boolean list, an absent flag parses as false, which would make the five !== undefined checks treat "not provided" as "provided, false" and freeze the config file out of those options. The null default plus the !== undefined && !== null guard is the pattern disable-web-security.tsx already uses; I copied it rather than inventing a new one.
  • bun test src/test/boolean-option-precedence.test.ts in packages/renderer passes (1 test, 28 expectations) with the new guards in place, so the config-over-CLI precedence the suite pins down is preserved.
  • bun test src in packages/renderer: 377 passing, 3 skipped, 5 failing, and the five failures are identical before and after the change, which I checked by stashing the diff and re-running. They are "Need to pass valid metadata" twice, "Returns buffer in promise result", "Catches invalid image format" and "Fast Start is selected from the output container", all of which need the built compositor binary that this checkout does not have.
  • bun test src/test in packages/cli: 194 passing, 3 failing, same three failures before and after ("all source Remotion skills match the current Remotion version", "Get GitHub repo", "forwards the correct arguments to the skills CLI"). They need network access and a GitHub API round trip.
  • bunx tsc -d --noEmit reports 0 errors in both packages/cli and packages/renderer.
  • bunx eslint src reports 0 errors in packages/renderer. In packages/cli eslint cannot run in this checkout at all (eslint-plugin-react-hooks is missing and even an untouched file fails to load the config), so that check is left to CI.

Tradeoffs

The two groups of flags behave slightly differently on an explicit --flag false, and that is inherited from the repo's own conventions rather than decided here: for the five guarded options, an explicit false on the command line wins over the config file, while for the two truthy-check options an explicit false falls through to the config value, because their getValue() can only return the CLI value when it is truthy. Unifying the two groups would mean rewriting the two truthy getValue() implementations, which is a behavior change well beyond this parsing fix. Anyone who wants prefer-lossless off via the config file can set Config.setPreferLosslessAudio(false) in remotion.config.ts, and that works the same before and after this PR.

@vercel

vercel Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
bugs Ready Ready Preview Oct 26, 2026 8:36pm UTC
remotion Ready Ready Preview Oct 26, 2026 8:36pm UTC

Request Review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor inconsistency in the disable-ask-ai handling.

Reviewed changes

  • Boolean flag list: seven renderer flags (throw-if-site-exists, cross-site-isolation, prefer-lossless, enable-multiprocess-on-linux, enable-folder-expiry, enable-lambda-insights, disable-ask-ai) are added to BooleanFlags, so minimist now parses an explicit --flag false into boolean false instead of the string "false".
  • null defaults: the five flags whose getValue() distinguishes "provided" from "absent" get a null entry in the minimist default map, so an absent flag no longer reads as false and the config file can still win.
  • getValue() guards: those five switch to !== undefined && !== null (four also wrap the value in Boolean()) — the same shape disableWebSecurityOption already uses.

I verified the parsing against the repo's own minimist: all seven now parse --flag false → false, a bare --flag → true, and absent → null for the five with a default entry. I also checked that the shared CliInternals.BooleanFlags is spread into the Lambda and CloudRun minimist configs (which lack the null defaults), but every getValue() call site passes CliInternals.parsedCli, so those commands are unaffected.

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash | 𝕏

@pullfrog

pullfrog Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Inline note for the disable-ask-ai point in the review (the review tool only allows one submission per session):

packages/renderer/src/options/ask-ai.tsx (L22-L23)

--disable-ask-ai is the one option where --flag false is not respected. The guard now accepts false, but the body still hardcodes askAIEnabled = false, so --disable-ask-ai false disables Ask AI rather than enabling it. The other four guarded options derive their value from the flag, and .agents/skills/add-cli-option/SKILL.md describes negating flags as inverting the value in getValue(), so deriving it here would keep the bare flag working and let false mean "enable":

if (commandLine[cliFlag] !== undefined && commandLine[cliFlag] !== null) {
	askAIEnabled = !Boolean(commandLine[cliFlag]);

If presence-only is intentional for this flag, it may be worth noting in the PR description, since the current wording reads as if false is honored here too.

Pullfrog  | View workflow run | via Pullfrog | Using deepseek-v4.1-flash | 𝕏

This branch was successfully deployed

2 active deployments
Preview – remotion — 80ac99f1 Deployed Oct 26, 2026 by vercel[bot]
Preview – bugs — 80ac99f1 Deployed Oct 26, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant