fix(ai): compare .mcp.json by key, not by bytes

devstack owns ONE KEY in .mcp.json, not the file's formatting, but --check
byte-compared the whole file. Any reformat by an editor, a formatter or another
tool therefore marked the artifact stale forever — and since `ai check` gates
`make ci`, a purely cosmetic change would fail the build while `ai install`
fought the other tool on every commit. This is not hypothetical: it happened
during the dogfood commit.

The JSON-key mode now compares the value at the key path semantically and
rewrites the file only when that value actually differs, so a user's formatting
survives. MergeWhole and MergeFence keep byte comparison, which is correct there
because devstack owns exactly the bytes it writes.

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

Author: gustavobertoi

internal/ai/ai_test.go

@@ -503,3 +503,84 @@ func TestOutputDoesNotDependOnVersion(t *testing.T) {
 		}
 	}
 }
+
+// TestJSONKeyIgnoresFormatting is a regression test for a real failure: another
+// tool reformatted .mcp.json (same JSON, different whitespace) and `ai check`
+// reported it stale — which, with ai-check gating CI, would fail the build on a
+// purely cosmetic change and make devstack fight the other formatter on every
+// commit. devstack owns ONE KEY in that file, not its formatting.
+func TestJSONKeyIgnoresFormatting(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, ".mcp.json")
+
+	// Write it once the normal way.
+	if _, err := Write(buildAll(t, dir)); err != nil {
+		t.Fatalf("Write: %v", err)
+	}
+
+	// Now reformat the file the way a different tool would: identical JSON,
+	// compact array, extra key ordering churn.
+	reformatted := `{
+  "mcpServers": {
+    "devstack": { "command": "devstack", "args": ["ai", "mcp"] }
+  }
+}
+`
+	if err := os.WriteFile(path, []byte(reformatted), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	stale, err := Stale(buildAll(t, dir))
+	if err != nil {
+		t.Fatalf("Stale: %v", err)
+	}
+	for _, a := range stale {
+		if a.Rel == ".mcp.json" {
+			t.Error(".mcp.json reported stale after a cosmetic reformat; devstack owns the key, not the formatting")
+		}
+	}
+
+	// And a write must leave the reformatted file alone.
+	results, err := Write(buildAll(t, dir))
+	if err != nil {
+		t.Fatalf("Write: %v", err)
+	}
+	for _, r := range results {
+		if r.Path == ".mcp.json" && r.Changed {
+			t.Error("devstack rewrote .mcp.json purely to reformat it")
+		}
+	}
+	if got := readFile(t, path); got != reformatted {
+		t.Errorf("the user's formatting was not preserved:\n%s", got)
+	}
+}
+
+// TestJSONKeyDetectsARealChange is the other half: a genuinely wrong value must
+// still be corrected.
+func TestJSONKeyDetectsARealChange(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, ".mcp.json")
+	if err := os.WriteFile(path, []byte(
+		`{"mcpServers":{"devstack":{"command":"WRONG","args":["ai","mcp"]}}}`), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	stale, err := Stale(buildAll(t, dir))
+	if err != nil {
+		t.Fatalf("Stale: %v", err)
+	}
+	var found bool
+	for _, a := range stale {
+		if a.Rel == ".mcp.json" {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatal("a wrong command value should be reported stale")
+	}
+	if _, err := Write(buildAll(t, dir)); err != nil {
+		t.Fatalf("Write: %v", err)
+	}
+	if strings.Contains(readFile(t, path), "WRONG") {
+		t.Error("the wrong value was not corrected")
+	}
+}

internal/ai/writeio.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
 )
 
