fix: report unedited saves as unchanged, follow Chef precedence in node ssh, surface export config errors

apps/cinc/cmd/client.go

@@ -5,7 +5,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
-	"reflect"
 	"slices"
 
 	cinc "github.com/cinc-project/cinc-api"
@@ -131,7 +130,7 @@ cinc client edit worker-01`,
 				if err != nil {
 					return err
 				}
-				if reflect.DeepEqual(*current, *edited) {
+				if unchanged(*current, *edited) {
 					fmt.Fprintf(cmd.OutOrStdout(), "Client %q unchanged\n", name)
 					return nil
 				}

apps/cinc/cmd/common.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"bytes"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -515,3 +516,14 @@ func realRunFirstRunConfigure(cmd *cobra.Command, cincPath string) error {
 func missingCredentialsError(cincPath string) error {
 	return fmt.Errorf("no credentials yet at %s — run `%s config create` to set one up", cincPath, progname.Get())
 }
+
+// unchanged reports whether an edited object would send the same JSON as the
+// original. The comparison is on the encoding rather than the Go values: the
+// server sends empty attribute maps as {}, the editor's round trip through
+// omitempty turns them into nil, and reflect.DeepEqual calls those different,
+// so an edit that changed nothing would still be PUT and reported as updated.
+func unchanged(before, after any) bool {
+	a, errA := json.Marshal(before)
+	b, errB := json.Marshal(after)
+	return errA == nil && errB == nil && bytes.Equal(a, b)
+}

apps/cinc/cmd/common_test.go

@@ -2,13 +2,18 @@ package cmd
 
 import (
 	"bytes"
+	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
 
+	cinc "github.com/cinc-project/cinc-api"
 	"github.com/spf13/cobra"
 
 	"github.com/cinc-project/cinc-cli/cli/config"
@@ -535,3 +540,81 @@ func TestFirstRunConfiguresWhenChefFileUnusable(t *testing.T) {
 		t.Errorf("an unusable file should not be offered for migration:\n%s", stderr.String())
 	}
 }
+
+// editorRoundTrip is what the real JSON editor does when the user saves
+// without changing anything: marshal the object, then unmarshal it again.
+func editorRoundTrip[T any](t *testing.T) func(*T) (*T, error) {
+	return func(in *T) (*T, error) {
+		b, err := json.Marshal(in)
+		if err != nil {
+			t.Fatal(err)
+		}
+		var out T
+		if err := json.Unmarshal(b, &out); err != nil {
+			t.Fatal(err)
+		}
+		return &out, nil
+	}
+}
+
+// rawObjectServer answers GET on path with body verbatim (so empty maps the
+// server sends survive, unlike re-encoding a Go struct with omitempty) and
+// records whether a PUT arrived.
+func rawObjectServer(t *testing.T, path, body string, put *bool) *httptest.Server {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != path {
+			t.Errorf("unexpected request %s %s", r.Method, r.URL.Path)
+			return
+		}
+		if r.Method == http.MethodPut {
+			*put = true
+		}
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = io.WriteString(w, body)
+	}))
+	t.Cleanup(srv.Close)
+	return srv
+}
+
+func TestEditWithoutChangesSendsNothing(t *testing.T) {
+	for _, tc := range []struct {
+		noun, path, body string
+		stub             func(t *testing.T)
+	}{
+		{
+			noun: "role", path: "/organizations/acme/roles/web",
+			body: `{"name":"web","description":"","json_class":"Chef::Role","chef_type":"role","run_list":[],"default_attributes":{},"override_attributes":{},"env_run_lists":{}}`,
+			stub: func(t *testing.T) { withStubRoleEditor(t, editorRoundTrip[cinc.Role](t)) },
+		},
+		{
+			noun: "environment", path: "/organizations/acme/environments/web",
+			body: `{"name":"web","description":"","json_class":"Chef::Environment","chef_type":"environment","cookbook_versions":{},"default_attributes":{},"override_attributes":{}}`,
+			stub: func(t *testing.T) { withStubEnvironmentEditor(t, editorRoundTrip[cinc.Environment](t)) },
+		},
+	} {
+		t.Run(tc.noun, func(t *testing.T) {
+			var put bool
+			srv := rawObjectServer(t, tc.path, tc.body, &put)
+			tc.stub(t)
+			cfgPath := filepath.Join(t.TempDir(), "credentials")
+			cfg := fmt.Sprintf("[default]\ncinc_server_url = \"%s/organizations/acme\"\nclient_name = \"tim\"\nclient_key = %q\n", srv.URL, writeTestKey(t))
+			if err := os.WriteFile(cfgPath, []byte(cfg), 0o600); err != nil {
+				t.Fatal(err)
+			}
+			root := newRootCmd()
+			var buf bytes.Buffer
+			root.SetOut(&buf)
+			root.SetArgs([]string{tc.noun, "edit", "web", "--config", cfgPath})
+			if err := root.Execute(); err != nil {
+				t.Fatalf("cinc %s edit: %v", tc.noun, err)
+			}
+			if put {
+				t.Errorf("an unedited save sent a PUT; output %q", buf.String())
+			}
+			if !strings.Contains(buf.String(), "unchanged") {
+				t.Errorf("output = %q, want it to say the %s is unchanged", buf.String(), tc.noun)
+			}
+		})
+	}
+}

apps/cinc/cmd/databag.go

@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"reflect"
 	"slices"
 
 	cinc "github.com/cinc-project/cinc-api"
@@ -351,7 +350,7 @@ cinc databag item edit passwords mysql`,
 				if err != nil {
 					return err
 				}
