docs(release): add a command-surface regression check to the release gate (PRINFRA-499)

[somanshreddy]

Scope

Surfaces: CLI | Module: Release process

Description

gen/ is generated from an OpenAPI spec owned by another repo, and a bot opens a codegen resync PR on every production deploy. So the CLI's user-facing contract can change with no human-authored commit here describing it: the resync lands as a single codegen: resync gen/ from EF <sha> line, and /changelog-cli — which reads git log — collapses it into "Internal".

That means the two artifacts a releaser actually reads before cutting a tag, the commit log and the changelog, are exactly the two places a breaking change is invisible. The pre-release checklist had no step that would catch one.

How it works

The mental model is that the generated surface, not the commit history, is the contract. Every field in gen/ that decides what a user can type is a plain literal on its own line: a command's Group, Name, Endpoint, Method, its Args, and each flag's Name, Type, Default, Required, Enum, Min, Max, Source, and JSONName. Reduce gen/ to just those lines at two refs, diff the two reductions, and any contract change has to show up.

Reading the output is then a two-step rule. A line on the old side is a candidate break, because something a user could previously type is gone or narrower. A line only on the new side is usually safe — with two exceptions the doc calls out explicitly, since both look like pure additions: a newly added flag that arrives already Required: true, and a new entry under Args. Either one makes every prior invocation of that command fail with exit 2.

Two design points are load-bearing:

The filter is an allowlist, not a denylist. A field missing from a denylist is invisible forever. The first draft of this check proved that the hard way: it filtered out Source, BodyEncoding, and JSONName, each of which reroutes the request — Source decides query vs. body vs. multipart, BodyEncoding decides whether -d/--data is registered at all, and JSONName is the actual key sent to the API. That is precisely the failure mode the check exists to prevent. The allowlist now covers every field gen/ emits except the four that are pure prose, and the doc says how to re-derive the inventory if codegen ever adds one.

Schemas are collapsed, not dropped. RequestSchema and ResponseSchema contents are enormous and churn on nearly every resync, but their presence is what decides whether --request-schema and --response-schema exist. So each is reduced to a <present> marker before the comparison: presence is compared, content is not.

A shape diff still cannot see a flag that keeps its signature and stops doing anything, so a second pass greps newly added help text for deprecation language. That case is live right now — --enable-caption on video-translate create, video-translate proofreads generate, and lipsync create became a no-op upstream, and nothing about its shape changed.

This also corrects the bump rule, which listed codegen resyncs as patch-worthy without qualification and would therefore have mislabeled a breaking resync as a patch.

Testing

Docs-only, so the check was verified by running it rather than by unit tests.

• Extracted the surface() function verbatim from the committed file and ran it against a worktree carrying six simultaneous breaks: a newly added already-required flag, Source query→body, a deleted RequestSchema, BodyEncoding json→multipart, a dropped Destructive, and a JSONName rename. All six surfaced.
• Both fenced bash blocks were extracted programmatically from the committed file and executed verbatim; exit 0.
• On v0.6.0..main the comparison is empty, confirming a quiet result is reachable and not a broken pipeline.
• On v0.5.0..v0.6.0 it reports 78 lines; an independent parse of every Enum at both tags confirms that range removed no values and added four, i.e. correctly additive.
• A script check confirms no field emitted into gen/ falls outside allowlist ∪ prose ∪ container lines.

[somanshreddy]

Endorse (COMMENTED — my own account). Genuinely rigorous release-gate design — it targets the exact blind spot the stack is about: a spec resync lands as one codegen: resync commit filed under Internal, so the two things a releaser reads (git log + changelog) are precisely where a breaking surface change is invisible. Diffing the generated surface instead is the right instrument. Strong specifics: the interpretation table maps every <-removal to its user-facing effect + action; the deprecated_flags() awk scan catches the "flag went quiet without a shape change" case the surface diff can't see (literally this thread's failure mode); and Deprecated: true is correctly a release-note item, not a break.

