fix(config): sort SetConfigFields keys, remove redundant MkdirAll, rename test

Kieran Klukas created 2 weeks ago

- Sort keys in SetConfigFields before applying sjson.Set for
  deterministic output regardless of map iteration order.
- Remove duplicate os.MkdirAll from atomicWrite (already done in
  lockConfig).
- Rename concurrent test to _concurrentInProcess to accurately reflect
  that it only exercises the in-process mutex, not the cross-process
  flock.

Change summary

internal/config/store.go      | 16 +++++++++++-----
internal/config/store_test.go |  8 +++++---
2 files changed, 16 insertions(+), 8 deletions(-)

Detailed changes

internal/config/store.go

@@ -172,9 +172,6 @@ func (s *ConfigStore) atomicWrite(scope Scope, fn func(current []byte) ([]byte,
 		return err
 	}
 
-	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
-		return fmt.Errorf("create config directory: %w", err)
-	}
 	return atomicWriteFile(path, newData, 0o600)
 }
 
@@ -221,11 +218,20 @@ func (s *ConfigStore) SetConfigField(scope Scope, key string, value any) error {
 // The write is protected by an in-process mutex and a cross-process flock
 // to prevent races between concurrent writers in different processes.
 func (s *ConfigStore) SetConfigFields(scope Scope, kv map[string]any) error {
+	// Sort keys for deterministic output regardless of map iteration
+	// order. This also ensures consistent results when callers pass
+	// overlapping JSONPath keys (e.g. "a" and "a.b").
+	keys := make([]string, 0, len(kv))
+	for k := range kv {
+		keys = append(keys, k)
+	}
+	slices.Sort(keys)
+
 	err := s.atomicWrite(scope, func(data []byte) ([]byte, error) {
 		v := string(data)
-		for key, value := range kv {
+		for _, key := range keys {
 			var sErr error
-			v, sErr = sjson.Set(v, key, value)
+			v, sErr = sjson.Set(v, key, kv[key])
 			if sErr != nil {
 				return nil, fmt.Errorf("failed to set config field %s: %w", key, sErr)
 			}

internal/config/store_test.go

@@ -732,9 +732,11 @@ func TestRefreshOAuthToken_UsesDiskTokenWhenDifferent(t *testing.T) {
 	require.Equal(t, "refresh-abc", updatedConfig.OAuthToken.RefreshToken)
 }
 
-// TestConfigStore_SetConfigFields_concurrent verifies that concurrent writes do
-// not lose data when protected by the in-process mutex and cross-process flock.
-func TestConfigStore_SetConfigFields_concurrent(t *testing.T) {
+// TestConfigStore_SetConfigFields_concurrentInProcess verifies that
+// concurrent in-process writes do not lose data when serialized by the
+// s.mu mutex. This does not exercise the cross-process flock; testing
+// that would require spawning a separate OS process.
+func TestConfigStore_SetConfigFields_concurrentInProcess(t *testing.T) {
 	t.Parallel()
 
 	dir := t.TempDir()