[object Object]

← back to Cli Printing Press

fix(regenmerge): semantic dedup for AddCommand calls (#462)

97ca79940bc6e61bc66abf11e2694cf3129d745f · 2026-05-01 11:59:35 -0700 · Trevin Chow

* fix(regenmerge): semantic dedup for AddCommand calls

Replaces text-based call normalization with a (parent.AddCommand(ctor))
fingerprint so argument-shape variation across template generations no
longer produces false-positive lost registrations.

Surfaced by dogfooding regen-merge on pypi: the older template emitted
`rootCmd.AddCommand(newPypiCmd(&flags))` while the new template emits
`rootCmd.AddCommand(newPypiCmd(flags))`. The whitespace-collapse
normalizer treated these as different calls, flagged 10 in pypi as
"lost", and would have re-injected them on top of fresh's existing calls
producing duplicate cobra registrations at runtime.

The semantic key collapses to `rootCmd.AddCommand(newPypiCmd)` for both
forms. When parent or constructor cannot be cleanly extracted (chained
calls, inline command literals), falls back to the previous text-based
form.

Tests added:
- arg-shape variation (`flags` vs `&flags`) produces zero lost calls
- same constructor under different parent receivers stays distinct

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(regenmerge): cover chained-call fallback + document variadic limit

Review follow-ups on PR #462:
- Comment near the semantic-key construction noting that variadic
  AddCommand calls fingerprint by first ctor only — pre-existing
  behavior, but worth surfacing now that the key is the load-bearing
  dedup signal.
- TestExtractLostRegistrationsChainedCallFallback exercises the
  text-comparison fallback path (parent receiver is a *ast.CallExpr,
  not *ast.Ident) — pinned but previously untested.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Files touched

Diff

commit 97ca79940bc6e61bc66abf11e2694cf3129d745f
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Fri May 1 11:59:35 2026 -0700

    fix(regenmerge): semantic dedup for AddCommand calls (#462)
    
    * fix(regenmerge): semantic dedup for AddCommand calls
    
    Replaces text-based call normalization with a (parent.AddCommand(ctor))
    fingerprint so argument-shape variation across template generations no
    longer produces false-positive lost registrations.
    
    Surfaced by dogfooding regen-merge on pypi: the older template emitted
    `rootCmd.AddCommand(newPypiCmd(&flags))` while the new template emits
    `rootCmd.AddCommand(newPypiCmd(flags))`. The whitespace-collapse
    normalizer treated these as different calls, flagged 10 in pypi as
    "lost", and would have re-injected them on top of fresh's existing calls
    producing duplicate cobra registrations at runtime.
    
    The semantic key collapses to `rootCmd.AddCommand(newPypiCmd)` for both
    forms. When parent or constructor cannot be cleanly extracted (chained
    calls, inline command literals), falls back to the previous text-based
    form.
    
    Tests added:
    - arg-shape variation (`flags` vs `&flags`) produces zero lost calls
    - same constructor under different parent receivers stays distinct
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    * test(regenmerge): cover chained-call fallback + document variadic limit
    
    Review follow-ups on PR #462:
    - Comment near the semantic-key construction noting that variadic
      AddCommand calls fingerprint by first ctor only — pre-existing
      behavior, but worth surfacing now that the key is the load-bearing
      dedup signal.
    - TestExtractLostRegistrationsChainedCallFallback exercises the
      text-comparison fallback path (parent receiver is a *ast.CallExpr,
      not *ast.Ident) — pinned but previously untested.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    ---------
    
    Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 internal/pipeline/regenmerge/registrations.go      |  51 +++++++--
 internal/pipeline/regenmerge/registrations_test.go | 124 +++++++++++++++++++++
 2 files changed, 166 insertions(+), 9 deletions(-)

diff --git a/internal/pipeline/regenmerge/registrations.go b/internal/pipeline/regenmerge/registrations.go
index b42dea65..f1f05021 100644
--- a/internal/pipeline/regenmerge/registrations.go
+++ b/internal/pipeline/regenmerge/registrations.go
@@ -174,22 +174,55 @@ func formatCallExpr(fset *token.FileSet, ce *ast.CallExpr) (addCommandCall, erro
 	}
 	src := buf.String()
 
-	// Normalize: collapse internal whitespace into single spaces. This
-	// makes `rootCmd.AddCommand(newX(flags))` and the same call across
-	// formatting differences match for set-comparison.
-	normalized := strings.Join(strings.Fields(src), " ")
-
-	// Infer constructor name from the first argument: usually
-	// `newX(args...)` — extract `newX`.
+	// Infer the constructor name from the first argument. Two shapes:
+	//  - `newX(args...)` — extract `newX` from the *ast.CallExpr.
+	//  - `someCmd` — bare ident, treat the ident as the constructor.
 	var ctor string
 	if len(ce.Args) > 0 {
-		if argCall, ok := ce.Args[0].(*ast.CallExpr); ok {
-			if id, ok := argCall.Fun.(*ast.Ident); ok {
+		switch arg := ce.Args[0].(type) {
+		case *ast.CallExpr:
+			if id, ok := arg.Fun.(*ast.Ident); ok {
 				ctor = id.Name
 			}
+		case *ast.Ident:
+			ctor = arg.Name
+		}
+	}
+
+	// Extract the parent receiver: `rootCmd.AddCommand(...)` -> `rootCmd`.
+	// Only handles bare-ident receivers; chained calls fall through to the
+	// text-based fallback below.
+	var parent string
+	if sel, ok := ce.Fun.(*ast.SelectorExpr); ok {
+		if id, ok := sel.X.(*ast.Ident); ok {
+			parent = id.Name
 		}
 	}
 
+	// Semantic dedup key: <parent>.AddCommand(<ctor>). Treats
+	// `rootCmd.AddCommand(newX(flags))` and `rootCmd.AddCommand(newX(&flags))`
+	// as the same call — template generations regularly tweak the
+	// argument shape (pointer vs value, added context arg, etc.) without
+	// changing the registration's identity. A pure text comparison would
+	// flag the older form as "lost" and re-inject it on top of the newer
+	// form, producing duplicate cobra registrations at runtime.
+	//
+	// Limitation: AddCommand is variadic. A multi-arg call like
+	// `rootCmd.AddCommand(newA(), newB(), newC())` fingerprints by the
+	// first ctor only, so additional args don't participate in the
+	// lost-set comparison. Cobra projects almost universally use one
+	// AddCommand call per command, so this hasn't bitten in practice;
+	// revisit if a template starts emitting multi-arg form.
+	//
+	// When parent or ctor can't be cleanly extracted (chained calls,
+	// inline command literals), fall back to whitespace-collapsed source.
+	var normalized string
+	if parent != "" && ctor != "" {
+		normalized = parent + ".AddCommand(" + ctor + ")"
+	} else {
+		normalized = strings.Join(strings.Fields(src), " ")
+	}
+
 	return addCommandCall{source: src, normalized: normalized, constructorName: ctor}, nil
 }
 
diff --git a/internal/pipeline/regenmerge/registrations_test.go b/internal/pipeline/regenmerge/registrations_test.go
index 5d5bf451..97633c3c 100644
--- a/internal/pipeline/regenmerge/registrations_test.go
+++ b/internal/pipeline/regenmerge/registrations_test.go
@@ -99,3 +99,127 @@ func contains(s, sub string) bool {
 	}
 	return false
 }
+
+// TestExtractLostRegistrationsArgShapeNoFalsePositives is the regression test
+// for the pypi dogfood finding: a templated AddCommand call where the new
+// template tweaked the argument shape (e.g., `flags` -> `&flags`, or `c`
+// -> `cmd.Context(), c`) must NOT flag the published form as "lost". Pure
+// text comparison would, and the call would be re-injected on top of fresh's
+// existing call producing duplicate cobra registrations at runtime.
+func TestExtractLostRegistrationsArgShapeNoFalsePositives(t *testing.T) {
+	t.Parallel()
+
+	pubCLI := `package cli
+
+import "github.com/spf13/cobra"
+
+func Execute() {
+	rootCmd := &cobra.Command{Use: "x"}
+	flags := &rootFlags{}
+	rootCmd.AddCommand(newPypiCmd(&flags))
+	rootCmd.AddCommand(newRssCmd(&flags))
+	_ = rootCmd.Execute()
+}
+
+type rootFlags struct{}
+func newPypiCmd(*rootFlags) *cobra.Command { return nil }
+func newRssCmd(*rootFlags) *cobra.Command  { return nil }
+`
+	freshCLI := `package cli
+
+import "github.com/spf13/cobra"
+
+func Execute() {
+	rootCmd := &cobra.Command{Use: "x"}
+	flags := rootFlags{}
+	rootCmd.AddCommand(newPypiCmd(flags))
+	rootCmd.AddCommand(newRssCmd(flags))
+	_ = rootCmd.Execute()
+}
+
+type rootFlags struct{}
+func newPypiCmd(rootFlags) *cobra.Command { return nil }
+func newRssCmd(rootFlags) *cobra.Command  { return nil }
+`
+	pubDir, freshDir := buildSyntheticFixture(t,
+		map[string]string{"internal/cli/root.go": pubCLI},
+		map[string]string{"internal/cli/root.go": freshCLI})
+	regs, err := extractLostRegistrations(pubDir, freshDir)
+	require.NoError(t, err)
+	assert.Empty(t, regs, "arg-shape variation alone should not produce lost registrations")
+}
+
+// TestExtractLostRegistrationsChainedCallFallback pins the fallback contract:
+// when a parent receiver isn't a bare ident (e.g., `getRoot().AddCommand(...)`
+// where the AST root is *ast.CallExpr, not *ast.Ident), the semantic key
+// extraction can't apply, so the call falls back to whitespace-collapsed
+// source comparison. Identical chained calls in pub+fresh must still match
+// under that fallback.
+func TestExtractLostRegistrationsChainedCallFallback(t *testing.T) {
+	t.Parallel()
+
+	chainedCLI := `package cli
+
+import "github.com/spf13/cobra"
+
+func getRoot() *cobra.Command { return &cobra.Command{Use: "x"} }
+
+func Execute() {
+	getRoot().AddCommand(newSubCmd())
+}
+
+func newSubCmd() *cobra.Command { return nil }
+`
+	pubDir, freshDir := buildSyntheticFixture(t,
+		map[string]string{"internal/cli/root.go": chainedCLI},
+		map[string]string{"internal/cli/root.go": chainedCLI})
+	regs, err := extractLostRegistrations(pubDir, freshDir)
+	require.NoError(t, err)
+	assert.Empty(t, regs, "identical chained-call AddCommand should match via text fallback")
+}
+
+// TestExtractLostRegistrationsDistinguishesParents pins the other half of the
+// semantic dedup contract: two calls with the same constructor but different
+// parent receivers are distinct registrations and must not be deduped.
+func TestExtractLostRegistrationsDistinguishesParents(t *testing.T) {
+	t.Parallel()
+
+	pubCLI := `package cli
+
+import "github.com/spf13/cobra"
+
+func Execute() {
+	rootCmd := &cobra.Command{Use: "x"}
+	parentCmd := &cobra.Command{Use: "p"}
+	rootCmd.AddCommand(parentCmd)
+	parentCmd.AddCommand(newSubCmd())
+	_ = rootCmd.Execute()
+}
+
+func newSubCmd() *cobra.Command { return nil }
+`
+	freshCLI := `package cli
+
+import "github.com/spf13/cobra"
+
+func Execute() {
+	rootCmd := &cobra.Command{Use: "x"}
+	rootCmd.AddCommand(newSubCmd())
+	_ = rootCmd.Execute()
+}
+
+func newSubCmd() *cobra.Command { return nil }
+`
+	pubDir, freshDir := buildSyntheticFixture(t,
+		map[string]string{"internal/cli/root.go": pubCLI},
+		map[string]string{"internal/cli/root.go": freshCLI})
+	regs, err := extractLostRegistrations(pubDir, freshDir)
+	require.NoError(t, err)
+
+	// pub registers newSubCmd under parentCmd; fresh registers it under
+	// rootCmd. These are distinct registrations — pub's parentCmd-attached
+	// call should be flagged as lost.
+	require.Len(t, regs, 1, "parentCmd's distinct registration of newSubCmd must be flagged")
+	assert.Contains(t, regs[0].Calls, "parentCmd.AddCommand(newSubCmd())",
+		"lost call should preserve the parentCmd parent (not deduped against rootCmd's call)")
+}

← be309e6a chore(main): release 3.2.0 (#460)  ·  back to Cli Printing Press  ·  ci: drop the PR-title scope allow-list, keep requireScope (# 9bc8276e →