feat(ci): report command-surface changes on PRs that touch gen/ (PRINFRA-509)

[somanshreddy]

Scope

Surfaces: CLI | Module: CI / Release process

Summary

The command-surface check only ran when a releaser remembered step 4 of the pre-release checklist. This runs it automatically on every PR that touches gen/, and posts the result as a comment.

Why the resync PR, not the release cut

A resync that narrows the surface merges cleanly today. It surfaces weeks later at the release cut, interleaved with other resyncs, with nobody left who remembers the change. And the remedy — a deprecated alias in cmd/heygen/aliases.go — belongs to whoever reviews the resync, not to whoever happens to cut the next release.

Release-time stays as the backstop; RELEASE.md now says so.

What the comment says

Deterministic template built by the script. Same input, same bytes — no model in the loop.

### Command surface: 1 command(s) REMOVED — read before merging

**Removed — every script calling these breaks.** Re-register the old path in
`cmd/heygen/aliases.go` or call it out in the release notes.
- `heygen brand glossaries update`

versus a routine resync:

### Command surface: additive only — 2 new command(s)

The headline states the answer so it reads from the PR list without opening anything.

The part that took the work

Attribution. Counting raw diff lines is useless here. The v0.6.0→main range adds one Required: true and one Args entry — both on brand-new commands, where they break nothing. A naive report would flag those, so every routine resync would look identical to one that breaks callers, and people would stop reading it inside a month.

So the report keys every surface line to its owning command, then classifies: removed / existing changed / new. Only the first two can break anyone.

Advisory, never blocking

Two reasons, and the second is the one that matters:

• Reading the result needs judgment CI cannot supply — a removal is correct when the same PR adds the alias.
• A blocking check would deadlock the codegen sync bot, which cannot write that alias itself. A legitimate rename would sit stuck until a human intervened. Non-blocking removes the need for a bypass label.

The job always exits 0, writes to the step summary, and upserts a single comment keyed on an HTML marker so it edits rather than accumulating one per push.

Testing

• Marker consistency between the script and ci.yml is pinned by a test — if they drift nothing fails loudly, the PR just grows a comment per push.
• Classification pinned against immutable history (v0.5.0→v0.6.0: existing commands changed, none removed).
• Both mutation-verified: drifting the marker, and collapsing the changed/added distinction, each turn their own test red.
• Exercised the removal path end to end against a worktree with a command deleted; the report names it and says what to do.

Incidental fix

The script anchored to the repo root. Every git pathspec in it is relative (gen/), so running it from a subdirectory silently matched nothing rather than failing. Found because a test skipped instead of passing — worth chasing rather than papering over.

[somanshreddy]

Both review findings fixed:

• Unpaginated comment lookup — real bug, and it refuted my own assumption. GitHub returns issue comments 30 per page by default, so on a busy PR the marker would fall off page one and the job would post a duplicate on every push, which is the exact failure the marker exists to prevent. Now --paginate with per_page=100.
• Silent 40-line truncation — a cut diff reads as the whole story. It now says ... N more line(s) with the count and where to get the rest. Verified it fires by forcing the cap down to 2.

Neither changes the classification logic, so the existing tests still cover it.

[somanshreddy]

Independent review (Somansh tagged me by name). Nice PR — the failure-toward-nothing discipline, the marker-drift test, and the padding/schema-presence reduction are all right. Findings, sharpest first.

1. [Answers your explicit question] A PR comment is not readable from the PR list — so the headline doesn't meet its own stated goal, and that's the real weakness for a bot-authored PR (not the advisory-ness). The GitHub PR list shows title / labels / review status / check icons — never comment bodies. And surface-report always exit 0, so its check icon is always green and says nothing about removed/changed/added. So today the headline is invisible until someone opens the PR — exactly the "comment nobody opens = no check" outcome you flagged. The fix keeps your deadlock constraint intact (still never blocks): surface the headline where the list actually renders it —

• emit a neutral Check Run (Checks API conclusion: neutral, output.title = "Command surface: 1 REMOVED") — advisory, never red, but the title shows in the list and in the merge box; or
• apply a label (surface:removed / surface:changed) — labels render in the PR list.

Advisory is the right call (blocking deadlocks the sync bot, agreed). But for a bot-authored PR that can ride green required checks to auto-merge, a comment is too weak precisely because nothing in the merge-decision surface reflects a removal. A neutral check or label closes that gap without ever failing the build. This is the one I'd act on.

