fix(release): ignore gofmt alignment in the command-surface diff (PRINFRA-509)

[somanshreddy]

Scope

Surfaces: CLI | Module: Release process

Summary

The command-surface check reported 27 removals on its first real run — the release cut that ships it. Every one was a phantom.

Root cause

Deprecated is a longer field name than any of its siblings in FlagSpec. Adding it gave gofmt a wider column to align to, so every value in those structs re-padded:

< 			Name:     "brand-voice-id",
> 			Name:       "brand-voice-id",

Same flag, same value, different whitespace. brand-voice-id appears three times on both sides of the comparison; nothing was removed.

Why it mattered

This is not cosmetic. The doc tells a releaser to read every < line as a candidate break and only then look at additions. Twenty-seven lines of whitespace noise is exactly the haystack a genuine removal disappears into — and it would arrive on the release after someone deleted a flag, when the reader has already learned the check cries wolf.

Fix

Collapse the alignment padding before comparing. The release's diff goes from 27 removals to 0, correctly reading as purely additive plus three newly deprecated flags.

Testing

• Re-ran the six-way break worktree: Source, JSONName, BodyEncoding, RequestSchema and Destructive breaks all still surface, so the normalization removes noise without removing signal.
• New test pins the normalization; removing it turns that test red.
• Full suite and lint green.

Found by dogfooding, not by review — nobody spots this reading a diff, only by running the thing on a release where a struct happened to gain a longer field.

[somanshreddy]

Endorse (COMMENTED — my own account; GH-APPROVE routes to @rames Jusso). The normalization is correctly scoped and does NOT drop signal — answering the hardest look precisely.

Why no signal is dropped: sed -E 's/:[[:space:]]+/: /' has no g flag, so it collapses only the first :[[:space:]]+ per line — the struct field's own colon + gofmt alignment padding — and nothing else:

• Field NAME (before the colon) — untouched.
• Field VALUE (after the single space) — untouched, including an internal colon-space in a value (e.g. Default: "key: value"), because first-match-only stops after the field colon.
• Field PRESENCE — untouched (a removed line is still absent → still surfaces).

So the only thing collapsed is the cosmetic alignment gofmt inserts to line values up — exactly the phantom-removal noise — and both sides ($LAST_STABLE and origin/main) run the same normalization, so it stays apples-to-apples. Cases: a re-pad → identical → suppressed ✓; a value change (false→true) → differs → surfaces ✓; a real removal → line absent → surfaces ✓; a value with internal : → first-colon-only leaves the internal intact → real internal changes still surface ✓. Your "5 fields break at once, all five surface" is the empirical confirmation.

Test: TestSurfaceReductionIgnoresGofmtAlignment is a string-PRESENCE pin — it guards against someone deleting the sed (which would re-introduce the 27 phantoms) and documents the why. Good regression guard. Net-new (non-blocking): a BEHAVIORAL golden test would be stronger — feed a re-padded struct block through surface() and assert 0 removals, plus feed a real removal + a value change and assert they surface. That catches a subtly-wrong normalization (e.g. a future g flag over-collapsing value internals) that the presence-pin can't.

