[object Object]

← back to Cli Printing Press

fix(cli): mcp-sync handles older library CLI drift (cliutil + name validation) (#369)

1a4bcae8f94654c64686aee6494bf955a70acfba · 2026-04-28 16:54:54 -0700 · Trevin Chow

* fix(cli): mcp-sync handles older library CLI drift (cliutil + name validation)

Two surgical mcp-sync improvements surfaced by testing the runtime
walker against older library CLIs (Tasks #14 + #15).

cliutil regen — older library CLIs ship internal/cliutil/ packages
that predate helpers the current MCP template uses (e.g.
SanitizeErrorBody, LooksLikeAuthError). Regenerated tools.go fails
to build with "undefined: cliutil.SanitizeErrorBody" on those CLIs
(open-meteo, recipe-goat). Fix: extend GenerateMCPSurfaceOnly to
also regenerate the cliutil package alongside MCP source. Per
AGENTS.md, internal/cliutil is generator-reserved — agents should
not hand-edit it, so unconditional regen is safe. Five files:
fanout.go, text.go, probe.go, ratelimit.go, verifyenv.go. The
spec-conditional cliutil files (freshness, autoRefresh) stay
inside renderOptionalSupportFiles so they don't get emitted when
the spec opts out.

spec.yaml.name vs directory validation — older library CLIs
(weather-goat, open-meteo) have spec.yaml.name fields that diverge
from their directory basename:

  weather-goat/spec.yaml -> name: weather
  open-meteo/spec.yaml   -> name: open-meteo-apis

Without validation, the generator faithfully creates spurious
cmd/<spec.name>-pp-{cli,mcp}/ directories alongside the canonical
cmd/<dir.name>-pp-{cli,mcp}/, AND emits server.NewMCPServer with
the wrong identity. mcp-sync now validates the equality up front
and refuses with an actionable error message: "spec.yaml name
\"weather\" does not match directory basename \"weather-goat\".
Fix spec.yaml's `name:` field to match the directory, or pass
--force to bypass."

Verified end-to-end:
  - mcp-sync open-meteo -> rejects with the expected error
  - mcp-sync weather-goat -> rejects with the expected error
  - mcp-sync espn (matching name) -> proceeds normally

Adds three tests for validateSpecNameMatchesDir covering accept,
reject-with-drift, and no-op-on-empty cases.

Both fixes share the same underlying narrative: mcp-sync's library
backfill needs to be robust to artifacts produced by older generator
versions. The architecture (PR #367) was validated through 7
successful migrations; these are the surgical fixes that close
the gap for the remaining ~15 library CLIs.

Tasks #14 and #15 closed; sweep PR can now run mechanically against
all affected CLIs after the spec.yaml name fixes are applied per CLI
(library-side hand edits, separate PR).

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

* review: address fix-up feedback from self-review

- Rename GenerateMCPSurfaceOnly -> GenerateMCPSurface (the function now
  rewrites cliutil too, so "only" was misleading)
- Expand the cliutil regen comment to call out the deliberate asymmetry
  vs the marker-checked tools.go / handlers.go / root.go paths
- Add TestSyncRegeneratesStaleCliutil integration test that locks the
  contract: a CLI with a stub cliutil/text.go missing SanitizeErrorBody
  gets the helper back after Sync()

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 1a4bcae8f94654c64686aee6494bf955a70acfba
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Tue Apr 28 16:54:54 2026 -0700

    fix(cli): mcp-sync handles older library CLI drift (cliutil + name validation) (#369)
    
    * fix(cli): mcp-sync handles older library CLI drift (cliutil + name validation)
    
    Two surgical mcp-sync improvements surfaced by testing the runtime
    walker against older library CLIs (Tasks #14 + #15).
    
    cliutil regen — older library CLIs ship internal/cliutil/ packages
    that predate helpers the current MCP template uses (e.g.
    SanitizeErrorBody, LooksLikeAuthError). Regenerated tools.go fails
    to build with "undefined: cliutil.SanitizeErrorBody" on those CLIs
    (open-meteo, recipe-goat). Fix: extend GenerateMCPSurfaceOnly to
    also regenerate the cliutil package alongside MCP source. Per
    AGENTS.md, internal/cliutil is generator-reserved — agents should
    not hand-edit it, so unconditional regen is safe. Five files:
    fanout.go, text.go, probe.go, ratelimit.go, verifyenv.go. The
    spec-conditional cliutil files (freshness, autoRefresh) stay
    inside renderOptionalSupportFiles so they don't get emitted when
    the spec opts out.
    
    spec.yaml.name vs directory validation — older library CLIs
    (weather-goat, open-meteo) have spec.yaml.name fields that diverge
    from their directory basename:
    
      weather-goat/spec.yaml -> name: weather
      open-meteo/spec.yaml   -> name: open-meteo-apis
    
    Without validation, the generator faithfully creates spurious
    cmd/<spec.name>-pp-{cli,mcp}/ directories alongside the canonical
    cmd/<dir.name>-pp-{cli,mcp}/, AND emits server.NewMCPServer with
    the wrong identity. mcp-sync now validates the equality up front
    and refuses with an actionable error message: "spec.yaml name
    \"weather\" does not match directory basename \"weather-goat\".
    Fix spec.yaml's `name:` field to match the directory, or pass
    --force to bypass."
    
    Verified end-to-end:
      - mcp-sync open-meteo -> rejects with the expected error
      - mcp-sync weather-goat -> rejects with the expected error
      - mcp-sync espn (matching name) -> proceeds normally
    
    Adds three tests for validateSpecNameMatchesDir covering accept,
    reject-with-drift, and no-op-on-empty cases.
    
    Both fixes share the same underlying narrative: mcp-sync's library
    backfill needs to be robust to artifacts produced by older generator
    versions. The architecture (PR #367) was validated through 7
    successful migrations; these are the surgical fixes that close
    the gap for the remaining ~15 library CLIs.
    
    Tasks #14 and #15 closed; sweep PR can now run mechanically against
    all affected CLIs after the spec.yaml name fixes are applied per CLI
    (library-side hand edits, separate PR).
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    * review: address fix-up feedback from self-review
    
    - Rename GenerateMCPSurfaceOnly -> GenerateMCPSurface (the function now
      rewrites cliutil too, so "only" was misleading)
    - Expand the cliutil regen comment to call out the deliberate asymmetry
      vs the marker-checked tools.go / handlers.go / root.go paths
    - Add TestSyncRegeneratesStaleCliutil integration test that locks the
      contract: a CLI with a stub cliutil/text.go missing SanitizeErrorBody
      gets the helper back after Sync()
    
    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/generator/generator.go        |  35 ++++++++--
 internal/pipeline/mcpsync/sync.go      |  39 ++++++++++-
 internal/pipeline/mcpsync/sync_test.go | 121 +++++++++++++++++++++++++++++++++
 3 files changed, 189 insertions(+), 6 deletions(-)

diff --git a/internal/generator/generator.go b/internal/generator/generator.go
index ba31570b..2c10f4a1 100644
--- a/internal/generator/generator.go
+++ b/internal/generator/generator.go
@@ -1190,21 +1190,46 @@ func (g *Generator) Generate() error {
 	return g.renderVisionAndRootFiles(g.PromotedCommands, g.PromotedResourceNames)
 }
 
-// GenerateMCPSurfaceOnly rewrites the generated MCP entrypoint, tools package,
-// and cobratree helpers without touching the printed CLI's command files.
-func (g *Generator) GenerateMCPSurfaceOnly() error {
+// GenerateMCPSurface rewrites the generated MCP entrypoint, tools package,
+// cobratree helpers, AND the generator-reserved cliutil package without
+// touching the printed CLI's command files. The cliutil package is
+// included because the MCP template references helpers (SanitizeErrorBody,
+// LooksLikeAuthError) that older library CLIs lack — leaving cliutil
+// stale produces "undefined: cliutil.SanitizeErrorBody" build errors on
+// regenerated tools.go. Per AGENTS.md, internal/cliutil is generator-
+// reserved (agents must not hand-edit it), so unconditional regen here
+// is intentionally asymmetric vs the marker-checked tools.go/handlers.go
+// paths in mcp-sync. Spec-conditional cliutil files (freshness,
+// autoRefresh) stay in renderOptionalSupportFiles so they don't get
+// emitted when the spec opts out.
+func (g *Generator) GenerateMCPSurface() error {
 	if err := g.prepareOutput(); err != nil {
 		return err
 	}
 	g.PromotedCommands, g.PromotedResourceNames, g.PromotedEndpointNames = buildPromotedCommandPlan(g.Spec)
-	for tmplName, outPath := range map[string]string{
+	mcpFiles := map[string]string{
 		"cobratree/walker.go.tmpl":   filepath.Join("internal", "mcp", "cobratree", "walker.go"),
 		"cobratree/classify.go.tmpl": filepath.Join("internal", "mcp", "cobratree", "classify.go"),
 		"cobratree/typemap.go.tmpl":  filepath.Join("internal", "mcp", "cobratree", "typemap.go"),
 		"cobratree/shellout.go.tmpl": filepath.Join("internal", "mcp", "cobratree", "shellout.go"),
 		"cobratree/cli_path.go.tmpl": filepath.Join("internal", "mcp", "cobratree", "cli_path.go"),
 		"cobratree/names.go.tmpl":    filepath.Join("internal", "mcp", "cobratree", "names.go"),
-	} {
+		// cliutil files. Deliberately asymmetric with the marker-checked
+		// tools.go / handlers.go / root.go paths elsewhere in mcp-sync:
+		// those files can carry hand-edits and require explicit
+		// confirmation before overwrite, but cliutil is generator-
+		// reserved per AGENTS.md and unconditional regen is the
+		// expected contract. Without this, library CLIs whose cliutil
+		// predates a helper the new MCP template uses (SanitizeErrorBody,
+		// LooksLikeAuthError) fail to build after migration. See the
+		// GenerateMCPSurface doc comment for the full rationale.
+		"cliutil_fanout.go.tmpl":    filepath.Join("internal", "cliutil", "fanout.go"),
+		"cliutil_text.go.tmpl":      filepath.Join("internal", "cliutil", "text.go"),
+		"cliutil_probe.go.tmpl":     filepath.Join("internal", "cliutil", "probe.go"),
+		"cliutil_ratelimit.go.tmpl": filepath.Join("internal", "cliutil", "ratelimit.go"),
+		"cliutil_verifyenv.go.tmpl": filepath.Join("internal", "cliutil", "verifyenv.go"),
+	}
+	for tmplName, outPath := range mcpFiles {
 		if err := g.renderTemplate(tmplName, outPath, g.Spec); err != nil {
 			return fmt.Errorf("rendering %s: %w", tmplName, err)
 		}
diff --git a/internal/pipeline/mcpsync/sync.go b/internal/pipeline/mcpsync/sync.go
index 98139a2d..dc8244d0 100644
--- a/internal/pipeline/mcpsync/sync.go
+++ b/internal/pipeline/mcpsync/sync.go
@@ -59,6 +59,20 @@ func Sync(cliDir string, opts Options) (Result, error) {
 	if err != nil {
 		return Result{}, err
 	}
+	// Validate that spec.yaml.name matches the directory's basename.
+	// Older library CLIs sometimes have drift (weather-goat's
+	// spec.yaml.name = "weather"; open-meteo's name diverges similarly)
+	// because the directory was renamed via emboss/republish but the
+	// spec was never updated. Without this guard, the generator
+	// faithfully creates spurious cmd/<spec.name>-pp-cli/ and
+	// cmd/<spec.name>-pp-mcp/ directories alongside the canonical ones,
+	// and emits server.NewMCPServer(<spec.name>) with the wrong identity.
+	// The fix per CLI is to update spec.yaml's name field to match the
+	// directory; mcp-sync surfaces the divergence rather than silently
+	// generating wrong artifacts.
+	if err := validateSpecNameMatchesDir(cliDir, parsed); err != nil && !opts.Force {
+		return Result{}, err
+	}
 	// Preserve the existing manifest.json's display_name onto the parsed
 	// spec when the spec itself doesn't carry one. Library CLIs printed
 	// before spec.display_name existed (v1.x) lack the canonical source,
@@ -98,7 +112,7 @@ func Sync(cliDir string, opts Options) (Result, error) {
 		gen := generator.New(parsed, cliDir)
 		gen.NovelFeatures = features
 		gen.ModulePath = modulePath
-		if err := gen.GenerateMCPSurfaceOnly(); err != nil {
+		if err := gen.GenerateMCPSurface(); err != nil {
 			return Result{}, fmt.Errorf("rendering MCP surface: %w", err)
 		}
 	}
@@ -379,6 +393,29 @@ func titleCaseFromSlug(slug string) string {
 	return string(runes)
 }
 
+// validateSpecNameMatchesDir refuses to migrate when spec.yaml.name
+// diverges from the CLI directory's basename. This catches the
+// weather-goat / open-meteo class of drift where an old emboss/rename
+// updated the directory but left spec.yaml.name behind, producing
+// spurious cmd/<spec.name>-pp-{cli,mcp}/ directories on regen and a
+// wrong MCP server identity. Caller can pass --force to bypass when
+// they know the divergence is intentional (e.g., a deliberate alias).
+func validateSpecNameMatchesDir(cliDir string, parsed *spec.APISpec) error {
+	if parsed == nil || parsed.Name == "" {
+		return nil
+	}
+	dirName := filepath.Base(cliDir)
+	if dirName == parsed.Name {
+		return nil
+	}
+	return fmt.Errorf(
+		"spec.yaml name %q does not match directory basename %q. "+
+			"This produces spurious cmd/%s-pp-{cli,mcp}/ directories on regen and an incorrect MCP server identity. "+
+			"Fix spec.yaml's `name:` field to match the directory, or pass --force to bypass",
+		parsed.Name, dirName, parsed.Name,
+	)
+}
+
 // removeStaleMCPHandlersFile deletes internal/mcp/handlers.go when it
 // carries the generator's don't-edit marker. Older templates split MCP
 // handlers across tools.go and handlers.go; the current template emits
diff --git a/internal/pipeline/mcpsync/sync_test.go b/internal/pipeline/mcpsync/sync_test.go
index bcd1f2c3..146024d0 100644
--- a/internal/pipeline/mcpsync/sync_test.go
+++ b/internal/pipeline/mcpsync/sync_test.go
@@ -119,6 +119,127 @@ func ExitCode(err error) int { return 0 }
 	assert.NotContains(t, src, "newRootCmd(flags)")
 }
 
+// TestSyncRegeneratesStaleCliutil locks the contract that mcp-sync
+// regenerates the cliutil package alongside MCP source. Older library
+// CLIs ship cliutil packages that predate helpers the new MCP template
+// uses (SanitizeErrorBody, LooksLikeAuthError); without regeneration
+// the regenerated tools.go fails to build with "undefined:
+// cliutil.SanitizeErrorBody". This test simulates that drift by
+// stubbing cliutil/text.go with a minimal file lacking
+// SanitizeErrorBody, runs Sync, and asserts the regenerated file
+// carries the helper.
+func TestSyncRegeneratesStaleCliutil(t *testing.T) {
+	t.Parallel()
+
+	apiSpec := &spec.APISpec{
+		Name:    "cliutiltest",
+		Version: "0.1.0",
+		BaseURL: "https://api.example.com",
+		Auth: spec.AuthConfig{
+			Type:    "api_key",
+			Header:  "Authorization",
+			Format:  "Bearer {token}",
+			EnvVars: []string{"CLIUTILTEST_TOKEN"},
+		},
+		Config: spec.ConfigSpec{
+			Format: "toml",
+			Path:   "~/.config/cliutiltest-pp-cli/config.toml",
+		},
+		Resources: map[string]spec.Resource{
+			"items": {
+				Description: "Manage items",
+				Endpoints: map[string]spec.Endpoint{
+					"list": {Method: "GET", Path: "/items", Description: "List items"},
+				},
+			},
+		},
+	}
+
+	cliDir := filepath.Join(t.TempDir(), "cliutiltest")
+	gen := generator.New(apiSpec, cliDir)
+	require.NoError(t, gen.Generate())
+
+	specData, err := yaml.Marshal(apiSpec)
+	require.NoError(t, err)
+	require.NoError(t, os.WriteFile(filepath.Join(cliDir, "spec.yaml"), specData, 0o644))
+
+	// Stub cliutil/text.go with the marker but without SanitizeErrorBody.
+	// Mirrors the actual drift pattern in open-meteo: the file is generator-
+	// emitted, just from an older template version that didn't ship the
+	// helper yet.
+	textPath := filepath.Join(cliDir, "internal", "cliutil", "text.go")
+	staleText := `// Generated by CLI Printing Press (https://github.com/mvanhorn/cli-printing-press). DO NOT EDIT.
+package cliutil
+
+// CleanText is the only helper this older template emitted.
+func CleanText(s string) string { return s }
+`
+	require.NoError(t, os.WriteFile(textPath, []byte(staleText), 0o644))
+
+	// Stub tools.go so Sync detects MCPSurfaceStaticList state and runs
+	// the migration path (which calls GenerateMCPSurface).
+	oldTools := `// Generated by CLI Printing Press (https://github.com/mvanhorn/cli-printing-press). DO NOT EDIT.
+package mcp
+
+func RegisterNovelFeatureTools() { shellOutToCLI("items list") }
+`
+	require.NoError(t, os.WriteFile(filepath.Join(cliDir, "internal", "mcp", "tools.go"), []byte(oldTools), 0o644))
+
+	result, err := Sync(cliDir, Options{})
+	require.NoError(t, err)
+	assert.True(t, result.Changed)
+
+	regenerated, err := os.ReadFile(textPath)
+	require.NoError(t, err)
+	src := string(regenerated)
+	assert.Contains(t, src, "SanitizeErrorBody", "cliutil/text.go should be regenerated with SanitizeErrorBody")
+	assert.NotContains(t, src, "// CleanText is the only helper this older template emitted.", "stale stub content should be replaced")
+}
+
+// TestValidateSpecNameMatchesDirAccepts ensures matching name and dir
+// pass the preflight without error. The dir basename is the source of
+// truth for the package identity (it appears in go.mod, in directory
+// listings, in the published library URL); spec.yaml.name should track it.
+func TestValidateSpecNameMatchesDirAccepts(t *testing.T) {
+	t.Parallel()
+	cliDir := filepath.Join(t.TempDir(), "weather-goat")
+	require.NoError(t, os.MkdirAll(cliDir, 0o755))
+	parsed := &spec.APISpec{Name: "weather-goat"}
+	if err := validateSpecNameMatchesDir(cliDir, parsed); err != nil {
+		t.Errorf("matching name should accept; got %v", err)
+	}
+}
+
+// TestValidateSpecNameMatchesDirRejectsDrift catches the weather-goat
+// class of bug: spec.yaml.name = "weather" but directory is
+// "weather-goat" because an old rename never updated the spec.
+func TestValidateSpecNameMatchesDirRejectsDrift(t *testing.T) {
+	t.Parallel()
+	cliDir := filepath.Join(t.TempDir(), "weather-goat")
+	require.NoError(t, os.MkdirAll(cliDir, 0o755))
+	parsed := &spec.APISpec{Name: "weather"}
+	err := validateSpecNameMatchesDir(cliDir, parsed)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "spec.yaml name \"weather\"")
+	assert.Contains(t, err.Error(), "directory basename \"weather-goat\"")
+	assert.Contains(t, err.Error(), "--force")
+}
+
+// TestValidateSpecNameMatchesDirNoOpOnEmptySpec — a nil spec or empty
+// name skips validation. The downstream loadArchivedSpec already errors
+// on missing spec; this layer doesn't need to redundantly check.
+func TestValidateSpecNameMatchesDirNoOpOnEmptySpec(t *testing.T) {
+	t.Parallel()
+	cliDir := filepath.Join(t.TempDir(), "anything")
+	require.NoError(t, os.MkdirAll(cliDir, 0o755))
+	if err := validateSpecNameMatchesDir(cliDir, nil); err != nil {
+		t.Errorf("nil spec should not error; got %v", err)
+	}
+	if err := validateSpecNameMatchesDir(cliDir, &spec.APISpec{}); err != nil {
+		t.Errorf("empty name should not error; got %v", err)
+	}
+}
+
 // TestRemoveStaleMCPHandlersFile locks behavior for the food52 case:
 // older generator templates emitted MCP handlers in a separate
 // internal/mcp/handlers.go; the current template emits everything in

← 358ff782 feat(cli): MCP tool surface mirrors Cobra tree at runtime, w  ·  back to Cli Printing Press  ·  fix(cli): transliterate spec strings to ASCII via Unidecode 283c9923 →