Skip to content

refactor: define template commands with citty - #150

Open
adelrodriguez wants to merge 6 commits into
mainfrom
t3code/replace-yargs-with-citty
Open

refactor: define template commands with citty#150
adelrodriguez wants to merge 6 commits into
mainfrom
t3code/replace-yargs-with-citty

Conversation

@adelrodriguez

Copy link
Copy Markdown
Collaborator

The template command runner depended on yargs and a local type-only wrapper, which kept every command tied to the old builder API. This replaces that command tree with citty definitions while preserving nested help, required positional arguments, Boolean negation, and repeated or comma-separated workspace selections.

Direct yargs dependencies and the obsolete wrapper are removed, and the lockfile now records citty.

Validation: bun run check, bun test, bun run check:monorepo, and CLI help checks.

Implemented by gpt-5.6-sol in T3 Code through the Codex harness.

@vercel

vercel Bot commented Aug 18, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
init Ready Ready Preview Aug 18, 2026 4:16pm
init-docs Ready Ready Preview Aug 18, 2026 4:16pm

Request Review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important

Two behaviors the description lists as preserved did not survive the port: TemplateFault errors now print a raw stack dump instead of the formatted message, and --keepApps is silently ignored by template setup.

Reviewed changes — the full port of the scripts/ command tree from yargs to citty@0.2.2, verified by running the CLI against the checked-out branch and by reading citty's parser in node_modules/citty/dist/index.mjs.

  • Entry point rewrittenscripts/index.ts drops the yargs builder chain and the top-level faultier error handler in favour of await runMain(main).
  • Command definitions portedtemplate, add, rename, and setup move from yargs builder/handler to citty args/meta/run.
  • Positional choices replacedadd validates kind with an explicit TemplateFault throw because citty positionals cannot carry options.
  • Array option hand-rolledgetOptionValues re-scans rawArgs for --keep-apps / --keep-packages, since citty resolves repeated flags last-wins.
  • Dependenciesyargs@18 and @types/yargs removed, citty@0.2.2 added, scripts/utils.ts deleted. The 566-line lockfile diff is hoisting reshuffle from dropping a direct dependency (cliui, wrap-ansi, string-width, eslint-scope, estraverse move to their remaining consumers' versions) and looks benign.

Verified as intact: no-argument and bare template both print usage and exit 1, --help works at all three nesting levels, missing required positionals fail before run, and --no-git / --no-install still resolve to false.

ℹ️ Unknown flags are no longer rejected

.strict() was applied at both the root and template levels and has no citty equivalent — citty parses with strict: false (index.mjs:90), so unrecognized flags land in values and are ignored. Under yargs@18 the same invocation failed with Unknown argument: bogus. The practical cost is that a typo in a destructive command goes unnoticed: bun template setup --no-instal --yes runs bun install instead of skipping it.

There is no drop-in fix, so this is a decision rather than a defect: accept the looser parsing as the cost of the migration, or validate rawArgs against the declared arg names in the commands where a silent miss is expensive.

Technical details
# `.strict()` has no replacement in the citty command tree

## Affected sites
- `scripts/index.ts:15` — root command no longer rejects unknown flags
- `scripts/template/index.ts:8-16``template` command no longer rejects unknown flags

## Required outcome
- Decide whether unknown-flag rejection is still a requirement for `template setup`, whose `run` prunes workspaces, renames the project, and deletes template files. If it is, unrecognized `--flags` must produce a non-zero exit before any of that work starts.

## Open questions for the human
- Is losing strict parsing an acceptable trade for the migration, or should `setup` specifically validate its own `rawArgs`?

ℹ️ Nitpicks

  • add's kind positional lost the yargs choices: ["app", "package"], so bun template add --help prints KIND Workspace type to add (Required) with no hint of the accepted values. citty positionals cannot carry options, so naming them in description is the only place left for them.
  • The root command's epilogue("Add your own project commands to scripts/index.ts.") is gone with no citty equivalent. docs/template-commands.md:50 still covers it, so this is only a loss of in-help discoverability.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread scripts/index.ts Outdated
Comment thread scripts/template/setup.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ Both blocking findings from the previous review are genuinely fixed — verified by running the CLI on this branch. One residual strictness gap and a few nits below.

Reviewed changes — the second commit, which restores the validation behavior the yargs → citty port dropped.

  • Reinstated scripts/utils.ts as a compatibility layerdefineTemplateCommand wraps each leaf command's run so a thrown TemplateFault renders through consola.error(error.flatten()) plus its details field and exits 1, and unknown --flags are rejected before run starts.
  • Restored .strict()-equivalent rejectiongetUnknownOption scans the leaf command's rawArgs against the declared arg names and aliases, normalizing kebab/camel spellings, stripping a leading no-, and stopping at --.
  • Fixed the --keepApps dropgetOptionValues now compares through normalizeOptionName, so --keepApps and --keep-apps are equivalent, and repeated plus =-inline forms accumulate.
  • Added the first scripts/** testscripts/template/__tests__/setup.test.ts pins the flag spellings getOptionValues claims to support.
  • Clarified add's kind help text — the description now names the accepted values, recovering what the yargs choices used to print.

Confirmed on the branch: template rename and template add widget foo print only the flattened message and details with no stack trace and exit 1; template rename --bogus, --no-bogus, and template setup --keepApp x all report Unknown option: --<name> and exit 1 before any filesystem work; --help never reaches the new guard because citty's runMain intercepts it ahead of runCommand; tsc --noEmit -p tsconfig.json, bunx oxlint scripts, and bun test scripts are all clean.

ℹ️ Flag position now changes the outcome, and misplacement is silent

The guard reads context.rawArgs, which citty has already sliced past the subcommand name, so a flag written before the subcommand is dropped by the parser and never reaches the guard. bun scripts --bogus template rename prints rename's own fault with no unknown-option error, and bun scripts template --yes setup reaches setup with rawArgs of []. Under yargs@18 options were parsed globally, so both positions worked; the worst realistic case is bun template --keep-apps app setup --yes, which discards the selection and keeps every workspace.

Technical details
# Pre-subcommand flags escape both citty's parser and the new unknown-option guard

## Affected sites
- `scripts/utils.ts:13``getUnknownOption(context.rawArgs, ...)`; `runCommand` passes `opts.rawArgs.slice(subCommandArgIndex + 1)` (`citty/dist/index.mjs:217`), so anything before the subcommand name is already gone
- `scripts/index.ts:5-13` and `scripts/template/index.ts:7-17` — the two `subCommands`-only levels use citty's plain `defineCommand`, so they perform no flag validation at all

## Reproduction
```
$ bun run scripts/index.ts --bogus template rename
[error] Provide --name when renaming a project.        # no "Unknown option: --bogus"

$ bun run scripts/index.ts template --name foo rename
Unknown command foo                                    # flag value read as a command name
```

## Required outcome
- Decide whether a flag in a position citty ignores should fail loudly. If it should, an unrecognized or misplaced `--flag` anywhere in `argv` must exit non-zero before `setup` prunes workspaces, renames the project, or deletes template files.

## Suggested approach (optional)
- The leaf command is the only place that knows its own arg names, so the check needs the untruncated `argv` (e.g. `process.argv.slice(2)` filtered of the command path) rather than `context.rawArgs`.
- If the ordering constraint is accepted instead, `docs/template-commands.md` is the place to state that options follow the subcommand.

## Open questions for the human
- Is "options must follow the subcommand" an acceptable documented constraint, or should misplacement be an error?

ℹ️ Nitpicks

  • The unknown-option path prints only consola.error(...), where yargs' .fail handler showed help before the message. Calling citty's showUsage(context.cmd) first would restore the hint about which flags are valid.
  • getOptionValues is now covered, but getUnknownOption and the fault wrapper are not, even though a false positive in the guard silently skips the command. They are also the pieces whose correctness depends on citty internals (--help interception happening before run), which is exactly what a test would pin.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread scripts/utils.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important

The new guard closes the misplaced-flag hole, including the dangerous --keep-apps app template setup case. But it decides "is this word a command?" from a hand-maintained set, so it misreports unknown commands and will reject valid flags on any command added later.

Reviewed changes — the third commit, which makes a misplaced option a hard error instead of a silent drop. Verified by running the CLI and the helpers on the branch.

  • Added a pre-parse guard in the entry pointscripts/index.ts runs getOptionBeforeCommand(process.argv.slice(2), templateCommandNames) before runMain(main, { rawArgs }) and exits 1 with Place --<name> after the template subcommand.
  • Restored usage output on an unknown optiondefineTemplateCommand calls showUsage(context.cmd) ahead of consola.error("Unknown option: --x"), recovering what yargs' .fail used to print.
  • Removed the args castTemplateCommandDefinition<T> narrows CommandDef<T>["args"] from Resolvable<T> to T, so Object.entries(definition.args) no longer needs as T and the undefined / function-form shapes are rejected at compile time.
  • Covered the two argument helpersscripts/__tests__/utils.test.ts pins getUnknownOption (kebab/camel/no- spellings) and getOptionBeforeCommand (before vs. after the leaf command, --help exemption).

Confirmed on the branch: --bogus template rename, template --yes setup, and --keep-apps app template setup all exit 1 before any filesystem work; --help still exits 0 at every nesting level and reaches neither guard; bun test scripts passes 6 tests and bun run check reports 0 warnings and 0 errors.

ℹ️ Nitpicks

  • showUsage(context.cmd) passes no parent, and renderUsage builds its header from parentMeta.name + cmdMeta.name (citty/dist/index.mjs:304). So template rename --bogus prints USAGE rename [OPTIONS] while template rename --help prints USAGE template rename [OPTIONS] — citty's own help path gets the parent from resolveSubCommand.
  • getOptionBeforeCommand returns null where its sibling getUnknownOption returns undefined, and it does not break on -- the way getUnknownOption does. getOptionBeforeCommand(["template", "--", "rename"], names) returns "", which passes the caller's if (misplacedOption) only because the empty string happens to be falsy.
  • Both guards inspect only -- tokens, and findSubCommandIndex (citty/dist/index.mjs:269-278) skips any --prefixed token without consuming a value unless _isValueFlag matches — so bun scripts -v template setup drops -v and runs setup. Exposure is small today because no arg declares a short alias and -h is intercepted globally, but the guard's contract reads as "misplaced options are rejected".
  • The guard exempts --version, yet main.meta carries no version, so bun scripts --version prints the root usage plus No version specified and exits 1. Either set meta.version from the root package.json or drop the exemption.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread scripts/index.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important

Deriving the command names from template.subCommands fixes the drift the last review flagged, but the same commit narrows the guard to one argv position — so an option typed before template is silently dropped again and the leaf command still runs.

Reviewed changes — the fourth commit, which replaces the hand-maintained command-name set in the misplaced-option guard. Verified by tracing citty's parser and by driving a stub command tree that imports the real getOptionBeforeCommand.

  • Derived the command names from the command treescripts/template/index.ts exports templateSubCommands and passes the same object to subCommands, so scripts/index.ts builds the guard's set with new Set(Object.keys(templateSubCommands)) and adding a leaf can no longer start rejecting valid flags on it.
  • Scoped the guard to the template commandgetOptionBeforeCommand now takes the parent command name and bails unless rawArgs[0] matches it, so bun scripts nope --flag reaches citty's Unknown command nope instead of being misdiagnosed as a misplaced option.
  • Narrowed the scan to a single argv position — only rawArgs[1] is inspected; it now also catches single-dash tokens and returns null on a bare --.
  • Updated the helper's tests — added a case pinning that unknown and root-level commands fall through to citty, and removed the case asserting that an option before template is rejected.

Confirmed on the branch: template --yes setup, template -x setup, and template --version setup still exit 1 before any filesystem work; nope --flag, template nope --flag, --keep-apps app template setup, and --name foo template setup all end in citty's own Unknown command error; bun test scripts passes 7 tests and bun run check reports 0 warnings and 0 errors.

ℹ️ Nitpicks

  • The new single-dash branch strips every leading dash before the message is formatted, so bun scripts template -x setup prints Place --x after the template subcommand. — a flag spelling the user never typed.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread scripts/utils.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ The silent-drop hole from the last review is genuinely closed — I could not construct an argv where a leaf runs with a typed option missing. One piece of dead code and two message-quality nits below.

Reviewed changes — the fifth commit, which widens the misplaced-option guard from a single argv position to everything ahead of the parent command.

  • Located the parent command the way citty doesgetOptionBeforeCommand now takes the first token that does not start with - as the parent position, mirroring findSubCommandIndex, and reports the first --prefixed token before it (a bare -- is ignored).
  • Moved the help exemption to the whole argv--help / -h anywhere disables the guard, matching citty's runMain, which intercepts help before runCommand ever runs.
  • Preserved the spelling the user typed — the guard returns the raw token and scripts/index.ts prints Place ${misplacedOption} after the template subcommand., so template -x setup no longer reports --x.
  • Re-pinned the reopened position in testsscripts/__tests__/utils.test.ts covers ["--bogus", "template", "setup"] and ["template", "-x", "setup"] alongside the updated --yes expectation.

Verified on the branch with an enumeration harness that feeds the real getOptionBeforeCommand and then runCommand over a stub tree reusing the real defineTemplateCommand and args definitions: across 280 argv shapes, 208 are rejected by the guard, 69 end in citty's own error, 3 are legitimate leaf runs, and 0 reach a leaf with a typed option absent from both args and rawArgs. bun test scripts passes 7 tests, and the working tree stays clean after the CLI runs.

ℹ️ Nitpicks

  • The leading-option branch (scripts/utils.ts:73) returns the token verbatim, so --name=foo template rename prints Place --name=foo after the template subcommand., while the after-parent branch strips at = (scripts/utils.ts:79) and prints --name. Worth picking one.
  • An option whose value is a separate token still escapes the guard, because the value is the first non-- token: --keep-apps app template setup bails at scripts/utils.ts:70 and surfaces as citty's Unknown command app. Exit is still 1 before any run, and mirroring citty's _isValueFlag is not possible while the root command declares no args, so this is message quality rather than a hole.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread scripts/utils.ts Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No new issues found. Both nits from the last review are fixed, and the removed branch really was unreachable.

Reviewed changes — the sixth commit, which trims dead code from the misplaced-option guard and makes the token it reports spell consistently.

  • Deleted the unreachable --version / -v exemptionscripts/utils.ts drops (argument === "--version" || argument === "-v") && rawArgs.length === 1. Reaching that line required rawArgs[parentCommandIndex + 1] to be truthy, so rawArgs.length was always at least 2 and the condition could never hold.
  • Stripped =value from the leading-option token — the pre-parent branch now returns leadingOption.split("=", 1)[0], matching the after-parent branch, so --name=foo template rename and template --name=foo rename both report --name.
  • Pinned the new spellingscripts/__tests__/utils.test.ts adds an exact toBe("--bogus") assertion for ["--bogus=value", "template", "setup"], which fails against the previous verbatim return.

Confirmed on the branch: --bogus=value template rename and --name=foo template rename now print Place --bogus … / Place --name … and exit 1 before any filesystem work; template --version, template --version rename, bun scripts --version, and template --help all behave exactly as they did at bf7a2e8c, so the branch removal is a verified no-op. bun test scripts passes 7 tests, tsc --noEmit -p tsconfig.json and oxlint scripts are clean, and the working tree stays clean. Every invocation documented under docs/ places its options after the subcommand, so the guard rejects nothing the docs advertise.

Pullfrog  | View workflow run | Using Claude Opus𝕏

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