One net-new — the single point of failure. The surface() grep is an allowlist, and you already flag that "a field left out is invisible forever" + give the maintenance grep to refresh it. That manual step is the weak link — same shape as any silent filter: if codegen emits a new surface-deciding field and nobody re-runs the refresh, a regression in it is invisible and the gate still looks green. Worth a tiny CI check that fails when gen/*.go emits a field name absent from the allowlist (comm the emitted set vs the allowlist) — it turns the "add it here" reminder into an enforced gate so the surface-check can't silently rot. Non-blocking (a manual step is acceptable for a docs procedure), but automating the allowlist's own completeness is the durable fix.

[james-russo-rames-d-jusso]

HEAD: 7ea65b3 · CI: all green (3-OS test matrix + lint + govulncheck + goreleaser-check) · Peer state: author somanshreddy left a self-COMMENTED note flagging the allowlist-drift gap and suggesting a CI check that comms emitted gen/*.go field names against the allowlist. This review layers on top rather than re-hunting that finding.

---

Series-coordination notes (EF#45566 · heygen-cli#284 · heygen-cli#285)

• Forward-compat with #285. #285 adds a Deprecated field to FlagSpec and wires codegen to emit it. #284's allowlist already includes Deprecated: — so once #285 lands and the next resync runs, a newly-deprecated flag surfaces as either a Deprecated: true addition (surface diff) or a Help-text hit (deprecated_flags() awk). No follow-up allowlist tweak needed after #285. Nice foresight.
• Mechanism, not handwritten wrapper. #285's MarkHidden is spec-driven (FlagSpec.Deprecated == true → builder.go calls MarkHidden at command-registration time), not handwritten. So the "handwritten MarkHidden gets clobbered by resync" hazard doesn't apply here — the hidden state is a pure function of a field this gate covers. That said, the codebase still has two handwritten MarkHidden calls today (cmd/heygen/auth_login.go:202, cmd/heygen/root.go:69) that live outside gen/ — same class of "reviewed via PR" carve-out, worth being aware of.
• Deploy ordering. #284 is docs-only and safe to land any time; it only starts being useful at the next release cut. If it lands before #285, the first post-#285 release cut will show new Deprecated: true lines on the deprecated flags, which the doc correctly interprets as release-note-worthy but not breaking. If it lands after #285, no harm — the check picks up the same signal. No ordering constraint.
• The check's blast radius is gen/ only — internal/command/hidden.go (the hiddenEndpoints map, which is what makes asset search invisible) is handwritten and OUT of scope. The PR body scopes this to "hand-written commands in cmd/heygen/" but hidden.go lives under internal/command/, so a strict reader could miss that it's also out. Small doc tweak: name the file explicitly.

---

🟠 Concerns

• SendDefaultWhenOmitted is a concrete example of Somansh's allowlist-drift gap that isn't hypothetical. codegen/templates/command.go.tmpl:57-59 already emits SendDefaultWhenOmitted: true conditionally, gated on the OpenAPI x-cli-default extension. No gen/*.go file currently emits it (grep returns nothing), so today's inventory-check script wouldn't flag it — but internal/command/spec.go:98-116 documents it as changing what actually gets sent on the wire ("materialize the flag's Default into the request even when the user didn't pass the flag"). If EF adds x-cli-default to any schema property, the next resync starts emitting SendDefaultWhenOmitted: true and the surface diff silently misses it — an explicit CLI-side default flip is exactly the kind of "same input, different request" change the gate is meant to catch. Two options: (a) add SendDefaultWhenOmitted: to the allowlist prophylactically now, since the mechanism to emit it is already committed; (b) build Somansh's proposed CI-side comm-based completeness check first and let it fire on the next resync. Either works; (a) is a one-line diff, (b) is the durable fix.

• Schema-presence sed relies on codegen never emitting RequestSchema: nil (or equivalent). s/^([[:space:]]*(RequestSchema|ResponseSchema)):.*/\1: <present>/ collapses ANY value to <present>. Today codegen guards emission with {{- if .RequestSchema}} (template line 14, 17), so absent = no line. Perfect. But if a future template change moves to always-emitting the field with nil for no-schema (e.g. to simplify struct init or to make gofmt happier), then RequestSchema: nil and RequestSchema: "{...}" both collapse to <present> and a real schema disappearing produces no diff. The gate then silently blesses the removal of --request-schema support on a command. Worth one sentence in the RELEASE.md prose: "this assumes codegen omits the field when absent; if the emit format changes to always-emit-with-nil, adjust the sed to preserve the nil vs { distinction." Cheap tripwire.