2. Removal/add detection keys on the generated spec var name (v=$2), not the user-facing heygen <group> <name> identity. removed/added/changed are all cut -f1 over the var …Cmd token; display_name (Group+Name) is only cosmetic. So a codegen template change that renames a var for an unchanged user command reports it as REMOVED + ADDED — a false alarm on the highest-stakes headline. Rare (needs a codegen naming change) and self-evidently wrong when it happens, but since the whole design goal is not crying wolf, keying removal on the stable user-facing identity (Group+Name) would be more semantically faithful. At minimum worth a comment on why var-name keying is acceptable.

3. [Latent trap] The base comparison is correct only because the default checkout resolves HEAD to the PR merge ref. No ref: override + fetch-depth: 0 → HEAD is refs/pull/N/merge, so both BASE_SHA and HEAD already contain any main advancement, and a branch behind main won't misreport main's commands as removed. That safety is load-bearing and uncommented: if anyone later pins the checkout to head.sha (a common 'get the real branch' change), a behind-main PR will false-report every command main added since the branch point as REMOVED. Worth a one-line comment pinning the assumption, or diff against git merge-base "$BASE_SHA" HEAD explicitly.

Minor

• gh api --paginate --jq '…| last' applies the jq per page and concatenates, so last is per-page. With a single marker comment it resolves to one id; but if a prior post-after-failed-lookup ever left two marker comments, you can get two ids → a malformed comments/<id1\n id2> PATCH URL. per_page=100 makes it unlikely, not impossible.
• display_name's ^want\t\tGroup: match depends on reduce emitting exactly one leading tab; if indentation ever shifts, bullets render empty while the counts stay correct — confusing but not wrong.
• The top-level cd $(git rev-parse --show-toplevel) now couples release-surface.sh reduce (a pure stdin filter) to a git-repo cwd; and the usage: die string still says {diff|deprecated} (omits report|reduce) while the case's *) lists them — two usage messages, one stale.

None of 1–3 are blockers; #1 is the one worth doing. Not stamping — this is my own GH identity as author, so a real approve has to come from @rames D Jusso or another non-author.

[james-russo-rames-d-jusso]

Somu — read your own peer review first and this layers on it rather than repeats it. You asked a direct design question ("is advisory too weak a signal for a bot-authored PR?") so I want to answer it head-on, then add what your self-review didn't touch.

Direct answer to your question

Advisory is the right choice on the blocking axis; the weak spot is one layer up, and your self-review already named it — but I want to reinforce it with the empirical shape and take a position on which mitigation to land.

Verified independently:

• Sync-bot PRs already require human review. PR #289 is open right now (heygen-rui-bot, codegen: resync gen/ from EF f4301d8c) and its state is REVIEW_REQUIRED, mergeStateStatus=BLOCKED, autoMergeRequest=null. No auto-merge, no bypass. So "a blocking check would deadlock the sync bot" is only load-bearing if you think the bot's PRs merge without human touch — they don't. A blocking check on REMOVED could be resolved by the reviewer pushing the alias commit to the bot's branch, which is what a rename-handling review already does today.
• The alias file itself is legitimately un-bot-writable. cmd/heygen/aliases.go's own doc comment says aliases "encode naming history that the current spec has no knowledge of" and are "hand-maintained here, not in gen/". So the mechanism half of the deadlock argument (bot can't self-resolve) is correct — it just doesn't imply a deadlock in practice, because the escape hatch is the reviewer, not the bot.
• The "readable from the PR list without opening" claim doesn't hold as implemented, which is exactly your self-review's #1. GitHub's PR list renders title / labels / review status / check-status icon — never comment bodies. The surface-report job always exit 0, so its check icon is always green, so it says nothing about removed/changed/added. Every gen/-touching PR has comment count 1 after the upsert. The headline lives entirely inside the comment body, which is invisible until someone clicks in.

So the design question isn't "advisory vs blocking" — it's "does the advisory signal reach the scan-lane?" Right now, no. Two shapes that fix it without touching the blocking axis (both from your self-review — layering my position on them):

• Neutral Check Run (conclusion: neutral, output.title = "Command surface: 1 REMOVED"). Advisory (never red, never blocks), but the title renders in the PR list's check column and in the merge box. Lowest ceremony.
• Label (surface:removed / surface:changed / surface:additive). Labels render in the PR list and are filterable in the sync-bot dashboard scan.

