fix(review): stale worker config excludes that worker, not the crew (trail findings) · Entire

fix(review): stale worker config excludes that worker, not the crew (trail findings)

784fa6d→main·

peyton-alt·1w ago·4 files·+141 added/-5 removed

Trail findings on this branch:

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

Sessions

01KX0QBFH472YTJMEF7SXTA46SView transcript

Changes

4

67 unmodified lines

68
69
70
71
72
71
72
73
74
75
76
77
78
79

67 unmodified lines

},
    "codex": {
        {
            Message: "Install codex-review-pack: codex plugins add <url>",
            ProvidesAny: []string{"/codex:adversarial-review"},
            Message: "Install codex-review-pack: codex plugins add <url>",
            // $-form: codex discovery emits $name/$plugin:name invocations,
            // and suppression is an exact string match — a slash-form entry
            // here could never intersect the discovered set, so the hint
            // would show forever even with the plugin installed.
            ProvidesAny: []string{"$codex:adversarial-review"},
        },
    },
    "gemini": {

Mcmd/entire/cli/agent/skilldiscovery/registry.go+6/-2

71 unmodified lines

72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87

71 unmodified lines

t.Error("unknown agent should not be eligible")
    }

// TestActiveInstallHintsFor_CodexFingerprintMatchesDollarFormDiscovery pins
// the suppression fingerprint to the invocation form codex discovery actually
// produces: DiscoverReviewSkills emits "$plugin:name", so a slash-form
// ProvidesAny entry could never intersect the discovered set and the hint
// would show forever even with the plugin installed.
func TestActiveInstallHintsFor_CodexFingerprintMatchesDollarFormDiscovery(t *testing.T) {

t.Parallel()

discovered := map[string]struct{}{"$codex:adversarial-review": {}}
if hints := skilldiscovery.ActiveInstallHintsFor("codex", discovered); len(hints) != 0 {

t.Fatalf("codex hint not suppressed by $-form discovery; got %d hints: %+v", len(hints), hints)
}
}

Mcmd/entire/cli/agent/skilldiscovery/registry_test.go+13

1171 unmodified lines

1172
1173
1174
1175
1176
1177
1178
4 unmodified lines

1183
1184
1185
1185
1186
1187
1186
1187
1188
1189
1190
1191
1192
1193
1194
1195
1196
15 unmodified lines

1212
1213
1214
1215
1216
1217
1218
1219
1220
1221
1222
1223
1224
1225

1171 unmodified lines

checkpointContext = deps.ReviewCheckpointContext(ctx, worktreeRoot, scopeBaseRef)
}
reviewers := make([]reviewtypes.AgentReviewer, 0, len(launchableEligible))
var excludedWorkers []string
for _, choice := range launchableEligible {
workerName := choice.Name
agentCfg := profile.Agents[workerName]
4 unmodified lines

return fmt.Errorf("resolve agent %s: %w", agentName, agErr)
}
if err := VerifyConfiguredSkillsInstalled(ctx, ag, agentCfg); err != nil {
cmd.SilenceUsage = true
fmt.Fprintln(cmd.ErrOrStderr(), err.Error())
return deps.NewSilentError(err)
// One worker's stale config must not hold the whole crew
// hostage (e.g. codex's legacy auto-preselected "/review",
// orphaned when its curated builtin was removed). Exclude
// the worker loudly and let the remaining reviewers run;
// the all-excluded case fails below.
excludedWorkers = append(excludedWorkers, workerName)
fmt.Fprintf(cmd.ErrOrStderr(), "skipping reviewer %s: %s\n", workerName, err.Error())
continue
}
}
reviewer := deps.ReviewerFor(agentName)
15 unmodified lines

})

if len(reviewers) == 0 {
cmd.SilenceUsage = true
err := fmt.Errorf("no runnable reviewers: every configured worker failed skill validation (%s); run `entire review --edit` to reconfigure",
strings.Join(excludedWorkers, ", "))
fmt.Fprintln(cmd.ErrOrStderr(), err.Error())
return deps.NewSilentError(err)
}

runCtx, cancelRun := context.WithCancel(ctx)
defer cancelRun()

Mcmd/entire/cli/review/cmd.go+17/-3

1236 unmodified lines