• Args inline-struct field-ordering fragility. The allowlist regex uses \{Name: to capture Args entries; codegen/templates/command.go.tmpl:39 emits {Name: {{quote .Name}}, Param: {{quote .Param}}, Help: {{quote .Help}}}, with Name: first. If a future template edit reorders (e.g. {Param: ..., Name: ..., Help: ...} — plausible if someone alphabetizes for gofmt-neutral reasons), every Args line stops matching the allowlist and a URL-path parameter rename (Param: "video_id" → Param: "videoId", which changes what URL the CLI actually calls) becomes invisible. Same class as Somansh's allowlist-drift concern but the trigger is a template edit, not a new field. Suggest either widening the pattern (\{(Name|Param):) or matching container entries at the paren-open level (^\t\t\{) rather than the first-field level.

• git ls-tree --name-only "$1" gen/ without -r is fine now, silent-miss later. gen/ is currently flat (16 files, no subdirectories) so this works — I verified git ls-tree --name-only HEAD gen/ returns full paths like gen/ai-clipping.go. If codegen ever moves to subdirectories (per-API-version namespacing, per-group directories), ls-tree without -r returns only the top-level entries and everything under a subdirectory is silently dropped. Defensive -r costs nothing.

• The "manual step" gap Somansh's note doesn't cover. Somansh flagged that the allowlist itself can rot silently and proposed a CI-side completeness check. Separately: nothing enforces that a releaser actually runs step 4. If the pre-release checklist becomes long and a releaser is in a hurry, "compare $LAST_STABLE..origin/main with the surface script" is the exact step that gets skipped on the assumption "this resync looked routine." Worth considering, as a follow-up (not blocking on this PR): run the two scripts in a GHA that comments the diff on the release-cut PR / draft release, so the human step becomes "read the pre-computed comment" rather than "remember to run the script." That closes the "releaser skips it" failure mode the same way Somansh's proposal closes the "allowlist goes stale" one.

🟡 Nits

• Awk parser tab-count heuristic (^\tGroup:, ^\tName:, ^\t\t\tName:, ^\t\t\tHelp:). Depends on gofmt keeping tabs at exactly 1 and 3 depths. Currently correct — gen/*.go is gofmt-formatted and the template emits at those depths. But if the template ever wraps a Spec inside another block (e.g. a per-group container struct), the depths shift and the awk silently returns empty. Consider ^[[:space:]]+Group: / ^[[:space:]]+Help: to be indent-agnostic; the tab counts don't disambiguate anything the surrounding grammar doesn't already.

• Numbering restart in "Trigger the release" hunk. Old file had steps 5./6. there (continuing prior section); the PR renumbers to 1./2. (fresh list). More logical, no bug — flagging so a reviewer isn't surprised that a "renumber only" hunk is in the same PR as the checklist expansion.

• Bump-rule correction is genuinely load-bearing and easy to miss on a first read of the diff. The old rule listed "codegen resyncs" as patch-worthy unconditionally; the new rule qualifies with "purely additive." Consider a one-line release-notes callout in the PR body that pre-existing PRINFRA guidance might reference the old rule and should be updated. (Not code — process hygiene.)

🟢 Positive

• Allowlist-over-denylist reasoning is exactly right, and the anchoring anecdote about the first-draft draft filtering Source / BodyEncoding / JSONName (each of which routes the request) is the strongest justification for the design choice you could ask for — those are the three fields most likely to be silently invisible to a "just drop the noisy ones" filter.
• Schema-presence-collapse (<present>) is a genuinely clever fix for "the field's presence is the contract, its content is noise."
• The testing plan is unusually thorough for a docs-only PR: six simultaneous synthetic breaks in a worktree + tag-range validation (v0.5.0..v0.6.0 → 78 lines, independently verified as additive) + inventory-check-script confirmation that the allowlist covers everything currently emitted. This is the shape of "docs-only but exercised."
• The interpretation table maps every <-side line class to concrete user-facing effect + release-notes action, and the two "addition looks safe but breaks silently" cases (Required: true on a new flag; new Args entry) are called out explicitly. That table is the deliverable for a stressed releaser at 11pm.
• deprecated_flags() catches the "flag keeps shape but stops doing anything" case the surface diff can't see. This is literally the --enable-caption failure mode that motivated the whole series — the check catches its own motivating example.

Verdict

LGTM from my side, leaving as a comment. Somansh's own self-review already flags the biggest concern (allowlist can rot silently) and proposes the right fix (CI-side comm check). My additions are (a) a concrete unemitted-but-imminent field (SendDefaultWhenOmitted) that makes the drift risk non-hypothetical, (b) two template-ordering fragilities that widen the same failure class, (c) a git ls-tree -r defensive tweak, and (d) a "step 4 gets skipped" complement to Somansh's "allowlist gets stale" concern. None are blockers for a docs-only change that ships process, not code — worth folding into a follow-up PR that hardens the check itself.

— Review by Rames D Jusso

[jrusso1020]

Reviewed at 7ea65b3c. Approving. Documentation only, and it closes a real blind spot: a resync that narrows the command surface is indistinguishable in git log from one that doesn't, so diffing the emitted surface is the only thing that can see it.

I verified the two claims that decide whether the checklist actually works.

The allowlist gap is live, not hypothetical. Listing what the template emits today:

$ grep -oE '^[[:space:]]*[A-Za-z][A-Za-z0-9]*:' codegen/templates/command.go.tmpl | sort -u

gives 24 field names. Subtract the four prose fields and two containers the doc deliberately excludes, and every remainder appears in the surface() allowlist except one: SendDefaultWhenOmitted. So the gate ships with a field it cannot see, and that field decides whether a value is transmitted when the user omits the flag — a wire-behavior change, exactly the class this is meant to catch. One token in the grep fixes it. Worth doing before the next stable release rather than after, since the whole point is that the miss is invisible.

(Deprecated: being in the allowlist while the template does not yet emit it is correct, not a bug — the sibling PR adds that emission, and having the term already listed is what stops the first deprecation from landing unnoticed.)

The tag filter earns its place. git tag --list 'v*' --sort=-v:refname currently returns v0.6.1-dev.202608112003 first; the ^v[0-9]+\.[0-9]+\.[0-9]+$ filter is what makes LAST_STABLE resolve to v0.6.0 instead. Without it every comparison would silently baseline against a prerelease. Good catch to have written down.

gen/ is flat today, so the missing -r on git ls-tree is latent rather than live — worth adding anyway since it costs nothing and the failure is silent.

The one thing I'd add to the doc's reasoning. It says of the deprecated_flags() scan: "The match over-matches by design; a false positive is obvious once you can see which flag it named." That is true and it addresses the harmless direction. The dangerous direction is the other one, and every mechanism here fails toward it:

• the awk keys off literal tab depths (/^\tGroup:/, /^\t\t\tName:/), so a template reindent yields zero matches, which prints nothing, which reads as "no newly deprecated flags";
• an allowlist miss in surface() prints nothing for that field;
• if LAST_STABLE ever failed to resolve, the old side would be empty and every line would appear as an addition, which the doc tells the releaser to treat as usually safe.

Each of those failures is silent and produces a confident all-clear, which is the same shape as the problem the checklist exists to catch. Cheapest hardening is a sanity assertion rather than more coverage: have each snippet fail loudly when its own input looks wrong (non-empty LAST_STABLE, non-zero line count from surface(), non-zero match count from the awk). A gate that can only report "clean" is indistinguishable from a gate that is working.

Not blocking any of that — the checklist is strictly better than what it replaces, and none of it can regress anything, since the file is prose.

— Rames Jusso