feat(codegen): honor OpenAPI deprecated on generated flags (PRINFRA-503)

[somanshreddy]

Scope

Surfaces: CLI | Module: Codegen / command builder

Summary

Codegen ignored OpenAPI's deprecated keyword entirely, so a request field the API has moved on from was emitted as an ordinary, fully advertised flag. This threads the signal through — spec deprecated → FlagSpec.Deprecated → the Cobra builder — so a deprecated flag drops out of --help and warns when used, while still working exactly as before.

This is not hypothetical. brand_voice_id has carried deprecated: true in three schemas for months, and heygen template generate --brand-voice-id shows up in --help today with nothing marking it.

Context

The CLI already had deprecation machinery, which is why the gap is easy to miss. cmd/heygen/aliases.go re-registers a renamed command at its old path, hidden and deprecated. internal/command/hidden.go omits an unannounced command from help. Both are command-level and hand-maintained; neither has a flag-level equivalent, and FlagSpec had no Deprecated field at all.

How it works

The rule is deprecated means "stop advertising", never "stop working". Three consequences, each of which the tests pin:

	Behavior
--help	The flag is gone. This is the point: the CLI stops offering a field the API has moved on from.
Passing the flag	Still accepted, value still sent to the API, exit code unchanged. A notice goes to stderr.
--request-schema	Reports deprecated: true on the property.

That last row matters more than it looks. A caller who composes a body with -d/--data never touches the flag, so it never sees the flag's notice — the schema is the surface it reads instead. Dropping the keyword there would hide the signal from exactly the caller that bypasses the flag.

deprecated says only "don't use this", never why. Some deprecated fields are live aliases (brand_voice_id resolves to brand_glossary_id); others are no-ops the API ignores (enable_caption, once captions became unconditional). The spec draws no distinction, so the notice is deliberately generic and the field's own description carries the specifics.

Design decisions

MarkHidden plus a formatter notice, not pflag's MarkDeprecated. MarkDeprecated is the obvious call and it does hide the flag — but it also prints its own "Flag --x has been deprecated" line from inside Set(), written to the flag set's output, which Cobra points at the command's stdout. That puts a line of prose in front of the JSON response and makes stdout unparseable for anything piping it. I had this backwards from reading the source and only caught it because the test decodes stdout as JSON rather than just asserting stderr mentions the flag.

A new Formatter.Warn. The CLI had no channel for a non-fatal message: Data writes the response to stdout, Error writes to stderr and sets an exit code, and AGENTS.md forbids writing to stderr directly. Warn writes a {"warning": {...}} envelope on stderr in JSON mode, mirroring the error envelope so a machine consumer can apply one rule, and a styled line in --human mode. stdout is untouched in both.

Warn only on the flag the user actually set. Not for every deprecated flag the command happens to define.

Testing

Six tests: four on runtime behavior, one on codegen wiring with an undeprecated control field on the same body, one on the schema surface.

Each was mutation-verified — reverting to MarkDeprecated, dropping the Changed() guard, removing the hide, and deleting the schema passthrough each turn exactly one distinct test red, so none is dominated.

Also checked end to end against a local server: stdout stays clean JSON with the value still in the request body, stderr carries the notice, --help no longer lists --brand-voice-id but still lists --brand-glossary-id.

Regenerating gen/ from the current live spec marks exactly the three brand_voice_id flags. Forward-checked against the pending spec change that deprecates the caption flags: those pick it up too, for seven total. The gen/ committed here is from the current live spec, so this PR stands alone.

Known limitation

codegen/schema.go has no allOf branch, so a deprecated schema reachable only through allOf would lose the marker in --request-schema. This is pre-existing and applies to every metadata key the resolver copies, not just deprecated — and allOf appears zero times in the current spec. Left alone rather than building an untested path with no producer.

[somanshreddy]