@@ -63,6 +64,18 @@ type WriteResult struct {
 func Write(arts []Artifact) ([]WriteResult, error) {
 	out := make([]WriteResult, 0, len(arts))
 	for _, a := range arts {
+		// Nothing to do when the artifact's own content already matches. Checking
+		// first — rather than relying on writeIfChanged's byte compare — is what
+		// keeps devstack from reformatting a user-owned JSON file whose devstack
+		// key is already correct.
+		ok, err := satisfied(a)
+		if err != nil {
+			return out, err
+		}
+		if ok {
+			out = append(out, WriteResult{Path: a.Rel, Kind: a.Kind, Changed: false})
+			continue
+		}
 		want, err := merged(a)
 		if err != nil {
 			return out, err
@@ -76,40 +89,88 @@ func Write(arts []Artifact) ([]WriteResult, error) {
 	return out, nil
 }
 
-// UpToDate reports whether every artifact already matches disk. This is the basis
-// for --check, so it must apply the same merge each mode would perform rather
-// than comparing raw bytes.
+// UpToDate reports whether every artifact is already satisfied on disk.
 func UpToDate(arts []Artifact) (bool, error) {
-	for _, a := range arts {
-		want, err := merged(a)
-		if err != nil {
-			return false, err
-		}
-		existing, err := os.ReadFile(a.Path)
-		if err != nil || !bytes.Equal(existing, want) {
-			return false, nil
-		}
-	}
-	return true, nil
+	stale, err := Stale(arts)
+	return len(stale) == 0, err
 }
 
-// Stale returns the artifacts whose on-disk content differs, so --check can name
-// them instead of just failing.
+// Stale returns the artifacts that are not satisfied, so --check can name them
+// instead of just failing.
 func Stale(arts []Artifact) ([]Artifact, error) {
 	var out []Artifact
 	for _, a := range arts {
-		want, err := merged(a)
+		ok, err := satisfied(a)
 		if err != nil {
 			return nil, err
 		}
-		existing, err := os.ReadFile(a.Path)
-		if err != nil || !bytes.Equal(existing, want) {
+		if !ok {
 			out = append(out, a)
 		}
 	}
 	return out, nil
 }
 
+// satisfied reports whether an artifact's contribution is already present on
+// disk. What counts as "present" depends on how much of the file devstack owns.
+//
+// For MergeWhole and MergeFence devstack owns the bytes it writes (the whole file
+// or the fenced block, with everything outside preserved verbatim), so a byte
+// comparison of the merged result is exactly right.
+//
+// For MergeJSONKey devstack owns ONE KEY, not the file's formatting. Comparing
+// bytes there would mean any reformat by an editor, a formatter or another tool
+// marks the artifact stale forever — and since `ai check` gates CI, a purely
+// cosmetic change would fail the build and re-running `ai install` would fight
+// the other tool on every commit. So the key is compared semantically, and the
+// file is rewritten only when the key's VALUE actually differs.
+func satisfied(a Artifact) (bool, error) {
+	existing, err := os.ReadFile(a.Path)
+	if os.IsNotExist(err) {
+		return false, nil
+	}
+	if err != nil {
+		return false, fmt.Errorf("read %s: %w", a.Path, err)
+	}
+	if a.Merge == MergeJSONKey {
+		return jsonKeyMatches(existing, a.JSONPath, a.Data)
+	}
+	want, err := merged(a)
+	if err != nil {
+		return false, err
+	}
+	return bytes.Equal(existing, want), nil
+}
+
+// jsonKeyMatches reports whether the value at path already equals want, compared
+// as JSON values rather than as text.
+func jsonKeyMatches(existing []byte, path []string, want []byte) (bool, error) {
+	if len(bytes.TrimSpace(existing)) == 0 {
+		return false, nil
+	}
+	var doc any
+	if err := json.Unmarshal(existing, &doc); err != nil {
+		// A malformed file is not "satisfied"; Write surfaces the parse error.
+		return false, nil
+	}
+	cur := doc
+	for _, key := range path {
+		obj, ok := cur.(map[string]any)
+		if !ok {
+			return false, nil
+		}
+		cur, ok = obj[key]
+		if !ok {
+			return false, nil
+		}
+	}
+	var wantVal any
+	if err := json.Unmarshal(want, &wantVal); err != nil {
+		return false, fmt.Errorf("decode desired value for %s: %w", strings.Join(path, "."), err)
+	}
+	return reflect.DeepEqual(cur, wantVal), nil
+}
+
 // merged computes the full file content an artifact should produce, reading the
 // current file for the two merge modes that preserve user content.
 func merged(a Artifact) ([]byte, error) {