Correct fix, unblocks the v0.7.0 cut — and it's a clean validation of the #284 gate's premise: a codegen-adjacent change (#285's Deprecated field) silently perturbed the generated surface, the gate caught it (as noise — the safe direction), and this fix makes a real removal stand out instead of hiding among 27 phantoms.

[james-russo-rames-d-jusso]

HEAD 10af209c · base main · CI all green · peer @somanshreddy left a COMMENTED self-endorsement at 06:14:33Z (own account, since GH-APPROVE routes to Rames Jusso). Layering on with independent verification.

Hardest look: does the normalization drop signal along with noise?

No — verified empirically against the actual affected refs. Ran the pre-fix script (git show da7502a:scripts/release-surface.sh) and the post-fix script against 8652498 (pre-#285, before Deprecated added) → 19d2fc9 (post-#285, after Deprecated added):

script	removals	additions
pre-fix (main script as-of #284)	27	30
post-fix (this PR)	0	3 (the three Deprecated: true lines — correct)

That matches Somansh's phantom-27-removals claim exactly, and shows the fix collapses only the noise.

Then, on top of the PR's 5-way-break test, I ran three single-defect adversarial cases against the fixed pipeline:

• Value change — flipped Endpoint: "/v3/videos/batches" → "/v3/videos/CHANGED". Diff surfaced a single -/+ pair on the Endpoint line. ✓
• Boolean flip — Deprecated: true → Deprecated: false on gen/template.go:49. Single -/+ pair. ✓
• Field removal — deleted that same Deprecated: true, line entirely. Surfaced as a single - line, zero additions. ✓

Why signal survives, mechanically: sed -E 's/:[[:space:]]+/: /' has no g flag → collapses only the first :[[:space:]]+ on each line (the struct field's own colon + gofmt alignment padding). Everything after that first collapsed space is byte-identical — including internal colons in URL values (Endpoint: "/v2/videos:generate" keeps its second : intact, verified) and colons inside string values. Same normalization runs on both sides of the diff, so it stays apples-to-apples. A re-pad → identical after normalization → suppressed. A real change → still differs → surfaces. Somansh's read is correct.

🟡 Concerns (non-blocking)

• codegen/surface_allowlist_test.go:116-125 — new test is a presence pin, not a behavior pin. It greps for the literal sed -E 's/:[[:space:]]+/: /' substring. That guards against deletion of the sed line (its stated purpose), but two failure modes slip past: (a) a refactor that reworks the pipeline with equivalent semantics (e.g. awk, or moving the sed stage) turns the test red on a still-correct fix; (b) a subtly-wrong future edit that keeps the literal string present but breaks behavior (e.g. adding a g flag that over-collapses internal colons, or moving the sed before the grep) leaves the test green while the fix is dead. Somansh's own review flagged the same shape and called out "a behavioral golden test would be stronger." Non-blocking, but worth a follow-up: feed a re-padded block + a real removal + a value change through surface() in-test and assert diff shape (0 lines / 1 line / 1 line). That kind of test would also survive the sed being rewritten in a different tool.

• scripts/release-surface.sh:48 — the load-bearing "no g flag" is uncommented. The correctness of the signal-preservation claim rests on sed collapsing only the FIRST :[[:space:]]+ per line — everything downstream (URLs with internal colons, Default: "key: value"-style values, container-open lines like {Name: "x", Param: "y"}) survives because the second : isn't touched. A future maintainer adding /g "for consistency" would silently over-collapse and reintroduce a false-negative shape (though narrower than the phantom-27 case). One-line inline comment ("no g flag — collapse only the field's own alignment; anything downstream, including URL/JSON colons, must survive") would prevent that.

• RELEASE.md:94-102 docs lag the code. The "Checking for Regressions" section describes the diff without mentioning the normalization step. A releaser reading the doc after some future re-alignment would still expect raw-diff behavior. Tiny follow-up: add a sentence like "Alignment padding is stripped before the compare, so a gofmt re-pad of any struct is invisible to the check." Not a blocker.

Prior-round status (my #284 review)

Sweeping the four gaps I flagged on #284:

• Gap 1 — SendDefaultWhenOmitted allowlist: ✅ present in SURFACE_FIELDS at line 24 (...JSONName:|SendDefaultWhenOmitted:|...).
• Gap 4 — git ls-tree -r: ✅ present at line 30 (git ls-tree -r --name-only "$1" gen/).
• Gap 2 (template-ordering) / Gap 3 (RequestSchema: sed-collapse): not touched by this PR; verified as immaterial to this fix — RequestSchema/ResponseSchema lines in gen/ are all single-line Go string literals (checked all gen/*.go, longest is ~120KB on one line via \n escapes), so the earlier collapse-to-<present> stage runs cleanly before the new alignment-normalization stage.

Positive

• Root-cause writeup in the PR body + commit is exemplary: exact mechanism (gofmt column-widen on new longer field), why it's not cosmetic (removals are what the doc tells you to read), and empirical validation via the 5-way-break test. Found by dogfooding on the release that ships the check — exactly the shape of hazard the #284 gate exists for. Clean loop-close.
• CI all green (test on macOS / Ubuntu / Windows, lint, govulncheck, goreleaser-check, secrets, pr-template).

Verdict

🟢 LGTM from my side — leaving as COMMENTED. Correctness of the normalization is verified empirically on the actual affected refs (27 → 0) and on three independent single-defect cases (value / bool / removal). The 🟡s are follow-up polish, not merge-blockers. Blocks v0.7.0 cut per Somansh's note; unblock at will.

What I didn't verify

• The deprecated sub-command (release-surface.sh deprecated ...) path — unchanged by this PR and out of scope for the fix, but for the record I did not re-run its awk pipeline against the same before/after refs.
• Whether the release note copy published for v0.7.0 correctly calls out the three newly-deprecated brand-voice-id flags (that's a release-process concern downstream of this fix).

— Review by Rames D Jusso

[somanshreddy]

Correction to the /g example in my review above. I verified it and had it backwards: the pin greps for the literal sed -E 's/:[[:space:]]+/: /' including the closing quote, so a /g addition (which makes it …/: /g') does NOT contain that substring (/: /' ≠ /: /g') → the test goes RED. So the presence-pin catches a future /g regression — my earlier "the presence-pin can't catch a g flag" was wrong.

The pin's real limitation is the opposite — it's over-strict: a semantically-equivalent rewrite (awk, or a different-but-correct sed) changes the literal → false-red on a correct fix. That's the accurate reason a behavioral property-test (re-pad → 0 removals; real removal/value-change → surface) is stronger — it's implementation-agnostic — not that the string-pin misses /g. Everything else in the review (the no-g signal-preservation, the endorsement) stands.

[somanshreddy]

Pushed fixes for all three 🟡s rather than deferring them:

• Presence pin → behavioral pin. The reduction is now a reduce mode reading stdin, and the test drives that real pipeline with fixtures instead of grepping for a substring. It survives a rewrite in another tool and fails on a subtly wrong edit.
• The missing g flag is now commented and pinned. Adding /g turns a distinct case red. Getting there took two tries worth flagging: my first fixture used /v2/videos:generate, which has no space after the colon, so /g was a no-op on it and the mutation passed. The case that actually bites is two different values differing only in internal spacing ("fmt: pretty" vs "fmt: pretty") — /g flattens those into one and the check goes blind.
• RELEASE.md now says the padding is stripped, so a releaser isn't expecting raw-diff behavior.

Also noted in the test that Go's cache keys on Go inputs, not the shell script — a stale PASS is possible after editing the script alone, so -count=1 when touching it. That bit me while mutation-testing.

Not doing the deprecated subcommand re-verification you flagged as unverified: unchanged by this PR, and I ran it against the release refs during the cut — it correctly reports the three caption flags.