-				if reflect.DeepEqual(current, edited) {
+				if unchanged(current, edited) {
 					fmt.Fprintf(cmd.OutOrStdout(), "Item %q in bag %q unchanged\n", id, bag)
 					return nil
 				}

apps/cinc/cmd/databag_secret.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"reflect"
 
 	cinc "github.com/cinc-project/cinc-api"
 	"github.com/spf13/cobra"
@@ -200,7 +199,7 @@ cinc databag secret edit passwords mysql --secret-file ~/.cinc/secret`,
 				if err != nil {
 					return err
 				}
-				if reflect.DeepEqual(plain, edited) {
+				if unchanged(plain, edited) {
 					fmt.Fprintf(cmd.OutOrStdout(), "Encrypted item %q in bag %q unchanged\n", id, bag)
 					return nil
 				}

apps/cinc/cmd/environment.go

@@ -5,7 +5,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
-	"reflect"
 	"slices"
 
 	cinc "github.com/cinc-project/cinc-api"
@@ -66,7 +65,7 @@ cinc environment edit prod`,
 				if err != nil {
 					return err
 				}
-				if reflect.DeepEqual(*current, *edited) {
+				if unchanged(*current, *edited) {
 					fmt.Fprintf(cmd.OutOrStdout(), "Environment %q unchanged\n", name)
 					return nil
 				}

apps/cinc/cmd/group.go

@@ -5,7 +5,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
-	"reflect"
 	"slices"
 	"strings"
 
@@ -70,7 +69,7 @@ cinc group edit admins`,
 				if err != nil {
 					return err
 				}
-				if reflect.DeepEqual(*current, *edited) {
+				if unchanged(*current, *edited) {
 					fmt.Fprintf(cmd.OutOrStdout(), "Group %q unchanged\n", name)
 					return nil
 				}

apps/cinc/cmd/keys.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"reflect"
 	"slices"
 	"strings"
 
@@ -214,7 +213,7 @@ cinc %[1]s key edit %[2]s rotation --file regenerate.json --key-file rotation.pe
 				if err != nil {
 					return err
 				}
-				if reflect.DeepEqual(*current, *edited) {
+				if unchanged(*current, *edited) {
 					fmt.Fprintf(cmd.OutOrStdout(), "Key %q on %s %q unchanged\n", keyName, owner.noun, ownerName)
 					return nil
 				}

apps/cinc/cmd/node.go

@@ -606,24 +606,23 @@ func expandNodeSSHQuery(query string) string {
 	return "tags:*" + query + "* OR roles:*" + query + "* OR fqdn:*" + query + "* OR addresses:*" + query + "*"
 }
 
+// searchRowAttribute reads the SSH host attribute from one node search row.
+// The lookup follows Chef's read precedence (automatic, override, normal,
+// default) and accepts dotted paths such as cloud.public_hostname, the way
+// node[...] does in a recipe. "name" is the node's own name, which isn't an
+// attribute.
 func searchRowAttribute(row json.RawMessage, attr string) (string, error) {
-	var data any
-	if err := json.Unmarshal(row, &data); err != nil {
+	var node cinc.Node
+	if err := json.Unmarshal(row, &node); err != nil {
 		return "", err
 	}
-	for _, path := range candidateAttributePaths(attr) {
-		if value, ok := lookupAttribute(data, path); ok {
-			return attributeString(value), nil
-		}
+	if attr == "name" {
+		return node.Name, nil
 	}
-	return "", fmt.Errorf("search row missing SSH attribute %q", attr)
-}
-
-func candidateAttributePaths(attr string) [][]string {
-	if strings.Contains(attr, ".") {
-		return [][]string{strings.Split(attr, ".")}
+	if _, ok := node.Attribute(attr); !ok {
+		return "", fmt.Errorf("search row missing SSH attribute %q", attr)
 	}
-	return [][]string{{attr}, {"automatic", attr}, {"normal", attr}, {"default", attr}, {"override", attr}}
+	return node.AttributeString(attr), nil
 }
 
 func lookupAttribute(data any, path []string) (any, bool) {

apps/cinc/cmd/node_test.go

@@ -871,3 +871,23 @@ func TestNodeEditCommandReadsFromFile(t *testing.T) {
 		t.Errorf("PUT body = %+v, want name=web01 environment=qa", gotPut)
 	}
 }
+
+func TestSearchRowAttributeFollowsChefPrecedence(t *testing.T) {
+	for _, tc := range []struct{ name, row, attr, want string }{
+		{"dotted path under automatic", `{"automatic":{"cloud":{"public_hostname":"web01.cloud.test"}}}`, "cloud.public_hostname", "web01.cloud.test"},
+		{"override beats default", `{"default":{"ipaddress":"10.0.0.1"},"override":{"ipaddress":"10.0.0.2"}}`, "ipaddress", "10.0.0.2"},
+		{"automatic beats normal", `{"normal":{"fqdn":"n.test"},"automatic":{"fqdn":"a.test"}}`, "fqdn", "a.test"},
+		{"first element of an array", `{"automatic":{"addresses":["10.0.0.3","10.0.0.4"]}}`, "addresses", "10.0.0.3"},
+		{"node name", `{"name":"web01","automatic":{}}`, "name", "web01"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := searchRowAttribute(json.RawMessage(tc.row), tc.attr)
+			if err != nil || got != tc.want {
+				t.Errorf("searchRowAttribute(%s, %q) = %q, %v; want %q", tc.row, tc.attr, got, err, tc.want)
+			}
+		})
+	}
+	if _, err := searchRowAttribute(json.RawMessage(`{"automatic":{}}`), "cloud.public_hostname"); err == nil {
+		t.Error("want an error when the attribute is missing")
+	}
+}

apps/cinc/cmd/org.go

@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"reflect"
 	"slices"
 
 	cinc "github.com/cinc-project/cinc-api"
@@ -259,7 +258,7 @@ cinc org edit acme`,
 				if err != nil {
 					return err
 				}
-				if reflect.DeepEqual(*current, *edited) {
+				if unchanged(*current, *edited) {
 					fmt.Fprintf(cmd.OutOrStdout(), "Organization %q unchanged\n", name)
 					return nil
 				}

apps/cinc/cmd/policy_deploy.go

@@ -93,9 +93,16 @@ cinc policy export Policyfile.lock.json ./bundle --archive`,
 				destDir = args[1]
 			}
 
-			// Export only needs the server for chef_server cookbook sources, so
-			// a missing/unusable config is tolerated here (nil Chef client).
-			chef, _ := resolveClient(cmd)
+			// Export only needs the server for chef_server cookbook sources.
+			// Without one it works offline and never reads the config (which
+			// could otherwise start first-run setup); with one, a config
+			// problem is reported as itself.
+			var chef *cinc.Client
+			if lockNeedsServer(lock) {
+				if chef, err = resolveClient(cmd); err != nil {
+					return err
+				}
+			}
 			fetcher, err := newFetcher(filepath.Dir(lockPath), chef)
 			if err != nil {
 				return err
@@ -206,6 +213,17 @@ func resolvePushArchivePath(arg string) (string, error) {
 // newFetcher builds a policyfile.Fetcher rooted at the default cinc cookbook
 // cache, resolving path sources relative to lockDir and using chef for
 // chef_server sources.
+// lockNeedsServer reports whether any cookbook the lock pins is fetched from
+// the Cinc/Chef server rather than a path, git, or artifact source.
+func lockNeedsServer(lock *cinc.PolicyRevision) bool {
+	for _, cl := range lock.CookbookLocks {
+		if kind, _, err := cl.Origin(); err == nil && kind == cinc.SourceChefServer {
+			return true
+		}
+	}
+	return false
+}
+
 func newFetcher(lockDir string, chef *cinc.Client) (*policyfile.Fetcher, error) {
 	cacheRoot, err := policyfile.DefaultCacheRoot()
 	if err != nil {

apps/cinc/cmd/policy_deploy_test.go

@@ -244,3 +244,56 @@ func TestPolicyExportCommandEndToEnd(t *testing.T) {
 		t.Errorf("output = %q", out)
 	}
 }
+
+// Export needs the server only for chef_server cookbook sources. A lock
+// without one exports offline whatever state the config is in; a lock with
+// one surfaces the config problem instead of a vague "requires a configured
+// server connection" from deep inside the fetch.
+func TestPolicyExportResolvesTheServerOnlyWhenTheLockNeedsIt(t *testing.T) {
+	broken := filepath.Join(t.TempDir(), "credentials")
+	if err := os.WriteFile(broken, []byte("[default\nnot toml"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	t.Run("path sources ignore the config", func(t *testing.T) {
+		lockPath := writePolicyLockFixture(t, "0000000000000000000000000000000000000003")
+		root := newRootCmd()
+		root.SetOut(io.Discard)
+		root.SetArgs([]string{"policy", "export", lockPath, filepath.Join(t.TempDir(), "bundle"), "--config", broken})
+		if err := root.Execute(); err != nil {
+			t.Fatalf("cinc policy export of a path-sourced lock: %v", err)
+		}
+	})
+
+	t.Run("chef_server sources report the config error", func(t *testing.T) {
+		lockPath := writePolicyLockFixture(t, "0000000000000000000000000000000000000004")
+		data, err := os.ReadFile(lockPath)
+		if err != nil {
+			t.Fatal(err)
+		}
+		var lock map[string]any
+		if err := json.Unmarshal(data, &lock); err != nil {
+			t.Fatal(err)
+		}
+		base := lock["cookbook_locks"].(map[string]any)["base"].(map[string]any)
+		base["source_options"] = map[string]any{"chef_server": "https://chef.example.test/organizations/acme"}
+		base["cache_key"] = "base-1.0.0-chef.example.test"
+		if data, err = json.Marshal(lock); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(lockPath, data, 0o644); err != nil {
+			t.Fatal(err)
+		}
+		root := newRootCmd()
+		root.SetOut(io.Discard)
+		root.SetErr(io.Discard)
+		root.SetArgs([]string{"policy", "export", lockPath, filepath.Join(t.TempDir(), "bundle"), "--config", broken})
+		err = root.Execute()
+		if err == nil {
+			t.Fatal("want an error: the lock needs the server and the config is unreadable")
+		}
+		if strings.Contains(err.Error(), "requires a configured server connection") || !strings.Contains(err.Error(), broken) {
+			t.Errorf("error = %q, want the config problem naming %s", err, broken)
+		}
+	})
+}