fix(review): address trail + CI findings on scope context and allowlist · Entire

fix(review): address trail + CI findings on scope context and allowlist

06c598f·

peyton-alt·1w ago·8 files·+75 added/-15 removed

Four review findings and one lint failure, all confirmed real:

Commit cap kept the OLDEST commits (first lines of --reverse output) and silently dropped everything near HEAD, while the prompt forbids re-deriving scope. Cap newest-first, then reverse the kept slice for reading order — recent commits are the ones a review can least afford to lose. (Cursor Bugbot, high)
Inline diff budget 48KiB could exceed Windows' ~32KiB CreateProcess command-line cap (prompt travels as argv and ENTIRE_REVIEW_PROMPT). Lowered to 24KiB. (Copilot)
A diff containing ``` (any markdown-touching change) closed the diff fence early, letting diff content escape into instruction position. Fence is now one backtick longer than the longest run in the diff. (Copilot)
Bash(git branch:*) allowed mutating -d/-D/-m; removed from the read-only allowlist. (Copilot)
guidedProfileTask's profileName parameter became unused when the built-in fallback was removed; parameter dropped. (revive)

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

Sessions

320d86619050View transcript

Changes

cmd/entire/cli
- agent/claudecode
  - Mreviewer.go+1/-1
  - Mreviewer_test.go+1/-1
- review
  - Mpicker.go+2/-2
  - Mpicker_internal_test.go+4/-4
  - Mprompt.go+26/-2
  - Mprompt_test.go+22
  - Mscope.go+13/-5
  - Mscope_test.go+6

50 unmodified lines
51
52
53
54
54
55
56
57

50 unmodified lines

"Read", "Grep", "Glob", "Task", "TodoWrite", "Skill",
    "Bash(git diff:*)", "Bash(git log:*)", "Bash(git show:*)",
    "Bash(git status:*)", "Bash(git blame:*)", "Bash(git rev-parse:*)",
    "Bash(git merge-base:*)", "Bash(git ls-files:*)", "Bash(git branch:*)",
    "Bash(git merge-base:*)", "Bash(git ls-files:*)",
    "Bash(entire search:*)", "Bash(entire checkpoint explain:*)",
}

Mcmd/entire/cli/agent/claudecode/reviewer.go+1/-1

437 unmodified lines
438
439
440
441
441
442
443
444

437 unmodified lines

t.Errorf("allowlist missing %q; got %q", want, allowed)
        }
    }
    for _, banned := range []string{"Edit", "Write", "MultiEdit", "NotebookEdit", "Bash", "Bash(git:*)"} {
    for _, banned := range []string{"Edit", "Write", "MultiEdit", "NotebookEdit", "Bash", "Bash(git:*)", "Bash(git branch:*)"} {
        if members[banned] {
            t.Errorf("allowlist must not grant write-capable or blanket tool %q; got %q", banned, allowed)
        }

Mcmd/entire/cli/agent/claudecode/reviewer_test.go+1/-1

135 unmodified lines
136
137
138
139
139
140
141
142
18 unmodified lines
161
162
163
164
164
165
166
167

135 unmodified lines

if err != nil {
        return "", settings.ReviewProfileConfig{}, err
    }
    profile.Task = guidedProfileTask(profileName, profile.Task, existing.Task, customTask)
    profile.Task = guidedProfileTask(profile.Task, existing.Task, customTask)
    if len(profile.Agents) > 1 {
        judge, err := promptForJudge(ctx, launchable, existing)
        if err != nil {
18 unmodified lines

// launchableInstalledAgentNames returns the installed agents that have a
// review-runner adapter, in the order they can be offered to the user.
func guidedProfileTask(profileName, generatedTask, existingTask, customTask string) string {
func guidedProfileTask(generatedTask, existingTask, customTask string) string {
    if customTask != "" {
        return customTask
    }

Mcmd/entire/cli/review/picker.go+2/-2

31 unmodified lines
32
33
34
35
35
36
37
38
38
39
40
41
41
42
43
44
28 unmodified lines
73
74
75
76
76
77
78
79

31 unmodified lines

existing  = "saved custom task"
        custom    = "new custom task"
    )
    if got := guidedProfileTask(DefaultProfileName, generated, existing, ""); got != existing {
        if got := guidedProfileTask(generated, existing, ""); got != existing {
            t.Fatalf("guidedProfileTask without new custom task = %q, want existing %q", got, existing)
        }
        if got := guidedProfileTask(DefaultProfileName, generated, existing, custom); got != custom {
        if got := guidedProfileTask(generated, existing, custom); got != custom {
            t.Fatalf("guidedProfileTask with new custom task = %q, want %q", got, custom)
        }
        if got := guidedProfileTask(DefaultProfileName, generated, "", ""); got != generated {
        if got := guidedProfileTask(generated, "", ""); got != generated {
            t.Fatalf("guidedProfileTask without existing task = %q, want generated %q", got, generated)
        }
    }
28 unmodified lines

// skill-bearing workers kept receiving the maximal-audit brief forever.
func TestGuidedProfileTask_NoBuiltinFallbackPersisted(t *testing.T) {
    t.Parallel()
    if got := guidedProfileTask(DefaultProfileName, "", "", ""); got != "" {
    if got := guidedProfileTask("", "", ""); got != "" {
        t.Fatalf("guidedProfileTask with nothing user-provided = %q, want empty", got)
    }
}

Mcmd/entire/cli/review/picker_internal_test.go+4/-4

102 unmodified lines
103
104
105
106
106
107
108
109
110
111
108
112
113
114
115
1 unmodified line
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142

102 unmodified lines

switch {
        case sc.Diff != "":
            b.WriteString("\n\nDiff under review:\n```diff\n")
            // The fence must be longer than any backtick run inside the diff
            // (diffs touching markdown contain ``` lines), or the fence closes
            // early and diff content escapes into instruction position.
            fence := diffFence(sc.Diff)
            b.WriteString("\n\nDiff under review:\n" + fence + "diff\n")
            b.WriteString(strings.TrimRight(sc.Diff, "\n"))
            b.WriteString("\n```")
            b.WriteString("\n" + fence)
        case sc.DiffOmitted:
            b.WriteString("\n\nThe diff is too large to inline. Read it with `git diff " + baseRef + "...HEAD` (three-dot), plus `git status --porcelain` for uncommitted files.")
        }