Endorse (COMMENTED — these are my own account's PRs, so GH-APPROVE routes to @rames D Jusso / a non-author). The MarkHidden design is correct and comprehensively tested, and it answers the stdout hazard directly.

Hardest look — MarkHidden vs MarkDeprecated + stdout/JSON. The chosen design is right and, importantly, safe regardless of the exact Cobra routing. warnDeprecatedFlags emits via formatter.Warn → stderr, gated on cmd.Flags().Changed(name), and TestDeprecatedFlagWarnsOnStderrAndKeepsStdoutParseable proves stdout stays parseable while stderr carries the notice — that tested property is the real guarantee an agent relies on. On the specific rationale (MarkDeprecated → stdout): pflag prints the deprecation line to the flag set's own Output(), which defaults to os.Stderr, and Cobra buffers local-flag output internally — so whether it actually reaches stdout is Cobra/pflag-version dependent. Either way the manual formatter.Warn route is the correct call for reasons beyond the stream: it's formatter-consistent, message-controlled (generic notice + specifics via --request-schema), fires only on use, and errorFormatter.Warn is a no-op so nothing leaks in error mode. So "avoid MarkDeprecated to keep stdout clean" holds as a safe conclusion even if the precise stdout claim is version-specific — the test is what nails it. (RDJ is deep-diving the exact routing; this complements.)

Correctness verified. Deprecated ≠ removed — MarkHidden keeps the flag registered and TestDeprecatedFlagStillSendsItsValue proves the value reaches the API; TestDeprecatedFlagSilentWhenNotPassed proves no notice unless passed; TestDeprecatedFlagHiddenFromHelpButReplacementShown covers help hygiene. Codegen chain is end-to-end: param.Deprecated → FlagSpec.Deprecated (grouper OR-folds body + param) → template Deprecated: true → builder. Nice touch calling warnDeprecatedFlags after the --request-schema short-circuit — introspection shouldn't warn about flags it never sends. Solid.

[james-russo-rames-d-jusso]

HEAD 8a6c002dea · CI all green (test/lint/govulncheck/goreleaser/pr-template) · Peer state COMMENTED × 1 (author self-endorsement, at HEAD; no third-party review yet) · Base main

Author-flagged hardest look — MarkDeprecated → stdout corruption?

Traced end-to-end against pflag master and cobra main (git HEAD). The chosen MarkHidden design is correct. The stated rationale is a shade sharper than the code:

• pflag — MarkDeprecated sets flag.Deprecated = usageMessage + flag.Hidden = true. When the flag is subsequently parsed, Set() emits "Flag --%s has been deprecated, %s\n" via f.Output() — the FlagSet's configured writer. Left alone, f.Output() falls back to os.Stderr (flag.go:302-307).
• cobra — the flag set's output is set to a per-command bytes.Buffer (c.flags.SetOutput(c.flagErrorBuf), command.go:1694). After parse, cobra flushes the buffer via c.Print(c.flagErrorBuf.String()). c.Print writes to c.OutOrStderr() — but OutOrStderr calls getOut(os.Stderr), which reads c.outWriter, not c.errWriter (command.go:397-420).

So the actual routing of MarkDeprecated's notice depends on whether any consumer called SetOut:

Caller	c.outWriter	MarkDeprecated notice lands on
Production main() (no SetOut)	nil	os.Stderr (via cobra's getOut fallback)
This CLI's test harness (testutil_test.go:94 cmd.SetOut(&stdout))	&stdout buffer	stdout buffer — corrupts JSON
Any in-process wrapper that calls SetOut	wrapper's writer	wrapper's writer

Concretely: in a plain heygen video-translate create --enable-caption ... > out.json invocation from a shell, MarkDeprecated's notice would not corrupt out.json — it would land on os.Stderr and the shell redirect wouldn't affect it. The concrete "MarkDeprecated corrupts stdout in production" claim is only true when the CLI is embedded and the embedder has called SetOut (which the test harness models).

The author's own self-review (at 2026-08-11T23:55:49Z) already flags this — "whether it actually reaches stdout is Cobra/pflag-version dependent." Agreed. And the design is still correct for reasons beyond the stream:

1. Consumer-independent stderr routing — Formatter.Warn doesn't care what the embedder did with SetOut.
2. Consistent envelope shape — {"warning": {"message": "..."}} mirrors the existing error envelope (internal/output/json_formatter.go:55-64), so a machine consumer applies one rule to stderr.
3. Human-mode styling — warningStyle yellow prefix in HumanFormatter.Warn, which MarkDeprecated cannot express.
4. Reusable — first non-fatal-notice channel in the codebase, primed for future non-flag warnings.

Sibling-scope check

grep -n "MarkDeprecated\|MarkHidden" cmd/*.go internal/*.go at HEAD: three MarkHidden sites (builder.go:442, root.go:69 for --headers, auth_login.go:202 for --device-code), zero MarkDeprecated. No residual JSON-corruption vector even under the SetOut-embedded consumer scenario. Clean.

Positives

• Test lineup is unusually strong. Five tests pin distinct properties (deprecated_flags_test.go): stdout-parseable-as-JSON (the load-bearing one), value-still-reaches-API, silent-when-not-passed (control against unconditional emission), hidden-from-help-while-replacement-shown, and marker-survives-into---request-schema. The mutation-verification claim in the PR body is credible given the shape.
• --request-schema passthrough (codegen/schema.go:140-142). A -d/--data composer never touches the flag and so never sees Formatter.Warn — routing deprecated: true into the introspection surface closes that hole. This is subtle and easy to overlook.
• warnDeprecatedFlags placement (builder.go:71, after the --request-schema short-circuit). Introspection shouldn't warn about flags it never sends.
• Changed() gate in warnDeprecatedFlags (builder.go:460). Prevents unrelated deprecated flags on the same command from firing when the user passed a different one.
• Grouper OR-fold (grouper.go:400 — flag.Deprecated = flag.Deprecated || s.Deprecated). Handles OpenAPI's twin loci — deprecated can live on the parameter OR on its schema — and picks up EF's Pydantic-authored case (schema-side).
• Codegen wiring is minimal. FlagSpec.Deprecated bool + template {{- if .Deprecated}} + grouper set — three lines each, no ceremony.

Concerns

• 🟡 Rationale wording in PR body + CONTRIBUTING.md:107. As traced above, the "Cobra points [flag set output] at the command's stdout" claim is not true in the default production path — it's true only for embedders that call SetOut (which the test harness does). Suggest sharpening the wording so the next maintainer inherits the accurate picture, e.g.:

  MarkDeprecated's notice text is routed through pflag's f.Output() → cobra's flagErrorBuf → c.Print → c.OutOrStderr(), whose destination is stdout when the caller has invoked SetOut (as any in-process embedder or our own test harness does) and stderr otherwise. formatter.Warn sidesteps this entirely with an unambiguous stderr envelope.

  Same substantive design, honest mechanism. Non-blocking — but this comment is going to be a maintenance reference every time someone touches deprecation.

• 🟡 No query-param deprecation test. grouper.go:391 sets Deprecated: param.Deprecated on query parameters, but only TestGroupEndpoints_BodyFlagsCarryDeprecated (body-property path) exists. Same codepath, so risk is low — but a parametrized case that adds a deprecated: true query param on the test spec would seal it. If a future refactor splits the body/query paths, the missing assertion becomes silent regression cover.

• 🟡 Forward-checked but not committed for the caption case. PR body says "Forward-checked against the pending spec change that deprecates the caption flags: those pick it up too, for seven total." That's the correct forward-check, but nothing in this diff asserts it — once EF#45566 lands + gen/ resyncs, enable-caption on three commands quietly acquires Deprecated: true. Consider adding a follow-up test that uses the shape of a caption-like deprecated bool query/body field in test_spec.yaml, so the future EF resync is covered by existing CI (not by anyone remembering to re-check).

Cross-PR (series: EF#45566 → #285 → #284)

• Order-independence. internal/command/spec.go gains Deprecated bool and the codegen template gains the {{- if .Deprecated}} guard as one atomic change here. Neither exists prior to #285, so a codegen bot resync that lands before #285 would not emit Deprecated: lines and thus can't wedge compilation. Confirmed by reading the template. Safe.
• EF#45566 interaction. Once the caption deprecations land in EF and the next scheduled resync runs, three (or four, depending on whether MCPVideoTranslateCreateRequest.enable_caption propagates) caption flags will silently acquire Deprecated: true in gen/*.go and get hidden. That's the desired outcome — but there's no test in this PR that asserts the future gen/ shape. Worth watching the first post-EF#45566 resync PR for the expected +N Deprecated: true lines.
• #284 interaction. #284's surface()-diff regression check reads gen/ at two refs and diffs the reduced surface. When #285 lands, the reduction on main will lose --brand-voice-id from the visible surface (well, the reduction includes Name not visibility, so this is actually neutral — the flag stays registered). But #284's second pass — "grep newly added help text for deprecation language" — will flag brand_voice_id's and later enable_caption's help text on the release cut that first includes #285's gen/ update. That's the check working as designed; the releaser needs to know a match here is expected. Both PR bodies could cross-reference each other more explicitly so the eventual release-cut reviewer isn't confused.

What I didn't verify

• Did not run the test suite locally (go not available in this environment) — CI's test (ubuntu/macos/windows) all green at HEAD covers this.
• Did not exhaustively trace every FlagSet fallback for older pflag/cobra versions; the routing table above is from master/main at the time of review.

Verdict

🟢 LGTM from my side. Design choice is correct, tests pin what matters, sibling-scope is clean, cross-PR sequencing is safe. Two 🟡 polish items on rationale wording and coverage that can land in a follow-up or a body-only edit; neither blocks merge.

Leaving as --comment — approval routes through the PR-owner side of the team per this bot's standing rule.

— Review by Rames D Jusso

[jrusso1020]

Reviewed at 8a6c002d. Approving. The MarkHidden + formatter route is the right call, and I want to be precise about why, because the PR body's framing and the reviews above it diverge slightly.

On the stdout question. The defensible version of the claim is narrower than "it corrupts stdout". pflag emits its deprecation notice from Set() to the flag set's output, which Cobra buffers and flushes through c.Print → OutOrStderr(). With no consumer having called SetOut, that fallback is os.Stderr, so in the ordinary shell case (heygen ... > out.json) MarkDeprecated would land on stderr too. The corruption is real specifically for in-process embedders that redirect output — including this repo's own test harness.

That does not weaken the decision, it just relocates the argument. The manual route wins on properties that hold regardless of Cobra's version-dependent routing: the message is controlled, it fires only when the flag is actually set (Changed), it is a no-op on the error path, and it goes to a destination the code owns rather than one it inherits. I'd still soften the PR body's wording, because the release notes will outlive the reasoning and "corrupts stdout" is the kind of claim someone re-derives later and finds false.

What makes this safe is the test, not the routing argument. TestDeprecatedFlagWarnsOnStderrAndKeepsStdoutParseable decodes stdout as JSON rather than asserting that stderr contains the flag name. That distinction matters: the weaker assertion passes even if the notice is written to both streams, so it could not fail for the right reason. The comment on the test says exactly this. The surrounding four — value still sent, silent when not passed, hidden from help while the replacement stays visible, still present in --request-schema — cover deprecated-is-not-removed from each side it could break.

JSONFormatter.Warn mirroring the error envelope as {"warning": {...}} on stderr is a good choice: a machine consumer can apply one parse-or-discard rule to both. The Formatter interface gain is in internal/, so it cannot break anything outside this module, and lint plus test are green on all three platforms, which proves every implementer was updated.

Two non-blocking gaps, both about reach rather than correctness:

1. The notice is flag-scoped, so the raw-body path bypasses it. warnDeprecatedFlags keys off cmd.Flags().Changed(...), so a caller who sends the same deprecated field through -d/--data gets no warning at all. That is the population the deprecation is most aimed at — agents composing a raw body are exactly who will not read --help. The value still sends, so nothing breaks; it just means the notice reaches the users least likely to need it and misses the ones most likely to. Worth a follow-up rather than a change here, since it needs a body-key-to-field-spec walk.

2. Required and Deprecated are applied independently in registerFlag. Nothing stops the generated spec carrying both, and the result is a required flag hidden from --help whose "required flag(s) not set" error names something the user cannot discover. Not reachable today, but it is a one-line guard if you want it closed.

Placement of the warnDeprecatedFlags call after the schema short-circuit is right, and the comment explaining it (introspection is not a call) is the sort of thing that stops someone "tidying" it upward later.

— Rames Jusso