1237
1238
1239
1240
1241
1242
1243
1244
1245
1246
1247
1248
1249
1250
1251
1252
1253
1254
1255
1256
1257
1258
1259
1260
1261
1262
1263
1264
1265
1266
1267
1268
1269
1270
1271
1272
1273
1274
1275
1276
1277
1278
1279
1280
1281
1282
1283
1284
1285
1286
1287
1288
1289
1290
1291
1292
1293
1294
1295
1296
1297
1298
1299
1300
1301
1302
1303
1304
1305
1306
1307
1308
1309
1310
1311
1312
1313
1314
1315
1316
1317
1318
1319
1320
1321
1322
1323
1324
1325
1326
1327
1328
1329
1330
1331
1332
1333
1334
1335
1336
1337
1338
1339
1340
1341
1342
1343
1344

1236 unmodified lines

t.Fatal("auto synthesis should notify the TUI when the final judge starts/completes")
}

// TestDispatchFork_InvalidSkillExcludesWorkerNotWholeCrew pins the blast
// radius of spawn-time skill validation in multi-agent runs: a worker whose
// configured skill no longer validates (e.g. codex's legacy auto-preselected
// "/review", orphaned when the curated builtin was removed) is excluded with
// a loud warning, and the remaining reviewers still run. Aborting the whole
// crew for one stale entry held every other agent hostage to a codex
// reconfigure.
func TestDispatchFork_InvalidSkillExcludesWorkerNotWholeCrew(t *testing.T) {
setupCmdTestRepo(t)
// Controlled empty HOME: codex discovery finds nothing, so its "/review"
// (no longer a curated builtin) fails validation. Cannot t.Parallel —
// t.Setenv (setupCmdTestRepo already precludes it via t.Chdir).
t.Setenv("HOME", t.TempDir())

if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{
testAgentName: {
Skills: []string{"/review"},
},
testCodexAgent: {
Skills: []string{"/review"}, // stale legacy entry
},
}); err != nil {
t.Fatal(err)
}

claudeReviewer := &captureRunConfigReviewer{name: testAgentName}
codexReviewer := &captureRunConfigReviewer{name: testCodexAgent}
deps := review.Deps{
GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName {
return []types.AgentName{testAgentName, testCodexAgent}
},
NewSilentError: func(err error) error { return err },
HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) {
return false, ""
},
ReviewerFor: func(agentName string) reviewtypes.AgentReviewer {
switch agentName {
case testAgentName:
return claudeReviewer
case testCodexAgent:
return codexReviewer
default:
return nil
},
},
}

cmd := review.NewCommand(deps)
cmd.SetOut(&bytes.Buffer{})
errBuf := &bytes.Buffer{}
cmd.SetErr(errBuf)
cmd.SetArgs([]string{"general"})

if err := cmd.Execute(); err != nil {
t.Fatalf("run should proceed with the valid reviewer, got error: %v", err)
}
if !claudeReviewer.called {
t.Error("claude-code reviewer was not started — valid worker excluded with the invalid one")
}
if codexReviewer.called {
t.Error("codex reviewer started despite failing skill validation")
}
stderr := errBuf.String()
if !strings.Contains(stderr, "/review") || !strings.Contains(stderr, "skipping") {
t.Errorf("stderr should warn about the excluded worker and its skill; got:\n%s", stderr)
}
}

// TestDispatchFork_AllWorkersInvalidStillFails pins the floor: when skill
// validation excludes every worker, the run fails loudly instead of silently
// reviewing with nobody.
func TestDispatchFork_AllWorkersInvalidStillFails(t *testing.T) {
setupCmdTestRepo(t)
t.Setenv("HOME", t.TempDir())

if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{
testCodexAgent: {Skills: []string{"/review"}},
"gemini": {Skills: []string{"$also-missing"}},
}); err != nil {
t.Fatal(err)
}

deps := review.Deps{
GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName {
return []types.AgentName{testCodexAgent, "gemini"}
},
NewSilentError: func(err error) error { return err },
HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) {
return false, ""
},
ReviewerFor: func(agentName string) reviewtypes.AgentReviewer {
return &captureRunConfigReviewer{name: agentName}
},
}

cmd := review.NewCommand(deps)
cmd.SetOut(&bytes.Buffer{})
cmd.SetErr(&bytes.Buffer{})
cmd.SetArgs([]string{"general"})

if err := cmd.Execute(); err == nil {
t.Fatal("expected an error when every worker fails skill validation")
}
}

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