1 unmodified line

return b.String()
}

// diffFence returns a backtick fence one longer than the longest backtick
// run in the diff (minimum the standard three).
func diffFence(diff string) string {
    longest, run := 0, 0
    for _, r := range diff {
        if r == '`' {
            run++
            if run > longest {
                longest = run
            }
            continue
        }
        run = 0
    }
    if longest < 3 {
        return "```"
    }
    return strings.Repeat("`", longest+1)
}

const reviewerOutputFormatInstructions = `Output format:
- Start with one verdict line: approve / approve with nits / request changes, plus a short reason.
- Then list actionable findings only. Each finding MUST be a separate top-level Markdown bullet starting with [high], [medium], or [low].

Mcmd/entire/cli/review/prompt.go+26/-2

269 unmodified lines
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294

269 unmodified lines

t.Errorf("prompt missing truncation note:\n%s", got)
    }
}

func TestComposeReviewPrompt_DiffWithBackticksKeepsFenceIntact(t *testing.T) {
    t.Parallel()
    // A diff touching markdown can contain triple-backtick fences; a plain ```
    // wrapper would be closed early, corrupting the prompt structure and
    // letting diff content escape into instruction position.
    cfg := reviewtypes.RunConfig{
        Skills:       []string{"
/x"},
        ScopeBaseRef: "main",
        ScopeContext: reviewtypes.ScopeContext{
            Files: []string{"M\tREADME.md"},
            Diff:  "diff --git a/README.md b/README.md\n+```go\n+code\n+```",
        },
    }
    got := ComposeReviewPrompt(cfg)
    if !strings.Contains(got, "````diff") {
        t.Errorf("diff containing ``` must be wrapped in a longer fence; got:\n%s", got)
    }
    if !strings.Contains(got, "\n````") {
        t.Errorf("closing fence must match the longer opening fence; got:\n%s", got)
    }
}
````

Mcmd/entire/cli/review/prompt_test.go+22

```
10 unmodified lines
11
12
13
14
15
16
17
245 unmodified lines
263
264
265
265
266
267
268
266
267
268
269
270
271
272
273
274
275
13 unmodified lines
289
290
291
288
292
293
294
295
296
297
298
299
300
301
302
303

10 unmodified lines

import (
    "context"
    "fmt"
    "slices"
    "strconv"
    "strings"

245 unmodified lines

}

const (
    scopeMaxCommits      = 50
    scopeMaxFiles        = 200
    scopeMaxUncommitted  = 100
    scopeDiffInlineLimit = 48 * 1024
    scopeMaxCommits     = 50
    scopeMaxFiles       = 200
    scopeMaxUncommitted = 100
    // scopeDiffInlineLimit stays well under Windows' ~32KiB CreateProcess
    // command-line cap (the prompt travels as one argv string and again via
    // ENTIRE_REVIEW_PROMPT), leaving headroom for the rest of the prompt.
    scopeDiffInlineLimit = 24 * 1024
}

// BuildScopeContext enumerates the review scope for prompt injection: the
13 unmodified lines

func buildScopeContextCapped(ctx context.Context, repoRoot, baseRef string, caps scopeContextCaps) (reviewtypes.ScopeContext, error) {
    var sc reviewtypes.ScopeContext

commitsOut, err := runGit(ctx, repoRoot, "log", "--reverse", "--format=%h %s", baseRef+"..HEAD")
    // Newest-first from git, cap (keeping the newest — the prompt declares
    // this list authoritative, and recent commits are the ones a review can
    // least afford to lose), then reverse for oldest-first reading order.
    commitsOut, err := runGit(ctx, repoRoot, "log", "--format=%h %s", baseRef+"..HEAD")
    if err != nil {
        return sc, fmt.Errorf("list scope commits: %w", err)
    }
    sc.Commits, sc.CommitsTruncated = capScopeLines(commitsOut, caps.maxCommits)
    slices.Reverse(sc.Commits)

filesOut, err := runGit(ctx, repoRoot, "diff", "--name-status", baseRef+"...HEAD")
    if err != nil {