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

7c7d292·

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

8

• 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

130 unmodified lines

131
132
133
134
134
135
136
137
138
139
136
140
141
142
143
1 unmodified line

145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170

130 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

315 unmodified lines

316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340

315 unmodified lines

t.Errorf("prompt missing non-whole-minute duration:\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````\n") && !strings.HasSuffix(got, "````") && !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 {