fix(cli): reject extra positional arguments on every command

[miguel-heygen]

Summary

Every CLI command now handles positional arguments beyond the ones it declares, instead of silently dropping them. Before, hyperframes render ./proj out.mp4 ignored out.mp4 and rendered to the default path, and hyperframes lint a b linted only a. Now those exit with a usage error that names the extra argument and the command's usage line:

Unexpected extra argument for hyperframes render: out.mp4
Usage: hyperframes render [DIR] [OPTIONS]

Under --json (read from the parsed flag, so --json=true and --json=1 count, --json=false does not, and a --json after -- is an argument, not the flag) the same message goes to stdout as {"ok":false,"error":...}, matching the add command's existing usage error. add keeps its own hint on a third line: "Run add once per item, or pass a single tag to install every item tagged with it."

The two free-text commands join their words into one value instead: hyperframes catalog lower third searches "lower third" (it searched only "lower"), and hyperframes tts hello world speaks "hello world" (it spoke only "hello"). For tts, when the first word names an existing .txt file (tts script.txt extra), the words are not joined: tts reads the file as before and the extra word is rejected with the usage line. Text words together with --text-file are a usage error ("Pass text to speak or --text-file, not both.") instead of the words being dropped. catalog always joins, even when a word matches a file or folder name.

Also: lambda deploy and lambda destroy now fail clearly when HYPERFRAMES_REPO_ROOT points somewhere that is not a hyperframes checkout. Before, the bad value was silently ignored and the error that followed told the user to set the variable they had already set.

Telemetry never sees the user's arguments: the error the CLI reports carries only the command path and a count (2 unexpected extra arguments for hyperframes render); the detailed message above goes only to the terminal. The repo-root error likewise does not echo the HYPERFRAMES_REPO_ROOT value.

Root cause

citty binds each declared positional in order and leaves the rest in args._. The shared command wrapper only checked flags, so every command that does not read args._ dropped the extras. #3891 fixed this for add alone; this moves the check into the shared wrapper (wrapCommand) and deletes add's own guard.

Commands that read args._ on purpose are on an explicit list keyed by command path: compare, figma asset, skills update, timeline set. catalog and tts are on a second list whose last positional takes every remaining word. A test loads each listed command and fails if a path stops naming a real command. Command groups (auth, cloud, figma, history, skills, timeline) are skipped by structure: citty also runs a group's own handler after its subcommand, with the subcommand name in args._.

Behaviour change

A command run with more positionals than it declares now fails with exit code 1 (usage error) where it used to continue and ignore them. catalog and tts accept multi-word input. An invalid HYPERFRAMES_REPO_ROOT is now an error for lambda deploy and lambda destroy (before, it was ignored and both fell back to finding the checkout themselves).

Test plan

• command-failure-tracking.test.ts: one test per family through citty's real runCommand: plain leaf (text, --json, --json=true, --json=1), a leaf within its declared count, a nested subcommand named by its full path, an opted-out command, the two joining commands, tts with a .txt file plus a stray word, catalog whose first word names a folder, and a group whose own handler citty runs after the subcommand.
• cli.commands.test.ts: every path on both lists resolves to a real command and loader key.
• add.test.ts: add a --dir <dir> b c through the wrapper rejects before any registry call; a single-item add still succeeds.
• events.test.ts: a real extra-positional usage error sent through trackCommandFailure produces a cli_error payload with the count and no argument text.
• The real figma asset and skills update definitions accept several positionals only because they are on the opt-out list; the thrown error is marked as already presented; a required positional prints as <NAME>.
• tts.test.ts: words plus --text-file is rejected before anything is synthesized. add.test.ts checks add's hint. JSON mode: --json=1 on a command with no declared json flag gives JSON; -- --json does not.
• repoRoot.test.ts: a valid HYPERFRAMES_REPO_ROOT is returned; an invalid one throws.
• Each test fails with its piece of the change removed. Full packages/cli suite green apart from a Chromium launch timeout on the test machine; typecheck and the comment ratchet clean.

[github-actions]

