review: match explicit-model inspectors before default ones; clarify timedOut read safety

b6a149c→main·

dipree·1mo ago·3 files·+95 added/-7 removed

Two review notes:

- Session attribution: a default-model inspector has an empty model, which reviewRunModelMatches treats as matching any recorded model (this is necessary — the session records the resolved default the inspector never named, so rejecting empty-want would stop default inspectors from ever matching). To avoid a default inspector grabbing an explicit-model inspector's session, buildLocalReviewManifestFromSummary now matches in two passes: explicit-model inspectors claim their sessions first, then default inspectors take the rest. Sources still emit in original run order. Adds a regression test (default-first + more-recent explicit session) that fails under the old single-pass logic.

- timedOut read: documented at the final-accounting loop why reading the per-agent fields is race-free (the dispatch loop returns only after close(fanIn), which the close goroutine does after wg.Wait(), i.e. after every goroutine's deferred wg.Done() that follows the field writes). No behavior change; 'go test -race' is clean.

Sessions

94f89f9567c7View 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-94f89f9567c7/index.html)

Changes

3

• cmd/entire/cli/review

• Mmanifest.go+28/-6

• Mmanifest_test.go+60

• Mrun_multi.go+7/-1

67 unmodified lines

StartingSHA:     headSHA,
AggregateOutput: strings.TrimSpace(aggregateOutput),
}
// Match in two passes so inspectors with an explicit model claim their
// specific session before default-model inspectors take the leftovers. A
// default inspector has an empty model, which reviewRunModelMatches treats as
// matching any recorded model (necessary: the session records the resolved
// default the inspector never named) — so without this ordering a default
// inspector processed first could grab an explicit-model inspector's session.
// Sources are still emitted in the original agent-run order.
usedSessions := map[string]bool{}
for _, run := range summary.AgentRuns {
    agentName := agentNameForRun(run)
    st := matchReviewSessionState(worktreeRoot, headSHA, summary.StartedAt, agentName, run.Model, states, usedSessions)
    if st == nil || st.SessionID == "" {
        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
                }
                st := matchReviewSessionState(worktreeRoot, headSHA, summary.StartedAt, agentNameForRun(run), run.Model, states, usedSessions)
                if st == nil || st.SessionID == "" {
                    continue
                }
                usedSessions[st.SessionID] = true
                matched[i] = st
            }
        }
        matchInspectors(true)  // explicit-model inspectors first
        matchInspectors(false) // then default-model inspectors

for i, run := range summary.AgentRuns {
            st := matched[i]
            if st == nil {
                continue
            }
            usedSessions[st.SessionID] = true
            manifest.Sources = append(manifest.Sources, ManifestSource{
                SessionID: st.SessionID,
                Agent:     agentName,
                Agent:     agentNameForRun(run),
                Label:     labelForReviewRun(run),
                Status:    run.Status.String(),
                Output:    agentRunOutput(run),

Mcmd/entire/cli/review/manifest.go+28/-6

853 unmodified lines

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

215 unmodified lines

Mcmd/entire/cli/review/run_multi.go+7/-1

215 unmodified lines