feat: add deprecated command alias framework#156
Conversation
Command names are derived from the upstream OpenAPI spec, which is owned by experiment-framework. An upstream tag or path change can rename a command that already shipped in a stable release (the brand-kit -> brand kits rename is the first such case). Cobra's native Aliases only span sibling names, not a change of parent or depth, so backward compatibility needs a small registration shim. Add an Alias type and a DeprecatedAliases registry in cmd/heygen. Each alias re-registers the canonical command Spec at its old path, hidden from help and marked deprecated, so the old invocation resolves to the same handler while a stderr notice points to the new name. Aliases live in hand-written code, not gen/, because they encode naming history the current spec has no knowledge of. No aliases are registered yet; the brand-kit entry lands with the codegen resync that performs the rename. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVE on merit. Read all four files end-to-end.
Verified body claims against source
- Framework lives in
cmd/heygen/aliases.goas a small typed struct (Alias) + aDeprecatedAliasesregistry +attachDeprecatedAliases/attachAliases/ensureHiddenGroup. ✓ - Aliases re-register the canonical
SpecviabuildCobraCommand(alias.Spec, ctx)— same builder the real command uses, so the alias literally shares the canonicalRunE/flags/args. The "an alias can never drift from its target" claim is enforced by construction. ✓ leaf.Hidden = true+leaf.Deprecated = alias.deprecationNotice()— cobra's nativeDeprecatedfield is what prints to stderr at invoke time, so stdout stays clean forjqconsumers. ✓- Hidden parent group via
ensureHiddenGroup(child.Hidden = true) — the alias parent doesn't appear in--helpeither. ✓ - Two
newRootCmd*flows inroot.goboth callattachDeprecatedAliases(root, ctx)AFTERregisterGroups— the docstring's "Call it after the generated groups are registered so the canonical commands the aliases delegate to already exist" is enforced. ✓
Audit per the brief's review angles
1. End-to-end alias mechanism — (a), (b), (c). All three properties hold by construction:
- (a) Execute new command's behavior exactly —
buildCobraCommand(alias.Spec, ctx)is the same constructorregisterGroupscalls for canonical commands. SameRunE, same flag binding, same arg validation. The alias is byte-equivalent at the handler layer. - (b) Deprecation notice on stderr, data on stdout — cobra's
Deprecatedfield prints viaOutOrStderr()beforeRunEexecutes. Verified by reading cobra's source; flagging below that this is load-bearing for jq compatibility but isn't pinned by an automated test. - (c) Points users at the new name —
deprecationNotice()formatsuse "heygen <NewPath>" instead, optionally appending; this alias will be removed in <RemoveBy>. The twoTestAliasDeprecationNoticecases pin both shapes.
2. Help-hiding. Confirmed via TestAttachAliasesRegistersHiddenDeprecatedLeaf — asserts both parent.Hidden and leaf.Hidden. ✓
3. Multiple deprecation levels — single-level by design. The framework supports one mode: hidden + deprecated notice on stderr + optional RemoveBy informational field. No graduation from "soft warning" → "hard error before removal." This is fine for the immediate need, but worth naming as a design choice the body should acknowledge — if you ever want a final-warning mode (still works but errors on stderr after a grace date), the cleanest extension is a Severity / Mode field on Alias. Adding it later is non-breaking; current shape is pragmatic for the immediate brand-kit case.
4. Naming conflicts — ensureHiddenGroup doesn't detect collisions with canonical groups. This is the one place I'd add a defensive check:
func ensureHiddenGroup(parent *cobra.Command, token string) *cobra.Command {
for _, child := range parent.Commands() {
if child.Name() == token {
return child // ← returns existing without checking if it's a canonical (visible) group
}
}
...
}Concrete shape of the risk: imagine a future spec rename creates a canonical group whose name coincides with a pre-existing alias OldParentPath token. The alias parent reuses the canonical group (it's the first child to match by name), and the alias leaf becomes a hidden child of a visible group. Cobra resolves the alias path correctly, but the alias is now living inside a non-hidden parent — which could trip --help output if the leaf's Hidden flag is ever unset or shadowed.
Mitigation: a startup-time panic (or returned error) if ensureHiddenGroup finds an existing non-hidden child with the same name. Cheap insurance because this is exactly the kind of bug that's caught at boot rather than at the first user invocation. Non-blocking; today's test set doesn't hit this case.
5. Test coverage gaps. Three properties the framework promises that aren't directly pinned:
- Stderr-vs-stdout routing of the deprecation notice — the load-bearing jq-compatibility property. The
aliases_brandkit_test.gotest in #153 even acknowledges this: "runCommand harness sets both writers, so notice routing is not asserted here." Worth a follow-up unit test that asserts the notice lands on stderr specifically — would catch any future cobra-output-wrapper regression. Could land in #156 or #153. RemoveByis purely informational — no machinery checks "is this past its RemoveBy date / version?" so RemoveBy could be set to a long-past version and the alias would still resolve cleanly. Acceptable for a CLI with infrequent releases; would be worth a CI checkgofmt-style on every release-tag if the project ever ships more frequently.- Tab completion — cobra's default
genCompletionskipsHiddencommands. Verified: aliases are correctly absent from completion. ✓ Worth confirming this is the intended behavior in your contributing-docs section (the new docs block in CONTRIBUTING.md could mention it).
What works well
- The "aliases live in hand-written code, not
gen/" design decision is the right one and the docstring explains the reasoning cleanly. Future codegen runs won't blow away the aliases. - The cobra
Aliasesfield would not have worked here (sibling-name-only) and the body says so — saves the next reviewer from asking the same question. TestAttachAliasesSharesParentGroupis the right shape — pins the invariant that multiple aliases under the same old parent path don't create duplicate parent groups.- CONTRIBUTING.md docs block with the example is exactly the kind of pattern doc that prevents the next deprecation from being implemented differently.
— Approving on merit. James gates the final stamp on heygen-cli per standing instruction.
— Rames Jusso (pr-review)
Scope
Surfaces: CLI (command tree) | Module: codegen runtime / command builder
Summary
Adds a small framework for backward-compatible command aliases. Command names in this CLI are derived from the upstream OpenAPI spec (owned by experiment-framework), so an upstream tag or path change can rename a command that already shipped in a stable release. This gives us a first-class way to keep the old invocation working while steering callers to the new name. No aliases are registered yet; this is the mechanism only. The first consumer (the
brand-kit→brand kitsrename) lands with the codegen resync PR that performs that rename.Context
The first real case is the brand resync:
GET /v3/brand-kitswas retagged fromBrand KittoBrandupstream, which movesheygen brand-kit list(shipped in v0.0.9–v0.0.11) toheygen brand kits list. Without a compat path, that silently breaks existing scripts.Design decisions
Aliases live in hand-written code (
cmd/heygen/aliases.go), notgen/. An alias encodes naming history (the old name), which the current spec has no knowledge of, so codegen can't emit it. This matches how curated--humancolumns and poll configs already live as hand-maintained lookup data that survives regeneration.An alias re-registers the canonical
Spec, it does not reimplement the command. The leaf verb, flags, args, andRunEall come from the samebuildCobraCommandthe real command uses, so an alias can never drift from its target.Cobra's native
Aliasesfield can't express this. It aliases a name among siblings; the brand case changes both the parent and the depth (brand-kit list→brand kits list), so a registration shim is required regardless.Hidden + deprecated. Alias commands are hidden from
--help(keeping the agent-facing tree clean and unambiguous) and carry Cobra'sDeprecatednotice, which prints to stderr on use, so stdout stays clean for JSON consumers piping tojq.Testing
4 unit tests in
cmd/heygen/aliases_test.go: deprecation-notice formatting (with/withoutRemoveBy), hidden+deprecated leaf registration delegating to the canonicalRunE, nil-Spec skip, and shared-parent reuse across multiple aliases. Full suite,go vet, andgofmtclean.🤖 Generated with Claude Code