Edit accuracy: accurate 1216 (base branch 1216), smooth 996 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (1)

• seqnudge-none-pct-r0-nested-z100 (fixed by fix(studio): playhead keyframe edits keep their ease, undo names its edit and never waits on videos #4889): pass, passes

[somanshreddy]

First pass at 11e99d3e. This is a comment, not an approval. I'd fix one thing before merge (1). Two independent passes went into it, mine and Codex's, and I re-checked every point below at this head by running the CLI from source (bun packages/cli/src/cli.ts …).

What holds up

• The guard covers every command. I loaded all 45 top-level commands and every subcommand under them (83 in all) and compared what each declares against what it reads. Only compare, figma asset, skills update and timeline set read extra positionals on purpose, and the opt-out list matches exactly. The media-use subcommands declare no positionals, but their engine's parseArgs({ strict: true }) already rejects positionals, so nothing changes there. Groups route their first positional to a subcommand (timeline ./x → "Unknown command"), so skipping groups is safe. Codex reached the same inventory independently.
• Witnesses behave as described:
  • lint ./a ./b → exit 1, with the extra argument and the usage line on stderr.
  • --json, --json=1 and --json=true → {"ok":false,…} on stdout. --json=false → stderr.
  • cloud get id1 id2, history peek r p extra and keyframes a b give the right subcommand path and usage line.
  • add a b c is rejected.
  • catalog lower third --json searches "lower third".
  • figma asset 'KEY:1-2' 'KEY:3-4' (the batch form the figma skill documents) and skills update a b still reach their command bodies.
• No documented invocation breaks. I scanned skills/, docs/, registry/, packages/ and scripts/ for hyperframes <cmd> invocations, matching each against that command's positional count. Every hit is prose. The only programmatic call, init → runCommand(previewCmd, { rawArgs: [destDir] }), passes one positional.
• Tests:
  • The 4 changed test files pass 67 of 67.
  • The full packages/cli suite passed 4,102 with 13 skipped. One test failed, browser/manager.test.ts > withInstallLock …; it's a lock-timing test this PR doesn't touch, and it passed 49 of 49 when rerun alone.
  • 18 mutations: 14 were caught. The four that survived are in item 2.

Should fix

1. The usage error sends the user's raw arguments to telemetry (reject-extra-positionals.ts:62-68).

   • The full message, including every extra positional, becomes CliUsageError.message. executeCli passes that to reportCommandFailure (cli.ts:524), and from there it goes into cli_error.error_message (telemetry/events.ts:945-947, :887).
   • redactTelemetryMessage only rewrites paths and URL query strings. I ran the real redactor over the real messages:
     • tts script.txt call Jane Doe at 555-0100 keeps call, Jane, Doe, at, 555-0100 verbatim;
     • a bare word such as a project name (init acme-secret-launch-style), or a pasted token, passes through unchanged;
     • only ./clients/acme-q3-launch.mp4 becomes [path].
   • Before this PR the only other CliUsageError was failUsage()'s fixed "Invalid command usage". That includes the add guard deleted here, so add a b c never put user text in telemetry, and now it does. Unknown flag: --x carries only a flag name.
   • Suggested fix: print the detailed message as now, but throw a fixed one, e.g. new CliUsageError("Unexpected extra positional arguments", { presented: true }), or include a count. Codex flagged this as blocking. I'd call it a cheap fix to make before merge.
   • The same applies, less often, to the new repoRoot error. It interpolates HYPERFRAMES_REPO_ROOT as given, so a relative value without a separator (Acme-Project) isn't redacted. Codex found this part.

2. Four mutations leave every test passing. I ran the 4 changed test files plus figma/asset, compare, skills, tts, catalog and src/timeline/ (265 tests):

   • Removing "figma asset" from ACCEPTS_EXTRA_POSITIONALS. With that removed, the documented batch form figma asset 'KEY:1-2' 'KEY:3-4' fails with "Unexpected extra argument … KEY:3-4". I confirmed this by running it.
   • Removing "skills update". skills update a b is then rejected.
   • Dropping presented: true. The usage error would then print twice: once here, then again from the root boundary's showRequestedUsage.
   • Making usageLine treat every positional as optional. <NAME> would turn into [NAME].

   compare and timeline set are covered (the opt-out test and timeline.e2e). The other two opt-outs only have the "path resolves" test, which proves the path exists but not that it's needed. A wrapped-command test per opt-out, running the real figma asset with two refs and skills update with two names, would cover them.

Lower priority

3. tts with --text-file still discards positionals silently. tts.ts:93 resolves args["text-file"] ?? args.input. So tts hello world --text-file script.txt joins "hello world" into input, then reads the file and drops the words. The .txt exception (:55) checks only the positional, so this path never reaches the rejection. It behaved the same before the PR. But the PR's point is that extras are no longer ignored, and this is the remaining case. Rejecting positional text alongside --text-file would close it (Codex).

4. JSON detection edge cases (:65, Codex, both reproduced):

   • docs introduction extra --json=1 goes to stderr, because docs doesn't declare json and the raw-arg fallback only matches --json and --json=true. The description's "any spelling the parser accepts" holds only for commands that declare json.
   • docs introduction extra -- --json prints JSON on stdout, even though --json after -- is a literal positional.

Nits

• add lost its specific hint ("Run add once per item, or pass a single tag to install every item tagged with it"). add a b c is a common agent mistake, so a per-command hint hook might be worth keeping.
• The top-level path comes from meta.name (trackCommandFailures → commandName), not the commandLoaders key. All 45 match today. But the opt-out lists are keyed by path, and the test checks that listed paths resolve, not that meta.name === key. Passing the key in would remove the drift risk.
• The two hf-join-* temp dirs in command-failure-tracking.test.ts:181,191 are never removed. The afterEach doesn't rmSync them (Codex).

Where I disagreed with Codex

• Its second blocker didn't reproduce. It said check ./proj --frame-check is rejected because citty sends the whole argv to _ when a bare string flag has no value. At this head, citty parses it to {"_":["./p"],"frame-check":true}, and the command gets past the guard to "No composition found".
• Its "missing value gets joined" claim didn't reproduce either. parseArgs(["hello","--voice"]) gives _: ["hello"], voice: "", so tts hello --voice speaks "hello", not "hello --voice". catalog lower --query searched "lower".

CI: 93 checks pass. Only the WIP app check is pending.

— Somu

[somanshreddy]

Re-review of the 11e99d3e → 209ce48d delta. I re-checked each finding from my first pass by number. This is a comment, not an approval, and the full PR wasn't reviewed again. Everything I raised before is fixed, and the delta has no blockers.

Two independent passes went into it, mine and a static Codex pass in a separate checkout. I re-ran the CLI from source (bun packages/cli/src/cli.ts …) and the four mutations that survived last time.

First-pass items

1. Telemetry: fixed. The thrown CliUsageError now reads "<n> unexpected extra argument(s) for hyperframes <path>". The detailed message with the arguments is only printed to the terminal (stderr, or stdout under --json). trackCommandFailure and cli_error read error.message, so the arguments no longer reach them, and I found no other path that sends them. The new events.test.ts case runs the real trackCommandFailure and asserts Jane and 555-0100 are absent. repoRoot no longer interpolates HYPERFRAMES_REPO_ROOT. Reverting either change fails a test.
2. Four unpinned mutations: all now caught. Each one fails at least one test in the 365 that cover the touched commands and utilities:
   • removing "figma asset" or "skills update" from the opt-out list (the new real-command test fails);
   • dropping presented: true;
   • marking every positional optional in usageLine (the new <FILE> [AT] test fails).
3. tts with --text-file: fixed. tts hello world --text-file s.txt now exits 1 with "Pass text to speak or --text-file, not both." tts s.txt still reads the file. tts s.txt extra is still rejected as an extra argument.
4. JSON detection: fixed.
   • docs introduction extra --json=1 prints {"ok":false,…} on stdout.
   • --json=0 and -- --json go to stderr, with --json reported as an extra.
   • lint ./a ./b --json, --json=false and --no-json all behave correctly.

• Nits:
  • add a b c prints the "Run add once per item…" hint again.
  • The hf-join-* temp dirs are cleaned up.
  • One nit is unchanged and still optional: the opt-out lists are keyed by meta.name rather than the commandLoaders key.

New, all minor

• tts hello --text-file s.txt --list lists voices and exits 0 (tts.ts:88). The --list branch returns before the new conflict check. --list already ignored text before this PR (tts hello world --list), so I'd leave it, or move the check above the branch if you want every conflict rejected. Codex rated this should-fix; I rate it a nit.
• The "false"/"0" exclusion in wantsJson has no test. If it's removed, docs … --json=false would print JSON, and all 365 tests still pass. One row asserting stderr for --json=false on a command that doesn't declare json would pin it. Codex flagged the same gap.
• wantsJson is case-sensitive. --json=False counts as JSON. The commands' own if (args.json) checks treat it the same way, so it's consistent.

Tests: the 6 changed or new test files pass 152 of 152. The full packages/cli suite passes 4,110 with 13 skipped and 0 failures. CI at this head: 87 checks pass and 6 edit-accuracy shards are pending, with none failing.

— Somu

[somanshreddy]

Re-review of the 209ce48d → fccbb399 delta: one commit, +28/−4 across tts.ts, tts.test.ts and command-failure-tracking.test.ts. I didn't re-review the full PR. This is a comment, not an approval, and the delta has no findings.

• tts words plus --text-file under --list: fixed. The both-inputs check now runs before list mode, so tts hello --text-file s.txt --list is rejected instead of listing voices. To confirm the new test pins it, I moved the check back below the --list return, and exactly that test fails (1 failed, 2 passed in its block).
• --json=false/--json=0 exclusion: now tested. I removed value !== "false" && value !== "0" from reject-extra-positionals.ts:25, and the new stderr test fails (1 of 29).
• The reorder changes nothing else. --list on its own, and --list with only positional text or only --text-file, still list voices, because the guard only fires when both inputs are set.
• Tests: tts.test.ts and command-failure-tracking.test.ts pass 32 of 32 at this head. CI was 73 passed and 20 pending, with none failing, when I checked.

— Somu

[jrusso1020]

Approving at fccbb399.

What I checked

• The opt-out list is complete. I searched every command under src for reads of args._ and rawArgs. The leaves that read extra positionals are compare, figma asset, skills update and timeline set, all of which are on ACCEPTS_EXTRA_POSITIONALS. timeline apply and timeline undo fall back to positional(args)[1], but that fallback is unreachable because each declares its positional as required. figma, cloud, auth, history, skills and timeline read args._[0], but they are groups, which the !cmd.subCommands guard skips. check re-parses rawArgs only to normalize --frame-check, so its positional count is the same as citty's.
• Paths match. Every top-level meta.name equals its loader key, so ${path} ${name} builds the paths the lists use, and cli.commands.test.ts pins this.
• Telemetry stays clean. trackCommandFailure reports only error.name, error.message and stack. The thrown CliUsageError holds just the count and the command path, and the repo-root error doesn't include the env value.
• Real CLI runs:
  • render ./nope out.mp4, lint a b, lint a b --json, lint a b --json=false and compositions a b each exit 1 with the usage line. The JSON form goes to stdout as {"ok":false,...}, and --json=false goes to stderr.
  • catalog lower third --json searches "lower third".
• Tests: the six touched test files pass, 154/154. Seven deliberate mutations each turn tests red:
  • dropping the group guard;
  • making wantsJson strict (=== true);
  • removing the tts .txt exception;
  • putting the extras into the thrown message;
  • an off-by-one in the extras slice;
  • removing the opt-out early return;
  • removing the tts words-plus---text-file check.
• CI: 94/94 green at this head.

Nit (not blocking, and it predates this PR)

• lambda is a single leaf that declares three generic positionals (subcommand, target, extra). So lambda deploy x y still passes this check and silently ignores x y. I ran it, and it went straight into building the handler ZIP. A per-verb positional count would be a follow-up, not a change for this PR.

— Rames