fix(review): address dogfood-review findings — complete the no-persisted-task fix · Entire
fix(review): address dogfood-review findings — complete the no-persisted-task fix
82e6f09·
peyton-alt·1w ago·11 files·+169 added/-35 removed
Ran the fixed entire review against this branch itself; its verdict identified that fd7d1cbd9 was incomplete and five smaller defects. All confirmed against the code:
- buildCrewProfile, saveReviewProfileConfig, and the flags-configure path still seeded the built-in task into saved profiles, so every interactively-configured profile reintroduced the maximal-audit brief for skill-bearing workers. The prior test was tautological (inputs production never produces); replaced with a test through the real constructor, plus an inverted stale assertion in configure_test.
- Scope-context failure degraded at Debug (invisible at default level) right after the banner claimed the scope was computed. Now Warn plus a one-line stderr notice.
- The categorical "discard out-of-scope findings" rule contradicted truncated file lists (files past the cap are in scope) and rendered nonsense for net-zero diffs (commit + revert). The rule now matches what was rendered: absent without lists, softened to verify-first when truncated.
- Rendered list bytes are charged against the inline-diff budget so the composed prompt stays bounded on ~32KiB-limited platforms.
- Allowlist comment now names the accepted --output residual instead of claiming blanket write-safety.
- Test gaps: fan-out loop now asserts per-worker ScopeContext/Task wiring; uncommitted-only scope pins Diff==""/!DiffOmitted; redundant Skill allowlist test folded into the main one.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Sessions
d4ef50cdaee1View transcript
Changes
11
cmd/entire/cli
agent/claudecode
Mreviewer.go+7/-3
Mreviewer_test.go+1/-17
review
Mcmd.go+8/-7
Mcmd_test.go+14
Mconfigure_test.go+2/-2
Mpicker.go+2/-4
Mpicker_internal_test.go+14
Mprompt.go+14/-1
Mscope.go+19/-1
Mscope_test.go+49
41 unmodified lines
42
43
44
45
45
46
47
48
49
48
49
50
51
52
53
54
55
56
41 unmodified lines
//
// Deliberately narrow: read-only git subcommands are enumerated instead of
// granting Bash(git:*) — git can execute arbitrary code via aliases/hooks,
// and push/commit match the blanket pattern. Nothing write-capable (Edit,
// and push/commit match the blanket pattern. No file tools that write (Edit,
// Write) and never --dangerously-skip-permissions: reviewers process
// untrusted input (the diff under review), so the child must stay unable to
// modify the repo. --allowedTools ADDS to the user's own permission config;
// it cannot revoke anything.
// modify the repo. Known accepted residual: the granted subcommands accept
// --output=<path>, so a prompt-injected reviewer could write query output to
// an arbitrary path — defense-in-depth weakening, not code execution; the
// tradeoff is accepted because the alternative is denying the git access a
// review needs. --allowedTools ADDS to the user's own permission config; it
// cannot revoke anything.
var reviewToolAllowlist = []string{
"Read", "Grep", "Glob", "Task", "TodoWrite", "Skill",
"Bash(git diff:*)", "Bash(git log:*)", "Bash(git show:*)",
Mcmd/entire/cli/agent/claudecode/reviewer.go+7/-3
429 unmodified lines
430
431
432
433
433
434
435
436
47 unmodified lines
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
499
500
501
502
429 unmodified lines
members[strings.TrimSpace(m)] = true
}
for _, want := range []string{
"Read", "Grep", "Glob", "Task",
"Read", "Grep", "Glob", "Task", "Skill",
"Bash(git diff:*)", "Bash(git log:*)", "Bash(git show:*)",
"Bash(git status:*)", "Bash(git blame:*)", "Bash(git rev-parse:*)",
} {
47 unmodified lines
t.Errorf("prose prompt was modified: %q", got)
}
}
// TestReviewer_AllowlistIncludesSkillTool verifies the child can actually
// invoke the configured skills headlessly.
func TestReviewer_AllowlistIncludesSkillTool(t *testing.T) {
t.Parallel()
cmd := buildReviewCmd(context.Background(), reviewtypes.RunConfig{Skills: []string{"/x"}})
for i, arg := range cmd.Args {
if arg == "--allowedTools" {
if !strings.Contains(cmd.Args[i+1], "Skill") {
t.Errorf("allowlist missing Skill tool: %q", cmd.Args[i+1])
}
return
}
}
t.Fatal("--allowedTools not found")
}
Mcmd/entire/cli/agent/claudecode/reviewer_test.go+1/-17
654 unmodified lines
655
656
657
658
659
660
658
659
660
380 unmodified lines
1041
1042
1043
1047
1044
1045
1046
1047
127 unmodified lines
1175
1176
1177
1181
1178
1179
1180
1181
120 unmodified lines
1302
1303
1304
1308
1305
1306
1307
1308
1309
1310
1314
1311
1312
1313
1314
1315
1316
1317
1318
654 unmodified lines
if opts.Task != "" {
profile.Task = opts.Task
}
if strings.TrimSpace(profile.Task) == "" {
profile.Task = profileTask(profileName, settings.ReviewProfileConfig{})
}
// Judge: explicit --set-judge wins; otherwise a multi-reviewer profile gets
// an auto-selected judge, and a single-reviewer profile needs none.
380 unmodified lines
if deps.ReviewCheckpointContext != nil {
checkpointContext = deps.ReviewCheckpointContext(ctx, worktreeRoot, scopeBaseRef)
}
scopeCtx := buildScopeContextBestEffort(ctx, worktreeRoot, scopeBaseRef)
scopeCtx := buildScopeContextBestEffort(ctx, out, worktreeRoot, scopeBaseRef)
runCfg := reviewtypes.RunConfig{
ProfileName: profileName,
127 unmodified lines
if deps.ReviewCheckpointContext != nil {
checkpointContext = deps.ReviewCheckpointContext(ctx, worktreeRoot, scopeBaseRef)
}
scopeCtx := buildScopeContextBestEffort(ctx, worktreeRoot, scopeBaseRef)
scopeCtx := buildScopeContextBestEffort(ctx, out, worktreeRoot, scopeBaseRef)
reviewers := make([]reviewtypes.AgentReviewer, 0, len(launchableEligible))
for _, choice := range launchableEligible {
workerName := choice.Name
120 unmodified lines
// buildScopeContextBestEffort wraps BuildScopeContext for the launch paths:
// scope enumeration is prompt enrichment, so a failure (odd repo state, git
// error) degrades to the descriptive scope clause instead of failing the run.
func buildScopeContextBestEffort(ctx context.Context, worktreeRoot, scopeBaseRef string) reviewtypes.ScopeContext {
func buildScopeContextBestEffort(ctx context.Context, out io.Writer, worktreeRoot, scopeBaseRef string) reviewtypes.ScopeContext {
if scopeBaseRef == "" {
return reviewtypes.ScopeContext{}
}
sc, err := BuildScopeContext(ctx, worktreeRoot, scopeBaseRef)
if err != nil {
logging.Debug(ctx, "review scope context unavailable", slog.String("error", err.Error()))
// Degrading silently would revert to the divergent-scope behavior
// this enumeration exists to prevent, right after the banner told
// the user the scope was computed — so the fallback must be visible.
logging.Warn(ctx, "review scope context unavailable", slog.String("error", err.Error()))
fmt.Fprintln(out, "Note: could not compute the authoritative scope listing; reviewers will derive scope from the base ref themselves.")
return reviewtypes.ScopeContext{}
}
return sc
}
Mcmd/entire/cli/review/cmd.go+8/-7
817 unmodified lines
818
819
820
821
822
823
824
825
826
827
828
829
53 unmodified lines
883
884
885
886
887
888
889
890
891
892
893
894
895
896
817 unmodified lines
t.Fatal(err)
}
cwd, err := os.Getwd()
if err != nil {
t.Fatal(err)
}
testutil.WriteFile(t, cwd, "fanout-dirty.txt", "wip")
claudeReviewer := &captureRunConfigReviewer{name: "claude-code"}
codexReviewer := &captureRunConfigReviewer{name: testCodexAgent}
deps := review.Deps{
53 unmodified lines
if tc.reviewer.got.ReviewerTimeout != 7*time.Minute {
t.Fatalf("%s ReviewerTimeout = %v, want 7m", tc.name, tc.reviewer.got.ReviewerTimeout)
}
// The fan-out loop wires ScopeContext and Task independently of the
// single-agent path; a regression there passes every single-agent test.
if len(tc.reviewer.got.ScopeContext.Uncommitted) == 0 {
t.Fatalf("%s ScopeContext.Uncommitted is empty, want the dirty file", tc.name)
}
if tc.reviewer.got.Task != "Test review task." {
t.Fatalf("%s Task = %q, want the seeded user task", tc.name, tc.reviewer.got.Task)
}
}
}
Mcmd/entire/cli/review/cmd_test.go+14
68 unmodified lines
69
70
71
72
73
72
73
74
75
76
68 unmodified lines
if profile.Judge == nil || profile.Judge.Agent != "codex" {
t.Errorf("judge = %#v, want codex", profile.Judge)
}
if profile.Task == "" {
t.Error("task should default to the built-in general task")
}
if profile.Task != "" {
t.Errorf("task = %q, want empty — the built-in brief is a runtime fallback, never persisted config", profile.Task)
}
}
}
Mcmd/entire/cli/review/configure_test.go+2/-2
433 unmodified lines
434
435
436
437
438
439
438
440
441
442
531 unmodified lines
974
975
976
976
977
978
977
978
979
433 unmodified lines
// a worker keyed by workerIDForAgentModel, which disambiguates duplicates
// (claude-code, claude-code-2, claude-code:opus, …).
func buildCrewProfile(ctx context.Context, profileName string, slots []crewSlot) settings.ReviewProfileConfig {
// Task deliberately left empty: the built-in brief is a runtime fallback
// for skill-less workers, not user configuration to persist.
profile := settings.ReviewProfileConfig{
Task: profileTask(profileName, settings.ReviewProfileConfig{}),
Agents: make(map[string]settings.ReviewConfig, len(slots)),
}
for _, s := range slots {
531 unmodified lines
} else {
profile.Judge = nil
}
if strings.TrimSpace(profile.Task) == "" {
profile.Task = profileTask(profileName, settings.ReviewProfileConfig{})
}
hadProfiles := len(profiles) > 0
profiles[profileName] = profile
defaultName := decodeRawReviewDefault(raw)
Mcmd/entire/cli/review/picker.go+2/-4
76 unmodified lines
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
76 unmodified lines
t.Fatalf("guidedProfileTask with nothing user-provided = %q, want empty", got)
}
}
// TestBuildCrewProfile_NoBuiltinTaskPersisted drives the REAL guided-setup
// profile constructor — not guidedProfileTask with hand-fed empty inputs —
// and asserts it does not seed the built-in brief. This is the test the
// dogfood review flagged as missing: the previous test passed with inputs
// production never produces, while buildCrewProfile still baked the default
// task into every interactively-configured profile.
func TestBuildCrewProfile_NoBuiltinTaskPersisted(t *testing.T) {
t.Parallel()
profile := buildCrewProfile(context.Background(), DefaultProfileName, []crewSlot{{agent: "claude-code"}})
if profile.Task != "" {
t.Errorf("buildCrewProfile persisted Task %q, want empty (built-in brief is runtime fallback only)", profile.Task)
}
}
Mcmd/entire/cli/review/picker_internal_test.go+14
126 unmodified lines
127
128
129
130
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
126 unmodified lines
writeList("Files under review (vs merge-base with "+baseRef+"):", sc.Files, sc.FilesTruncated)
writeList("Uncommitted working-tree changes:", sc.Uncommitted, sc.UncommittedTruncated)
b.WriteString("\n\nOnly the files listed above are in scope. Findings that point anywhere else are out of scope — discard them.")
// The discard rule must match what was actually rendered: with no file
// lists there is nothing to gate on, and with truncated lists files
// beyond the cap are genuinely in scope — a categorical discard order
// would drop their findings before the judge could rescue them.
hasFileLists := len(sc.Files) > 0 || len(sc.Uncommitted) > 0
listsTruncated := sc.FilesTruncated || sc.UncommittedTruncated
switch {
case !hasFileLists:
// No gate to state.
case listsTruncated:
b.WriteString("\n\nThe file lists above are truncated. Prefer findings in the listed files; verify any finding outside them against `git diff` before keeping it.")
default:
b.WriteString("\n\nOnly the files listed above are in scope. Findings that point anywhere else are out of scope — discard them.")
}
switch {
case sc.Diff != "":
Mcmd/entire/cli/review/prompt.go+14/-1
337 unmodified lines
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
337 unmodified lines
t.Errorf("closing fence must match the longer opening fence; got:\n%s", got)
}
}
func TestComposeReviewPrompt_TruncatedFilesSoftensDiscardRule(t *testing.T) {
t.Parallel()
// Files beyond the cap are genuinely in scope; a categorical discard
// order would drop their findings before the judge could rescue them.
cfg := reviewtypes.RunConfig{
Skills: []string{"/x"},
ScopeBaseRef: "main",
ScopeContext: reviewtypes.ScopeContext{
Files: []string{"M\ta.go"},
FilesTruncated: true,
},
}
got := ComposeReviewPrompt(cfg)
if strings.Contains(got, "discard them") {
t.Errorf("truncated list must not carry the categorical discard rule:\n%s", got)
}
if !strings.Contains(got, "verify") {
t.Errorf("truncated list should ask for verification of outside findings:\n%s", got)
}
}
func TestComposeReviewPrompt_NoFilesNoDiscardRule(t *testing.T) {
t.Parallel()
// Commits with a net-zero three-dot diff (e.g. commit + revert) have no
// file list; rendering "only the files listed above" with no list would
// declare everything out of scope.
cfg := reviewtypes.RunConfig{
Skills: []string{"/x"},
ScopeBaseRef: "main",
ScopeContext: reviewtypes.ScopeContext{
Commits: []string{"abc1234 add then revert"},
},
}
got := ComposeReviewPrompt(cfg)
if strings.Contains(got, "out of scope") {
t.Errorf("no file list rendered — discard rule must be omitted:\n%s", got)
}
}
Mcmd/entire/cli/review/prompt_test.go+39
314 unmodified lines
315
316
317
318
319
320
321
322
323
324
325
321
326
327
328
329
2 unmodified lines
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
314 unmodified lines
if err != nil {
return sc, fmt.Errorf("compute scope diff: %w", err)
}
// The rendered lists ship in the same prompt (argv + ENTIRE_REVIEW_PROMPT
// env var), so they are charged against the inline budget before the
// diff: individually-capped sections could otherwise sum past the ~32KiB
// platform limits the budget exists for.
allowance := caps.diffInlineLimit - scopeListBytes(sc)
switch {
case strings.TrimSpace(diffOut) == "":
// No committed diff — nothing to inline, nothing omitted.
case caps.diffInlineLimit > 0 && len(diffOut) > caps.diffInlineLimit:
case caps.diffInlineLimit > 0 && len(diffOut) > allowance:
sc.DiffOmitted = true
default:
sc.Diff = diffOut
2 unmodified lines
return sc, nil
}
// scopeListBytes returns the bytes the rendered list sections will occupy
// in the composed prompt (one newline per line), for charging against the
// inline-diff budget.
func scopeListBytes(sc reviewtypes.ScopeContext) int {
total := 0
for _, group := range [][]string{sc.Commits, sc.Files, sc.Uncommitted} {
for _, line := range group {
total += len(line) + 1
}
}
return total
}
// capScopeLines splits command output into lines and applies a cap. Only
// trailing newlines are trimmed — porcelain status lines carry a significant
// leading space. maxLines <= 0 means unlimited.
Mcmd/entire/cli/review/scope.go+19/-1
640 unmodified lines
641
642
643
644
645
646
647
648
649
650
651
652
653
654
655
656
657
658
659
660
661
662
663
664
665
666
667
668
669
670
671
672
673
674
675
676
677
678
679
680
681
682
683
684
685
686
687
688
689
690
691
692
640 unmodified lines
t.Errorf("Uncommitted = %d, want 0", stats.Uncommitted)
}
}
// TestBuildScopeContext_UncommittedOnlyNoDiffOmitted verifies a branch with
// only working-tree changes yields Diff=="" and DiffOmitted==false — the
// prompt must not claim "too large to inline" for a diff that outputs nothing.
func TestBuildScopeContext_UncommittedOnlyNoDiffOmitted(t *testing.T) {
dir := t.TempDir()
initRepoOnMain(t, dir)
commitFile(t, dir, "main.go", "package main", "init")
testutil.GitCheckoutNewBranch(t, dir, "feat/dirty-only")
testutil.WriteFile(t, dir, "wip.go", "package wip")
sc, err := BuildScopeContext(context.Background(), dir, defaultBranchName)
if err != nil {
t.Fatalf("BuildScopeContext: %v", err)
}
if sc.Diff != "" || sc.DiffOmitted {
t.Errorf("Diff=%q DiffOmitted=%v, want empty diff without omission flag", sc.Diff, sc.DiffOmitted)
}
if len(sc.Uncommitted) != 1 {
t.Errorf("Uncommitted = %v, want the wip file", sc.Uncommitted)
}
}
// TestBuildScopeContext_ListsCountAgainstDiffBudget pins the total-size
// contract: the inline-diff allowance shrinks by the bytes the rendered
// lists already consume, so the composed prompt (argv + ENTIRE_REVIEW_PROMPT
// env var) stays bounded on platforms with ~32KiB limits.
func TestBuildScopeContext_ListsCountAgainstDiffBudget(t *testing.T) {
dir := t.TempDir()
initRepoOnMain(t, dir)
commitFile(t, dir, "main.go", "package main", "init")
testutil.GitCheckoutNewBranch(t, dir, "feat/budget")
commitFile(t, dir, "a.go", "package a", "add a")
// Budget generous enough for the diff alone, but the commit/file/porcelain
// list bytes must be charged against it first, leaving no room.
sc, err := buildScopeContextCapped(context.Background(), dir, defaultBranchName, scopeContextCaps{
maxCommits: 50,
maxFiles: 200,
maxUncommitted: 100,
diffInlineLimit: 40, // smaller than the rendered lists
})
if err != nil {
t.Fatalf("buildScopeContextCapped: %v", err)
}
if sc.Diff != "" || !sc.DiffOmitted {
t.Errorf("Diff=%q DiffOmitted=%v, want diff omitted once lists consume the budget", sc.Diff, sc.DiffOmitted)
}
}
}
Mcmd/entire/cli/review/scope_test.go+49