review/settings: clarify model offset-matching; mergeReviewProfiles always returns non-nil

f55c55a→main·

dipree·1mo ago·4 files·+24 added/-11 removed

• modelComponentsMatch: document that a shorter id matches at any offset in a longer one (so a provider/family prefix like 'anthropic-claude- sonnet-4-5' still matches 'claude-sonnet'), and that equal-length arrays are either identical (matched upstream via want==got) or different models. No behavior change; added tests for identical two-component ids, slash-provider stripping, and a provider-component offset match.

• mergeReviewProfiles: drop the both-empty early return so it always returns a fresh, non-nil map; a nil base with a non-nil-empty src no longer yields nil. Behavior-equivalent for current consumers (which treat nil and empty maps the same), but avoids the nil-from-non-nil surprise. Updated the test accordingly.

Sessions

5cbbb55f9459View transcript

[?
Checkout the hand off doc that I just added.Pi·Opus 4.8·2 steps](/content/gh/entireio/cli/session/019eca64-8c2c-7b00-90c6-3aa49738c497#timeline-5cbbb55f9459/index.html)

Changes

4

• cmd/entire/cli

• review

• Mmanifest.go+9/-4

• Mmanifest_test.go+3

• settings

• Msettings.go+3/-4

• Msettings_test.go+9/-3

478 unmodified lines

479
480
481
482
483
484
485
486
483
484
485
486
487
488
489
490
491
492
493
494

478 unmodified lines

// "claude-sonnet-4-5" while rejecting variant suffixes like "gpt-4o-mini" and
// bare version fragments like "4-5".
//
// `short` may appear at any offset in `long`, so a provider/family prefix on
// the recorded model does not block a match: "claude-sonnet" matches
// "anthropic-claude-sonnet-4-5" at offset 1 (the next component "4" is numeric).
//
// Equal-length cases are intentionally rejected here (`len(short) >= len(long)`):
// identical ids already returned true via reviewRunModelMatches's `want == got`
// short-circuit before this helper runs, and two *distinct* equal-length ids
// (e.g. "claude-sonnet" vs "claude-opus") are different models that must not
// match. Legitimate matches are always strict subsets, so `short` is shorter.
// two equal-length component arrays are either identical — already matched via
// reviewRunModelMatches's `want == got` short-circuit before this helper runs,
// since identical arrays imply identical normalized strings — or genuinely
// different models (e.g. "claude-sonnet" vs "claude-opus") that must not match.
// A strict subset needs a longer container, so `short` is always shorter.
func modelComponentsMatch(short, long []string) bool {
    if len(short) == 0 || len(short) >= len(long) {
        return false

Mcmd/entire/cli/review/manifest.go+9/-4

775 unmodified lines

776
777
778
779
780
781
782
783
784

775 unmodified lines

// Identical ids match regardless of component count (via the want==got
        // short-circuit), but two distinct equal-length ids must not.
        {"identical multi-component ids match", "claude-sonnet-4-5", "claude-sonnet-4-5", true},
        {"identical two-component ids match", "claude-sonnet", "claude-sonnet", true},
        {"slash provider prefix stripped then identical", "anthropic/claude-sonnet", "claude-sonnet", true},
        {"family matches across a provider component at offset", "claude-sonnet", "anthropic-claude-sonnet-4-5", true},
        {"thinking-suffix-only difference matches", "claude-sonnet:high", "claude-sonnet:low", true},
        {"equal-length different family does not match", "claude-sonnet", "claude-opus", false},
        {"equal-length different version does not match", "claude-sonnet-4", "claude-sonnet-5", false},

Mcmd/entire/cli/review/manifest_test.go+3

684 unmodified lines

685
686
687
688
688
689
690
691
690
691
692
692
693
694

684 unmodified lines

// 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.
// loaded ClonePreferences) can rely on their maps being left untouched. The
// result is always a fresh, non-nil map (empty when both inputs are empty), so
// callers never receive nil from a non-nil input.
func mergeReviewProfiles(base, src map[string]ReviewProfileConfig) map[string]ReviewProfileConfig {
    if len(base) == 0 && len(src) == 0 {
        return base
    }
    out := make(map[string]ReviewProfileConfig, len(base)+len(src))
    for name, cfg := range base {
        out[name] = cfg

Mcmd/entire/cli/settings/settings.go+3/-4

1189 unmodified lines

1190
1191
1192
1193
1194
1195
1193
1194
1195
1196
1197
1198
1199
1200
1201
1202
1203

1189 unmodified lines

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)
    // The result is always a fresh, non-nil map, even when both inputs are
    // empty/nil, so callers never receive nil from a non-nil input.
    if got := mergeReviewProfiles(nil, nil); got == nil {
        t.Error("merge(nil, nil) should return a non-nil empty map, got nil")
    } else if len(got) != 0 {
        t.Errorf("merge(nil, nil) = %v, want empty", got)
    }
    if got := mergeReviewProfiles(nil, map[string]ReviewProfileConfig{}); got == nil {
        t.Error("merge(nil, emptyNonNil) should return a non-nil empty map, got nil")
    }
}

Mcmd/entire/cli/settings/settings_test.go+9/-3