settings: make mergeReviewProfiles pure (no in-place mutation)

7e186be→main·

dipree·1mo ago·2 files·+57 added/-11 removed

mergeReviewProfiles mutated its base map in place before returning it. Today's callers tolerate it (base is always the freshly loaded settings map), but an in-place merge helper is a footgun. Build and return a new map, leaving both inputs untouched. Adds a test for purity + override precedence.

Sessions

28dcc15e4c3bView transcript

?\ Checkout the hand off doc that I just added.Pi·Opus 4.8·3 steps

Changes

2

• cmd/entire/cli/settings

• Msettings.go+16/-11

  • Msettings_test.go+41

675 unmodified lines

// mergeReviewProfiles overlays src review profiles onto base by name. A profile
// from a higher-precedence layer overrides the same-named one from a lower
// layer, but profiles unique to each layer are all preserved. This lets a team
// keep shared profiles in .entire/settings.json while individuals add or
// override profiles in clone-local preferences or .entire/settings.local.json,
// without one layer hiding the others' profiles.
// mergeReviewProfiles overlays src review profiles onto base by name, returning
// a new map. A profile from a higher-precedence layer (src) overrides the
// same-named one from a lower layer (base), but profiles unique to each layer
// are all preserved. This lets a team keep shared profiles in
// .entire/settings.json while individuals add or override profiles in
// clone-local preferences or .entire/settings.local.json, without one layer
// hiding the others' profiles.
//
// Neither input map is mutated: callers (and the maps they own, e.g. a freshly
// loaded ClonePreferences) can rely on their maps being left untouched.
func mergeReviewProfiles(base, src map[string]ReviewProfileConfig) map[string]ReviewProfileConfig {
    if len(src) == 0 {
        if len(base) == 0 && len(src) == 0 {
            return base
        }
        if base == nil {
            base = map[string]ReviewProfileConfig{}
        }
        out := make(map[string]ReviewProfileConfig, len(base)+len(src))
        for name, cfg := range base {
            out[name] = cfg
        }
        for name, cfg := range src {
            base[name] = cfg
            out[name] = cfg
        }
        return base
    }
    return out
}

func applyClonePreferences(settings *EntireSettings, prefs *ClonePreferences) {

Mcmd/entire/cli/settings/settings.go+16/-11

1153 unmodified lines

t.Errorf("AlwaysPrompt = %q, want %q", cfg.AlwaysPrompt, "Be brief.")
    }
}

func TestMergeReviewProfiles_PureAndPrecedence(t *testing.T) {
    t.Parallel()
    base := map[string]ReviewProfileConfig{
        "general":  {Task: "base general"},
        "security": {Task: "base security"},
    }
    src := map[string]ReviewProfileConfig{
        "general": {Task: "override general"}, // overrides base
        "scratch": {Task: "src scratch"},      // unique to src
    }

out := mergeReviewProfiles(base, src)

// Merged result: src overrides same-named, both layers' unique profiles kept.
    if out["general"].Task != "override general" {
        t.Errorf("general = %q, want src override", out["general"].Task)
    }
    if out["security"].Task != "base security" {
        t.Errorf("security = %q, want base preserved", out["security"].Task)
    }
    if out["scratch"].Task != "src scratch" {
        t.Errorf("scratch = %q, want src-only profile kept", out["scratch"].Task)
    }

// Inputs must not be mutated.
    if _, leaked := base["scratch"]; leaked {
        t.Error("base was mutated: src profile leaked into it")
    }
    if base["general"].Task != "base general" {
        t.Errorf("base[general] mutated: %q", base["general"].Task)
    }
    if len(src) != 2 {
        t.Errorf("src mutated: len = %d, want 2", len(src))
    }

// Both empty returns without allocating a non-nil map surprise.
    if got := mergeReviewProfiles(nil, nil); got != nil {
        t.Errorf("merge(nil,nil) = %v, want nil", got)
    }
}