[object Object]

← back to Cli Printing Press

fix(cli): mcp-sync auto-fixes spec.yaml name drift for internal YAML specs (#400)

d1b9cd27835c986ba4754d69c62946c492ad1467 · 2026-04-29 17:57:43 -0700 · Trevin Chow

* fix(cli): mcp-sync auto-fixes spec.yaml name drift for internal YAML specs

Older library CLIs sometimes have drift where the directory was
renamed via emboss/republish but spec.yaml's `name:` field was left
behind (weather-goat: name: weather, dir: weather-goat). mcp-sync
previously refused with --force required, forcing the agent to open
spec.yaml and rewrite the line by hand before retrying.

For internal YAML specs the rewrite is unambiguous — the top-level
`name:` field is the single source of truth and rewriting it to match
the directory basename is what the user always wants. Auto-fix the
drift in place: rewrite spec.yaml, update parsed.Name in memory, log
the rename, and continue the sync.

OpenAPI and GraphQL specs are left untouched. Their identity comes
from info.title or schema metadata and rewriting is too invasive; the
existing --force-required error path remains for those.

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

* refactor(cli): linearize reconcileSpecNameWithDir control flow

Apply /simplify findings on the new auto-fix:
- Switch os.WriteFile → writeFileAtomic to match the package's
  in-place-rewrite convention (ensureEndpointAnnotation,
  ensureGeneratedPlaceholder both use it). Protects spec.yaml from
  partial writes if interrupted mid-rewrite.
- Lift the spec-file lookup into findInternalYAMLSpec so
  reconcileSpecNameWithDir reads top-to-bottom: locate, decide, rewrite.
  The previous loop's break-as-fallthrough was misleading — it looked
  like iteration but was actually short-circuit logic, and skipped the
  second candidate when the first was OpenAPI rather than missing.
- Trim the doc comment from 13 lines to 7 — drop the bullet list of
  return paths the named returns already convey.

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 d1b9cd27835c986ba4754d69c62946c492ad1467
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Wed Apr 29 17:57:43 2026 -0700

    fix(cli): mcp-sync auto-fixes spec.yaml name drift for internal YAML specs (#400)
    
    * fix(cli): mcp-sync auto-fixes spec.yaml name drift for internal YAML specs
    
    Older library CLIs sometimes have drift where the directory was
    renamed via emboss/republish but spec.yaml's `name:` field was left
    behind (weather-goat: name: weather, dir: weather-goat). mcp-sync
    previously refused with --force required, forcing the agent to open
    spec.yaml and rewrite the line by hand before retrying.
    
    For internal YAML specs the rewrite is unambiguous — the top-level
    `name:` field is the single source of truth and rewriting it to match
    the directory basename is what the user always wants. Auto-fix the
    drift in place: rewrite spec.yaml, update parsed.Name in memory, log
    the rename, and continue the sync.
    
    OpenAPI and GraphQL specs are left untouched. Their identity comes
    from info.title or schema metadata and rewriting is too invasive; the
    existing --force-required error path remains for those.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    * refactor(cli): linearize reconcileSpecNameWithDir control flow
    
    Apply /simplify findings on the new auto-fix:
    - Switch os.WriteFile → writeFileAtomic to match the package's
      in-place-rewrite convention (ensureEndpointAnnotation,
      ensureGeneratedPlaceholder both use it). Protects spec.yaml from
      partial writes if interrupted mid-rewrite.
    - Lift the spec-file lookup into findInternalYAMLSpec so
      reconcileSpecNameWithDir reads top-to-bottom: locate, decide, rewrite.
      The previous loop's break-as-fallthrough was misleading — it looked
      like iteration but was actually short-circuit logic, and skipped the
      second candidate when the first was OpenAPI rather than missing.
    - Trim the doc comment from 13 lines to 7 — drop the bullet list of
      return paths the named returns already convey.
    
    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/mcpsync/sync.go      | 67 +++++++++++++++++++++++++--
 internal/pipeline/mcpsync/sync_test.go | 84 ++++++++++++++++++++++++++++++++++
 2 files changed, 147 insertions(+), 4 deletions(-)

diff --git a/internal/pipeline/mcpsync/sync.go b/internal/pipeline/mcpsync/sync.go
index 0d782e33..9e765e4f 100644
--- a/internal/pipeline/mcpsync/sync.go
+++ b/internal/pipeline/mcpsync/sync.go
@@ -74,12 +74,18 @@ func Sync(cliDir string, opts Options) (Result, error) {
 	// 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 {
+	//
+	// reconcileSpecNameWithDir auto-fixes the common case (internal YAML
+	// spec with a stale top-level `name:` field) by rewriting the line in
+	// place. For OpenAPI/GraphQL specs the fix is too invasive, so it
+	// falls through to the validator's --force-required error.
+	renamedFrom, err := reconcileSpecNameWithDir(cliDir, parsed)
+	if err != nil && !opts.Force {
 		return Result{}, err
 	}
+	if renamedFrom != "" {
+		fmt.Fprintf(os.Stderr, "mcp-sync: rewrote spec.yaml name from %q to %q to match directory basename\n", renamedFrom, parsed.Name)
+	}
 	// 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,
@@ -546,6 +552,59 @@ func validateSpecNameMatchesDir(cliDir string, parsed *spec.APISpec) error {
 	)
 }
 
+// internalSpecNameLine matches the top-level `name:` key in an internal
+// YAML spec. Anchored to start of line so it never matches nested keys.
+var internalSpecNameLine = regexp.MustCompile(`(?m)^name:[ \t]*.*$`)
+
+// reconcileSpecNameWithDir auto-fixes the common drift case (internal
+// YAML spec whose top-level `name:` doesn't match the directory) by
+// rewriting the line in place and updating parsed.Name. Falls back to
+// validateSpecNameMatchesDir's --force-required error for OpenAPI and
+// GraphQL specs, where rewriting info.title or schema metadata is too
+// invasive to do silently. The non-empty renamedFrom return signals
+// the caller to log the rename.
+func reconcileSpecNameWithDir(cliDir string, parsed *spec.APISpec) (renamedFrom string, err error) {
+	if parsed == nil || parsed.Name == "" {
+		return "", nil
+	}
+	dirName := filepath.Base(cliDir)
+	if dirName == parsed.Name {
+		return "", nil
+	}
+	specPath, data, ok := findInternalYAMLSpec(cliDir)
+	if !ok || !internalSpecNameLine.Match(data) {
+		return "", validateSpecNameMatchesDir(cliDir, parsed)
+	}
+	rewritten := internalSpecNameLine.ReplaceAll(data, []byte("name: "+dirName))
+	if err := writeFileAtomic(specPath, rewritten); err != nil {
+		return "", fmt.Errorf("rewriting %s: %w", specPath, err)
+	}
+	oldName := parsed.Name
+	parsed.Name = dirName
+	return oldName, nil
+}
+
+// findInternalYAMLSpec returns the first existing internal YAML spec
+// in cliDir. OpenAPI YAML files are skipped because their identity
+// derives from info.title rather than a top-level name field.
+func findInternalYAMLSpec(cliDir string) (path string, data []byte, ok bool) {
+	for _, candidate := range []string{"spec.yaml", "spec.yml"} {
+		p := filepath.Join(cliDir, candidate)
+		d, err := os.ReadFile(p)
+		if err != nil {
+			if errors.Is(err, fs.ErrNotExist) {
+				continue
+			}
+			return "", nil, false
+		}
+		if openapi.IsOpenAPI(d) {
+			continue
+		}
+		return p, d, true
+	}
+	return "", nil, false
+}
+
 // 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 debbe88f..7a98822d 100644
--- a/internal/pipeline/mcpsync/sync_test.go
+++ b/internal/pipeline/mcpsync/sync_test.go
@@ -321,6 +321,90 @@ func TestValidateSpecNameMatchesDirNoOpOnEmptySpec(t *testing.T) {
 	}
 }
 
+// TestReconcileSpecNameWithDirAutoFixesInternalYAML — the weather-goat
+// case. Internal YAML spec with `name: weather` in a `weather-goat/`
+// directory gets auto-corrected: spec.yaml is rewritten and the parsed
+// struct's Name is updated in-place so downstream generation uses the
+// corrected name.
+func TestReconcileSpecNameWithDirAutoFixesInternalYAML(t *testing.T) {
+	t.Parallel()
+	cliDir := filepath.Join(t.TempDir(), "weather-goat")
+	require.NoError(t, os.MkdirAll(cliDir, 0o755))
+	specPath := filepath.Join(cliDir, "spec.yaml")
+	original := "name: weather\ndescription: \"Weather GOAT\"\nversion: \"1.0.0\"\nbase_url: \"https://api.example.com\"\n"
+	require.NoError(t, os.WriteFile(specPath, []byte(original), 0o644))
+
+	parsed := &spec.APISpec{Name: "weather"}
+	renamedFrom, err := reconcileSpecNameWithDir(cliDir, parsed)
+	require.NoError(t, err)
+	assert.Equal(t, "weather", renamedFrom, "should report the prior name")
+	assert.Equal(t, "weather-goat", parsed.Name, "parsed.Name should be corrected in place")
+
+	// File on disk should have the corrected name; the rest of the
+	// content (description, version, base_url) should be untouched.
+	updated, err := os.ReadFile(specPath)
+	require.NoError(t, err)
+	assert.Contains(t, string(updated), "name: weather-goat\n")
+	assert.NotContains(t, string(updated), "name: weather\n",
+		"the unanchored 'name: weather' line should be gone")
+	assert.Contains(t, string(updated), `description: "Weather GOAT"`)
+	assert.Contains(t, string(updated), `base_url: "https://api.example.com"`)
+}
+
+// TestReconcileSpecNameWithDirSkipsOpenAPI — for OpenAPI specs the name
+// derives from info.title and rewriting it is invasive. Auto-fix should
+// not touch the file; the validator's --force-required error should
+// surface unchanged.
+func TestReconcileSpecNameWithDirSkipsOpenAPI(t *testing.T) {
+	t.Parallel()
+	cliDir := filepath.Join(t.TempDir(), "newname")
+	require.NoError(t, os.MkdirAll(cliDir, 0o755))
+	specPath := filepath.Join(cliDir, "spec.yaml")
+	original := "openapi: 3.0.0\ninfo:\n  title: Old Name\n  version: 1.0.0\npaths: {}\n"
+	require.NoError(t, os.WriteFile(specPath, []byte(original), 0o644))
+
+	parsed := &spec.APISpec{Name: "old-name"}
+	renamedFrom, err := reconcileSpecNameWithDir(cliDir, parsed)
+	require.Error(t, err, "OpenAPI spec must surface the validator error rather than be auto-fixed")
+	assert.Equal(t, "", renamedFrom)
+	assert.Equal(t, "old-name", parsed.Name, "parsed.Name must not change for OpenAPI specs")
+	assert.Contains(t, err.Error(), "--force")
+
+	// File on disk must be untouched.
+	after, err := os.ReadFile(specPath)
+	require.NoError(t, err)
+	assert.Equal(t, original, string(after), "OpenAPI spec must not be rewritten")
+}
+
+// TestReconcileSpecNameWithDirNoOpWhenAligned — no drift, no work.
+func TestReconcileSpecNameWithDirNoOpWhenAligned(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"}
+	renamedFrom, err := reconcileSpecNameWithDir(cliDir, parsed)
+	require.NoError(t, err)
+	assert.Equal(t, "", renamedFrom, "no rename should be reported when names already match")
+}
+
+// TestReconcileSpecNameWithDirNoYAMLFallsThroughToValidator — when no
+// internal YAML spec exists (e.g., spec.json or schema.graphql), the
+// auto-fix path is skipped and the validator's error surfaces.
+func TestReconcileSpecNameWithDirNoYAMLFallsThroughToValidator(t *testing.T) {
+	t.Parallel()
+	cliDir := filepath.Join(t.TempDir(), "weather-goat")
+	require.NoError(t, os.MkdirAll(cliDir, 0o755))
+	// Only a non-YAML spec exists.
+	require.NoError(t, os.WriteFile(filepath.Join(cliDir, "spec.json"), []byte(`{"name":"weather"}`), 0o644))
+
+	parsed := &spec.APISpec{Name: "weather"}
+	renamedFrom, err := reconcileSpecNameWithDir(cliDir, parsed)
+	require.Error(t, err)
+	assert.Equal(t, "", renamedFrom)
+	assert.Equal(t, "weather", parsed.Name, "parsed.Name unchanged when auto-fix declines")
+	assert.Contains(t, err.Error(), "--force")
+}
+
 // 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

← 2d9efda5 fix(skills): polish loads AskUserQuestion via ToolSearch in  ·  back to Cli Printing Press  ·  feat(cli): add GraphQLEndpointPath and EndpointTemplateVars 73d136f7 →