My position: land B (advisory + list-visible signal) as the strengthening. The neutral-check is the tightest fit — it lives on the same axis reviewers already scan (Checks), it never blocks, and it doesn't add label conventions to rot. Label is the simpler workflow-code change if you want to punt the check-run permission-setup. I would not push for full-blocking (your option C-equivalent) given #287 landed the release-notes backstop below (see the interaction section).

To your explicit ask — "is advisory too weak a signal for a bot-authored PR?" — advisory is fine; the comment-only delivery is what's too weak. Fix delivery, keep advisory.

Empirical verification of the mechanism claim (your self-review didn't trace this)

You said counting diff lines would flag every new command's Required: true and Args entry, and per-command keying dodges that. Traced it end-to-end:

flatten() {
  gen_at "$1" | reduce | awk '
    /^var [A-Za-z0-9_]+ = &command\.Spec\{/ { v=$2; next }
    v != "" { print v "\t" $0 }
  '
}

Every reduced line is prefixed with the enclosing var Foo = &command.Spec{ variable name. Classification partitions on cut -f1 | sort -u: a brand-new command is a NEW spec var, lives only in /tmp/surface-cmds-new.txt, reports as ADDED, and its Required: true / Args lines are keyed under that new var — never entering the "existing changed" diff -q pass. Your example holds: the Required: true and Args entries that arrive with every new command do not trigger a false CHANGED. Design is correct.

Layering on your rename finding (your #2)

Your #2 is right that keying on the generated spec-var name means a codegen template rename of an unchanged user command reports as REMOVED + ADDED. Adjacent observation to fold in: the report doesn't hint that the ADDED entry might be the replacement for the REMOVED one, even when the display names (Group + Name) are identical. That's a real UX gap on the loudest branch. Cheap softening if you keep the var-name keying: when |removed| == |added| == 1 and their display_names match on Group, print "possible rename? confirm" under the REMOVED block. Doesn't need semantic keying to catch the common case.

What your self-review didn't cover

• Awk state carries across files. gen_at concatenates all gen/*.go via git show with no explicit separator. Once v is set in flatten, it stays set until the next var Foo = &command.Spec{ line. If a gen/ file ever contains a Required: / Enum: / Type: line that survives the allowlist filter but sits between commands, or before the first command in a file, it gets attributed to whatever spec var was last seen in the prior file. Fine today because gen/ is pure command specs; becomes a footgun the day someone adds a helpers.go under gen/ with a random surface-field-shaped struct literal. next on ^package or an explicit v="" per-file reset closes it.

• Silent-drop when the script itself fails. The workflow's if ! scripts/release-surface.sh report … > /tmp/surface-report.md; then echo "::warning::…"; exit 0; fi means a die "flatten produced nothing - the check is broken" from the script (or any other die) produces: (a) job exit 0, (b) no comment updated, (c) previous comment stays stale, (d) surface-report check still green in the PR list. Combined with the same "nothing looks identical to a clean bill" property the diff mode already had, a broken report is completely invisible in the merge-decision surface. One-line fix: upsert a "the surface report broke" comment (or a neutral check with output.title = "surface report failed to generate") so the failure state shows up in the same channel a successful report would.

• No test on the REMOVED headline. TestSurfaceReportClassifiesChangeVsAddition pins the "existing changed" branch against v0.5.0..v0.6.0. The REMOVED branch — the loudest branch, the one users of this report will trust most — is only exercised manually per the PR body ("Exercised the removal path end to end against a worktree with a command deleted"). A fixture-driven test that constructs a synthetic old/new with one removed command and asserts the "1 command(s) REMOVED" headline would harden the branch that matters most. Mutation-testing rationale you already use for the marker: same argument for REMOVED.

• Interaction with #287 (release-notes baseline). #287 landed the changelog range fix ($LAST_STABLE..origin/main), so a silent removal that slips this PR's advisory report will appear as a commit in the release-notes commit list at cut time. But /changelog-cli files codegen resyncs under "Internal" and the releaser has to manually add "Breaking changes" per RELEASE.md step 7 — so #287 doesn't automatically surface the removal, it just makes it possible for the releaser to notice the commit that contained it. The pre-release checklist's step 4 (scripts/release-surface.sh diff + deprecated) is what actually names the break. So #287 softens the "resync merged silently" concern but doesn't close it; the release-cut window is still 2-3 weeks in which a distracted releaser could miss it. Which is why moving the signal into the merge-decision surface (neutral check or label) is the higher-leverage strengthening than leaning entirely on the release-time backstop.

• RELEASE.md wording. The new paragraph is clean, but "backstop" + "advisory" together might read to a fresh releaser as "already covered elsewhere, skip step 4." Worth being explicit that the release-time diff is still the gate, not the CI comment.

• display_name awk depends on exactly-\t\t. Not just cosmetic as your review noted — the pattern ^want\t\tGroup: (one tab from flatten's prefix + one tab from the source) is fragile to any future leading-whitespace normalization in reduce (e.g. a sed -E 's/^[[:space:]]+//' added to canonicalize source-side indent). If that happens, the bullet list under REMOVED / CHANGED renders empty (no matches for the awk pattern) while the headline counts stay correct. Empty bullets under a "1 REMOVED" headline reads as "the check misfired," and reviewers stop trusting it. Anchoring on ^v\t + a Group:-anywhere match, or splitting on \t explicitly instead of pattern-string-concatenation, decouples it.

Latent-trap concurrence

Your #3 (checkout resolves HEAD to refs/pull/N/merge, so BASE_SHA..HEAD is safe today but load-bearing on that default) is real and worth the one-line comment you suggest. Diffing against git merge-base "$BASE_SHA" HEAD is the stronger fix if you don't mind the double-git-call cost.

Small stuff

• The --paginate per-page-last jq pointed out in your minor: with per_page=100 you'd need 200+ comments before it matters, but the failure mode (malformed comments/<id1\nid2> in the PATCH URL) is silent-500 rather than obviously wrong. --slurp '[.[] | select(.body | contains($m))] | last | .id // empty' --arg m "$marker" is the boundary-safe form.
• die usage string still lists {diff|deprecated} while the *) handler says expected: diff, deprecated, report, reduce. Two truths in the same file.
• The workflow's permissions: pull-requests: write is right; note that this correctly degrades on fork PRs (the token becomes read-only, upsert warns, step summary still written). No fork PRs open here today — but flagged in case that changes.

What I didn't verify

• Whether the sync bot has ever auto-merged historically. I checked PR #289's current state (REVIEW_REQUIRED, autoMergeRequest=null), not the historical policy. If auto-merge is ever enabled for the bot, the "reviewer pushes the alias" resolution path for a blocking check evaporates — but so does the "human sees the advisory report" mitigation for the current design. Either way, the neutral-check / label strengthening still helps.
• CODEOWNERS / branch protection details (not visible with my token). The mergeStateStatus=BLOCKED signal on #289 is my proxy for "human review is required," which is what matters for the deadlock analysis.

Not stamping — bot-flow authorship + I'm posting as COMMENT.

— Review by Rames D Jusso

[somanshreddy]

All six fixed. Two were real bugs, one of which I introduced in the last round.

The --paginate fix was itself broken. --jq runs once per page, so a match on more than one page emits multiple ids and the PATCH URL becomes comments/<id1\nid2> — a silent 500, not an obvious failure. Verified with per_page=1: two pages, two ids. --slurp is rejected alongside --jq, so it now emits every match and takes tail -1.

Silent-drop on script failure — you were right, and it is the same trap the diff mode already guards against: warn, exit 0, stale comment left looking current, check green. It now upserts a report failed comment saying the check is telling you nothing about this PR, in the same channel a success would use.

Awk state across files — closed, plus a reset on ^package.

Keyed on the command path instead of the spec var. Better than the "possible rename?" hint: a var rename with an unchanged command now reports no user-visible change rather than REMOVED+ADDED. Verified by renaming BrandGlossariesGet → BrandGlossariesFetch and getting a clean report. This also deletes display_name entirely, which closes your fragility finding about its \t\t anchoring — the key is the display name now.

REMOVED-branch test — built from real refs (worktree at v0.6.0 with a command deleted) so the fixture cannot drift from the generated format. Asserts the headline, that the command is named, and that the remedy is pointed at. Mutation-verified twice.

RELEASE.md now says the release-time run is still part of the checklist, not an optional double-check.

Left alone: merge-base instead of base.sha. actions/checkout resolves HEAD to the merge ref, so they agree today; I added the comment rather than the second git call.

Separately — #289 is live proof this was worth building. Its body says "No change to the command surface"; this report says three existing commands changed, and all three flags are gone from --help on that branch.