review: clarify two-pass matched-skip and tighten modelComponentsMatch loop bound

2449c74→main

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

Two low/medium review notes, both clarity (no behavior change):

- Two-pass session matching: the 'already matched' skip was already present (folded into the pass condition with ||). Split it into a standalone 'if matched[i] != nil { continue }' so it's unmistakable, and noted that matched stays index-aligned with summary.AgentRuns. Added a test: an explicit-model inspector with no matching session is left unlinked (not misattributed to the default session) while the default inspector still links.

- modelComponentsMatch: a legitimate match always needs a numeric component after the matched span, so the end-aligned window can never match. Tightened the loop bound from <= to < len(long) to make that strict-subspan invariant explicit in the loop, dropping the now-dead 'end < len(long)' body check. Existing model-match table tests still pass.

Sessions

cb4f9fd21345View transcript

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

Changes

2

• cmd/entire/cli/review

• Mmanifest.go+10/-8

• Mmanifest_test.go+47

78 unmodified lines

79
80
81
82
83
82
83
84
85
86
87
88
89
398 unmodified lines

488
489
490
488
489
490
491
492
493
491
492
493
494
495
496
497
498

78 unmodified lines

matched := make([]*session.State, len(summary.AgentRuns))
    matchInspectors := func(explicitModel bool) {
        for i, run := range summary.AgentRuns {
            if matched[i] != nil || (strings.TrimSpace(run.Model) != "") != explicitModel {
                continue
            }
            if matched[i] != nil {
                continue // already linked (kept index-aligned with summary.AgentRuns)
            }
            if (strings.TrimSpace(run.Model) != "") != explicitModel {
                continue // belongs to the other pass
            }
            st := matchReviewSessionState(worktreeRoot, headSHA, summary.StartedAt, agentNameForRun(run), run.Model, states, usedSessions)
            if st == nil || st.SessionID == "" {
    398 unmodified lines

if len(short) == 0 || len(short) >= len(long) {
        return false
    }
    for i := 0; i+len(short) <= len(long); i++ {
        if !componentsEqualAt(long, short, i) {
            continue
        }
        end := i + len(short)
        if end < len(long) && isNumericComponent(long[end]) {
        // Stop before the end-aligned window (i+len(short) < len(long)): a legitimate
        // match needs a numeric component immediately AFTER the matched span (a
        // version/date), so the span is always a strict, non-suffix subspan of long.
        for i := 0; i+len(short) < len(long); i++ {
            if componentsEqualAt(long, short, i) && isNumericComponent(long[i+len(short)]) {
                return true
            }
        }

Mcmd/entire/cli/review/manifest.go+10/-8

913 unmodified lines

914
915
916
917
918
919
920
921
922
923
924
925
926
927
928
929
930
931
932
933
934
935
936
937
938
939
940
941
942
943
944
945
946
947
948
949
950
951
952
953
954
955
956
957
958
959
960
961
962
963

913 unmodified lines

t.Errorf("opus inspector linked to %q, want sess-opus", manifest.Sources[1].SessionID)
    }
}

// TestBuildLocalReviewManifestFromSummary_ExplicitModelWithoutMatchingSession
// verifies that an explicit-model inspector with no matching session is left
// unlinked (not force-attributed to the default-model session), and that the
// matched slice stays index-aligned so the default inspector still links.
func TestBuildLocalReviewManifestFromSummary_ExplicitModelWithoutMatchingSession(t *testing.T) {
    started := time.Date(2026, 5, 7, 10, 0, 0, 0, time.UTC)
    summary := reviewtypes.RunSummary{
        StartedAt: started,
        AgentRuns: []reviewtypes.AgentRun{
            { // explicit opus inspector, but only a sonnet session exists
                Name:      "claude-code",
                AgentName: "claude-code",
                Model:     "opus",
                Status:    reviewtypes.AgentStatusSucceeded,
                Buffer:    []reviewtypes.Event{reviewtypes.AssistantText{Text: "opus finding"}},
            },
            { // default inspector
                Name:      "claude-code",
                AgentName: "claude-code",
                Model:     "",
                Status:    reviewtypes.AgentStatusSucceeded,
                Buffer:    []reviewtypes.Event{reviewtypes.AssistantText{Text: "default finding"}},
            },
            ,
        },
    }
    states := []*session.State{
        {
            SessionID:    "sess-default",
            Kind:         session.KindAgentReview,
            WorktreePath: "/repo",
            BaseCommit:   "abc123",
            StartedAt:    started.Add(time.Second),
            AgentType:    agenttypes.AgentType("Claude Code"),
            ModelName:    "claude-sonnet-4-5",
        },
    }

manifest := buildLocalReviewManifestFromSummary("/repo", "abc123", summary, states, "")

if len(manifest.Sources) != 1 {
        t.Fatalf("sources = %d, want 1 (opus inspector unmatched, not misattributed)", len(manifest.Sources))
    }
    if manifest.Sources[0].SessionID != "sess-default" || manifest.Sources[0].Output != "default finding" {
        t.Errorf("source = %#v, want sess-default / 'default finding'", manifest.Sources[0])
    }
}