fix(cli): stop bare --frame-check from swallowing the next flag - #2966
Conversation
citty parses string options greedily, so a bare --frame-check consumed the following flag as its value (--caption-zone silently dropped, --json disabled) and failed with an error blaming --caption-zone. Normalize rawArgs at the check command boundary (bare --frame-check followed by a flag or at end becomes --frame-check=), attribute frame-check spec errors to --frame-check, and detect dash-prefixed values with corrective guidance. Fixes heygen-com#2965
miguel-heygen
left a comment
There was a problem hiding this comment.
Reproduced #2965 on main (c6925e4) before reviewing: check DIR --frame-check --caption-zone "x0=0;y0=.82;x1=1;y1=1" fails with Invalid --caption-zone on a valid zone, and --snapshots --samples 15 --frame-check --json silently drops --json. Both are fixed on this branch, verified end to end through the real CLI entry point, not just the test harness. Reverting only check.ts to the base commit under this PR's test file fails exactly the 3 new tests, so they are real regression tests. check.test.ts 49/49; full CLI suite 2326 passed with 2 pre-existing transcribe failures unrelated to this change; oxlint and oxfmt clean.
Worth noting that the tests drive runCommand directly, which skips citty's subcommand slicing, so the rawArgs read is not covered there. I checked it by hand through the real entry point and the dir positional still resolves correctly.
Three non-blocking follow-ups:
- Sibling flags still swallow, silently.
check DIR --samples --jsonon this branch still eats--json, andpositiveIntegerthen falls back to 9 with no error. Same root cause. The issue text mentioned a "value looks like a flag" guard for--caption-zoneand--layout, which did not land here. swallowedOptionErroris close to unreachable now. Normalization rewrites any dash-prefixed next token beforeparseFrameChecksees it, so the only live path is--frame-check=--json, where the message advisesuse --frame-check=, which the user already did.- The double parse in
run({ rawArgs })has no comment explaining why citty's own parsed args is bypassed. It is also a latent trap: a future citty-level validation such asrequired: truewould run against the un-normalized argv while the body uses the normalized one.
What
Fixes bare
--frame-checkswallowing the next CLI flag inhyperframes check, and makes frame-check spec parse errors reportInvalid --frame-checkinstead of blaming--caption-zone.Fixes #2965. Supersedes #2312 (closed unmerged in a backlog sweep; its close note invited a reopen once a reproducible case resurfaced — integrators assembling argv arrays now hit this in practice).
Why
--frame-checkis declared as a cittytype: "string"option but documented as usable bare. citty 0.2.2 greedily consumes the next argv token as the value even when it starts with-, so:check --frame-check --caption-zone "x0=…"→frame-checkbecomes"--caption-zone", the caption zone is silently dropped, and the run aborts withInvalid --caption-zone— pointing at a flag the user wrote correctly (or never wrote at all).check --snapshots --samples 15 --frame-check --json→--jsonis swallowed and JSON output silently disabled before the parse error aborts the run.The misattribution comes from
parseFrameCheckFieldsreusingparseCaptionField, which throwscaptionZoneError()on any field without=. Introduced in 659cb6a when the flag changed from boolean to string; existing tests only exercised orderings with--frame-checklast, so it went undetected.How
rawArgswith citty'sparseArgsafter rewriting a bare--frame-checkthat is the last token or followed by a--prefixed token into--frame-check=.--frame-check <spec>and--frame-check=<spec>are untouched, and the normalization is scoped to--frame-checkonly (the one string flag documented as bare-usable;--caption-zoneand--layoutreject bare use by design).parseCaptionField/captionSeverity/captionSeekstake an error-factory parameter (defaulting tocaptionZoneErrorfor caption-zone callers), so frame-check spec failures now throwInvalid --frame-check: ….-throws a targeted error explaining the flag appears to have swallowed the next option, suggesting--frame-check=or moving the flag to the end.Test plan
New argv-level regression tests through
runCommand: bare--frame-checkfollowed by--caption-zone …(both parsed),--snapshots --samples 15 --frame-check --json(json honored, frame-check defaults), plus the existing safe-order test (bare--frame-checklast) still passing.New unit test asserting a
--prefixed frame-check value throws the frame-check-attributed error.check.test.ts: 49/49 passing; full CLI suite: 2327 passed (the one failure,src/telemetry/agent_runtime.test.ts, fails identically on unmodifiedmainin this environment — it reads liveprocess.envand is unrelated).oxlint, oxfmt
--check, andtsc --noEmitclean on the changed files.Unit tests added/updated
Manual testing performed
Documentation updated (not applicable — behavior now matches the existing
--helptext)