test(review): pin the task contract — runtime default briefs every worker, never persisted

c1e199c·

peyton-alt·1w ago·1 file·+102 added/-0 removed

Coverage note after review discussion: the branch no longer changes which workers receive the canonical task (main's #1312 behavior stands: every worker gets the user task or the built-in default). What it does change is persistence — setup writes an empty task and the built-in brief is applied at runtime only. Pin both halves so neither regresses: worker briefing (skill-bearing, skill-less, user-task-verbatim) and the runtime-only nature of the default.

Co-Authored-By: Claude Fable 5 noreply@anthropic.com

Sessions

a457400d7876View transcript

Changes

1

• cmd/entire/cli/review

• Mcmd_test.go+102

68 unmodified lines

69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
1249 unmodified lines

1366
1367
1368
1369
1370
1371
1372
1373
1374
1375
1376
1377
1378
1379
1380
1381
1382
1383
1384
1385
1386
1387
1388
1389
1390
1391
1392
1393
1394
1395
1396
1397
1398
1399
1400
1401
1402
1403
1404
1405
1406
1407
1408
1409
1410
1411
1412
1413
1414
1415
1416
1417
1418
1419
1420
1421
1422
1423
1424
1425
1426
1427
1428

68 unmodified lines

return settings.SaveClonePreferences(ctx, prefs)
}

// seedReviewProfile persists an explicit profile into clone-local
// preferences — unlike seedReviewConfig it does not invent a task, so tests
// can distinguish user-configured tasks from the runtime default.
func seedReviewProfile(ctx context.Context, profile settings.ReviewProfileConfig) error {
    prefs, err := settings.LoadClonePreferences(ctx)
    if err != nil {
        return err
    }
    if prefs == nil {
        prefs = &settings.ClonePreferences{}
    }
    prefs.ReviewDefaultProfile = review.DefaultProfileName
    if profile.Judge == nil {
        if judge := defaultTestJudge(profile.Agents); judge != "" {
            profile.Judge = &settings.ReviewConfig{Agent: judge}
        }
    }
    prefs.ReviewProfiles = map[string]settings.ReviewProfileConfig{
        review.DefaultProfileName: profile,
    }
    return settings.SaveClonePreferences(ctx, prefs)
}

// newCaptureDeps builds minimal Deps that route every launch to reviewer.
func newCaptureDeps(reviewer *captureRunConfigReviewer) review.Deps {
    return review.Deps{
        GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName {
            return []types.AgentName{types.AgentName(reviewer.name)}
        },
        NewSilentError: func(err error) error { return err },
        HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) {
            return false, ""
        },
        ReviewerFor: func(agentName string) reviewtypes.AgentReviewer {
            if agentName == reviewer.name {
                return reviewer
            }
            return nil
        },
    }
}

func defaultTestJudge(cfg map[string]settings.ReviewConfig) string {
    if _, ok := cfg[string(agent.AgentNameClaudeCode)]; ok {
        return string(agent.AgentNameClaudeCode)
    }
    t.Fatal("no SynthesisSink composed")
}

// TestRunReview_TaskSemantics pins the task contract after the
// persistence-only change: the built-in brief is a RUNTIME default — a
// profile saved without a task still briefs every worker (skill-bearing
// included, #1312's deliberate design), while a user-configured task
// reaches workers verbatim. Setup never persisting the built-in text is
// covered separately (TestBuildCrewProfile_NoBuiltinTaskPersisted et al).
func TestRunReview_TaskSemantics(t *testing.T) {
    cases := []struct {
        name     string
        profile  settings.ReviewProfileConfig
        wantTask func(string) bool
        desc     string
    }{
        {
            name: "empty task briefs skill workers with runtime default",
            profile: settings.ReviewProfileConfig{
                Agents: map[string]settings.ReviewConfig{testAgentName: {Skills: []string{"/review"}}},
            },
            wantTask: func(got string) bool { return got != "" },
            desc: "want non-empty runtime default",
        },
        {
            name: "user task reaches workers verbatim",
            profile: settings.ReviewProfileConfig{
                Task:   "Focus on the storage layer only.",
                Agents: map[string]settings.ReviewConfig{testAgentName: {Skills: []string{"/review"}}},
            },
            wantTask: func(got string) bool { return got == "Focus on the storage layer only." },
            desc: "want the user task verbatim",
        },
        {
            name: "skill-less worker briefed too",
            profile: settings.ReviewProfileConfig{
                Agents: map[string]settings.ReviewConfig{testAgentName: {Prompt: "Look carefully."}},
            },
            wantTask: func(got string) bool { return got != "" },
            desc: "want non-empty runtime default",
        },
    }
    for _, tc := range cases {
        t.Run(tc.name, func(t *testing.T) {
            setupCmdTestRepo(t)
            if err := seedReviewProfile(context.Background(), tc.profile); err != nil {
                t.Fatal(err)
            }
            reviewer := &captureRunConfigReviewer{name: testAgentName}
            cmd := review.NewCommand(newCaptureDeps(reviewer))
            cmd.SetOut(&bytes.Buffer{})
            cmd.SetErr(&bytes.Buffer{})
            cmd.SetArgs([]string{"general"})
            if err := cmd.Execute(); err != nil {
                t.Fatalf("unexpected error: %v", err)
            }
            if !tc.wantTask(reviewer.got.Task) {
                t.Errorf("Task = %q, %s", reviewer.got.Task, tc.desc)
            }
        })
    }
}

Mcmd/entire/cli/review/cmd_test.go+102