Skip to content

feat: add deprecated command alias framework#156

Merged
somanshreddy merged 1 commit into
mainfrom
06-01-cli_add_deprecated_command_aliases
Jun 1, 2026
Merged

feat: add deprecated command alias framework#156
somanshreddy merged 1 commit into
mainfrom
06-01-cli_add_deprecated_command_aliases

Conversation

@somanshreddy
Copy link
Copy Markdown
Collaborator

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-kitbrand kits rename) lands with the codegen resync PR that performs that rename.

Context

The first real case is the brand resync: GET /v3/brand-kits was retagged from Brand Kit to Brand upstream, which moves heygen brand-kit list (shipped in v0.0.9–v0.0.11) to heygen brand kits list. Without a compat path, that silently breaks existing scripts.

Design decisions

Aliases live in hand-written code (cmd/heygen/aliases.go), not gen/. 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 --human columns 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, and RunE all come from the same buildCobraCommand the real command uses, so an alias can never drift from its target.

Cobra's native Aliases field can't express this. It aliases a name among siblings; the brand case changes both the parent and the depth (brand-kit listbrand 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's Deprecated notice, which prints to stderr on use, so stdout stays clean for JSON consumers piping to jq.

Testing

4 unit tests in cmd/heygen/aliases_test.go: deprecation-notice formatting (with/without RemoveBy), hidden+deprecated leaf registration delegating to the canonical RunE, nil-Spec skip, and shared-parent reuse across multiple aliases. Full suite, go vet, and gofmt clean.

🤖 Generated with Claude Code

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>
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

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

APPROVE on merit. Read all four files end-to-end.

Verified body claims against source

  • Framework lives in cmd/heygen/aliases.go as a small typed struct (Alias) + a DeprecatedAliases registry + attachDeprecatedAliases/attachAliases/ensureHiddenGroup. ✓
  • Aliases re-register the canonical Spec via buildCobraCommand(alias.Spec, ctx) — same builder the real command uses, so the alias literally shares the canonical RunE/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 native Deprecated field is what prints to stderr at invoke time, so stdout stays clean for jq consumers. ✓
  • Hidden parent group via ensureHiddenGroup (child.Hidden = true) — the alias parent doesn't appear in --help either. ✓
  • Two newRootCmd* flows in root.go both call attachDeprecatedAliases(root, ctx) AFTER registerGroups — 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 exactlybuildCobraCommand(alias.Spec, ctx) is the same constructor registerGroups calls for canonical commands. Same RunE, 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 Deprecated field prints via OutOrStderr() before RunE executes. 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 namedeprecationNotice() formats use "heygen <NewPath>" instead, optionally appending ; this alias will be removed in <RemoveBy>. The two TestAliasDeprecationNotice cases 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.go test 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.
  • RemoveBy is 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 check gofmt-style on every release-tag if the project ever ships more frequently.
  • Tab completion — cobra's default genCompletion skips Hidden commands. 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 Aliases field would not have worked here (sibling-name-only) and the body says so — saves the next reviewer from asking the same question.
  • TestAttachAliasesSharesParentGroup is 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)

@somanshreddy somanshreddy merged commit a6ec6b9 into main Jun 1, 2026
9 checks passed
@somanshreddy somanshreddy deleted the 06-01-cli_add_deprecated_command_aliases branch June 1, 2026 09:40
@somanshreddy somanshreddy restored the 06-01-cli_add_deprecated_command_aliases branch June 1, 2026 09:42
@somanshreddy somanshreddy deleted the 06-01-cli_add_deprecated_command_aliases branch June 1, 2026 09:44
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.

2 participants