Skip to content

Commit 29b6637

Browse files
committed
Harden reviewer branch parity checks
1 parent 4d3f425 commit 29b6637

8 files changed

Lines changed: 344 additions & 0 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
# Decision: Scope is reviewer branch-parity discipline: add full-diff first-pass behavior, f
2+
3+
Status: proposed
4+
By: smart
5+
6+
Scope is reviewer branch-parity discipline: add full-diff first-pass behavior, finding-scoped surrounding-function/sibling-branch verification, explicit branch checklist output, ordered reviewer config documentation, and fixture coverage for a sibling branch missing a guard.
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# reviewer branch parity discipline
2+
3+
status: archived
4+
previous_status: active
5+
archived_at: 2026-06-16T10:52:57Z
6+
7+
Summary:
8+
- decisions: 1
9+
- handoffs: 4
10+
- verification records: 1
11+
- run activity records: 4
12+
- findings open: 0
13+
- findings fixed: 0
14+
- findings verified: 1
15+
16+
## Scope
17+
18+
Recent handoffs:
19+
- Fixed reviewer full-diff packet untracked-file coverage. Reviewer prompts and local AI reviewer input now include bounded untracked-file diff content plus stat/list context. Added focused tests proving untracked source files appear in both ...
20+
- Fix the open medium finding: reviewer full-diff packet omits untracked files. The new reviewer/local AI diff helpers use `git diff --no-ext-diff HEAD`, which excludes untracked additions. Preserve surgical scope. Include untracked files ...
21+
22+
Decisions:
23+
- Scope is reviewer branch-parity discipline: add full-diff first-pass behavior, finding-scoped surrounding-function/sibling-branch verification, explicit branch checklist output, ordered reviewer config documentation, and fixture coverage for a sibling branch missing a guard.
24+
25+
Verification:
26+
- Reviewed working diff, task chunks, fixed finding evidence, reviewer/local reviewer prompt paths, untracked diff handling, and tests. Verified fixed finding 2026-06-16T104338Z-reviewer-full-diff-packet-omits-untracked-files.md. No unresolved issues found. ## Command $ [cache env] go test ...
27+
28+
## Final Status
29+
30+
Implemented reviewer branch-parity discipline in reviewer packets and local first-pass review; focused tests, go vet ./..., and GOCACHE-isolated go test ./... pass.
31+
32+
## Reviewer Verification
33+
34+
- Reviewed working diff, task chunks, fixed finding evidence, reviewer/local reviewer prompt paths, untracked diff handling, and tests. Verified fixed finding 2026-06-16T104338Z-reviewer-full-diff-packet-omits-untracked-files.md. No unresolved issues found. ## Command $ [cache env] go test ...
35+
36+
Detailed transient chunks remain available in git history.

README.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,26 @@ openai_compatible` for local chat completion servers. Per-role
355355
`model`/`reasoning` values override the named provider for that role only. Set
356356
`cmd: my-agent {prompt_file}` as an advanced escape hatch, or put provider
357357
flags directly in a custom role command.
358+
359+
For review-heavy projects, prefer an ordered reviewer pipeline: a local coder or
360+
local reviewer first pass for fast branch-parity checks, followed by Codex with
361+
high reasoning for the independent review. Reviewer list entries run in order;
362+
per-entry `model` and `reasoning` overrides apply only to that reviewer:
363+
364+
```yaml
365+
providers:
366+
local-reviewer:
367+
type: openai_compatible
368+
base_url: http://127.0.0.1:1234/v1
369+
model: your-local-review-model
370+
roles:
371+
reviewer:
372+
- local-reviewer
373+
- provider: codex
374+
model: gpt-5.5
375+
reasoning: high
376+
```
377+
358378
Legacy `settings.local_ai_*`, `settings.codex_*`, `settings.claude_*`, and
359379
`settings.cursor_model` keys still work, but new config should prefer
360380
`providers` and `roles.reviewer`.

internal/app/app.go

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6135,12 +6135,14 @@ func buildRolePrompt(e env, state loopState) (string, error) {
61356135
appendPromptDir(&b, e.cwd, state.Task.Path, "handoffs", "Handoffs")
61366136
appendPromptDir(&b, e.cwd, state.Task.Path, "findings", "Findings")
61376137
appendPromptDir(&b, e.cwd, state.Task.Path, "verification", "Verification")
6138+
appendGitDiffPrompt(&b, e.cwd)
61386139
fmt.Fprintln(&b, "Working agreement:")
61396140
if state.NextAction == "review plan before implementation" {
61406141
fmt.Fprintln(&b, "- This is a plan review gate before implementation. Review `plan.md` for readiness; do not review or request implementation changes unless they are needed to make the plan actionable.")
61416142
fmt.Fprintln(&b, "- If the plan is shallow, stale, missing evidence, incorrect, or omits validation or constraints, record findings against the plan so the implementer can fix it before code changes.")
61426143
fmt.Fprintln(&b, "- If the plan is ready, record review evidence. The human must still approve the plan before implementation proceeds.")
61436144
}
6145+
appendReviewerBranchParityDiscipline(&b)
61446146
fmt.Fprintln(&b, "- Review the working tree, tests, and task chunks; verify claims against the code, not the handoff text.")
61456147
fmt.Fprintln(&b, "- Review the current task plan in `plan.md` when present. Check for substantive objective, repo-relative evidence, assumptions or open questions, implementation shape, validation, risks, and constraints; do not require exact section headings.")
61466148
fmt.Fprintln(&b, "- Check whether the implementation and review scope match the approved or draft plan.")
@@ -6170,6 +6172,108 @@ func buildRolePrompt(e env, state loopState) (string, error) {
61706172
return b.String(), nil
61716173
}
61726174

6175+
func appendGitDiffPrompt(b *strings.Builder, cwd string) {
6176+
if stat := gitDiffStat(cwd); strings.TrimSpace(stat) != "" {
6177+
fmt.Fprintf(b, "## Git diff stat\n\n%s\n\n", stat)
6178+
}
6179+
if diff := gitDiff(cwd); strings.TrimSpace(diff) != "" {
6180+
fmt.Fprintf(b, "## Git diff\n\n```diff\n%s\n```\n\n", diff)
6181+
}
6182+
}
6183+
6184+
func gitDiff(cwd string) string {
6185+
out, err := exec.Command("git", "-C", cwd, "diff", "--no-ext-diff", "HEAD").CombinedOutput()
6186+
if err != nil {
6187+
return "(no git diff available)"
6188+
}
6189+
text := strings.TrimSpace(string(out))
6190+
if untracked := gitUntrackedDiff(cwd); strings.TrimSpace(untracked) != "" {
6191+
if text == "" {
6192+
text = untracked
6193+
} else {
6194+
text += "\n\n" + untracked
6195+
}
6196+
}
6197+
if text == "" {
6198+
return "(working tree clean)"
6199+
}
6200+
const maxDiffBytes = 120_000
6201+
if len(text) > maxDiffBytes {
6202+
return text[:maxDiffBytes] + "\n...[diff truncated; inspect the working tree directly before verifying]..."
6203+
}
6204+
return text
6205+
}
6206+
6207+
func gitUntrackedDiff(cwd string) string {
6208+
files := gitUntrackedFiles(cwd)
6209+
if len(files) == 0 {
6210+
return ""
6211+
}
6212+
const (
6213+
maxFiles = 50
6214+
maxFileBytes = 20_000
6215+
maxTotalBytes = 80_000
6216+
)
6217+
var b strings.Builder
6218+
for i, file := range files {
6219+
if i >= maxFiles {
6220+
fmt.Fprintf(&b, "\n...[untracked diff truncated; %d more files]...\n", len(files)-i)
6221+
break
6222+
}
6223+
if b.Len() >= maxTotalBytes {
6224+
fmt.Fprintln(&b, "\n...[untracked diff truncated; inspect the working tree directly before verifying]...")
6225+
break
6226+
}
6227+
data, err := os.ReadFile(filepath.Join(cwd, file))
6228+
if err != nil {
6229+
fmt.Fprintf(&b, "diff --git a/%s b/%s\nnew file mode 100644\n--- /dev/null\n+++ b/%s\n@@\n+[untracked file unreadable: %v]\n", file, file, file, err)
6230+
continue
6231+
}
6232+
if strings.ContainsRune(string(data), '\x00') {
6233+
fmt.Fprintf(&b, "diff --git a/%s b/%s\nnew file mode 100644\n--- /dev/null\n+++ b/%s\n@@\n+[binary untracked file omitted]\n", file, file, file)
6234+
continue
6235+
}
6236+
text := string(data)
6237+
truncated := false
6238+
if len(text) > maxFileBytes {
6239+
text = text[:maxFileBytes]
6240+
truncated = true
6241+
}
6242+
fmt.Fprintf(&b, "diff --git a/%s b/%s\nnew file mode 100644\n--- /dev/null\n+++ b/%s\n@@\n", file, file, file)
6243+
for _, line := range strings.Split(strings.TrimRight(text, "\n"), "\n") {
6244+
fmt.Fprintf(&b, "+%s\n", line)
6245+
}
6246+
if truncated {
6247+
fmt.Fprintln(&b, "+...[untracked file truncated; inspect the working tree directly before verifying]...")
6248+
}
6249+
}
6250+
return strings.TrimRight(b.String(), "\n")
6251+
}
6252+
6253+
func gitUntrackedFiles(cwd string) []string {
6254+
out, err := exec.Command("git", "-C", cwd, "ls-files", "--others", "--exclude-standard", "-z").Output()
6255+
if err != nil || len(out) == 0 {
6256+
return nil
6257+
}
6258+
parts := strings.Split(strings.TrimRight(string(out), "\x00"), "\x00")
6259+
files := make([]string, 0, len(parts))
6260+
for _, part := range parts {
6261+
if part != "" {
6262+
files = append(files, part)
6263+
}
6264+
}
6265+
return files
6266+
}
6267+
6268+
func appendReviewerBranchParityDiscipline(b *strings.Builder) {
6269+
fmt.Fprintln(b, "- Start every reviewer run with a full-diff review before considering seeded findings fixed or verified.")
6270+
fmt.Fprintln(b, "- Enumerate the changed functions and their sibling branches or paths, including success, error, early-return, 404, fallback, read counterpart, and write counterpart paths when they exist.")
6271+
fmt.Fprintln(b, "- Check branch parity across those sibling paths: shared guards, validation, persistence, cleanup, user-visible errors, task-state writes, and tests should be intentionally consistent or intentionally different.")
6272+
fmt.Fprintln(b, "- When verifying a provided finding, inspect the whole enclosing function and sibling branches before emitting `agents finding verify`; open new findings for any parity gap you discover.")
6273+
fmt.Fprintln(b, "- Record one finding per real issue independent of seeded findings; do not merge unrelated parity problems into a single finding and do not pass because only the seeded finding was fixed.")
6274+
fmt.Fprintln(b, "- Include an explicit branch checklist in your response naming each changed function and sibling path you reviewed, with the parity result for each.")
6275+
}
6276+
61736277
func applyRoleOutputDirectives(e env, actor, output string) (string, error) {
61746278
cleaned := output
61756279
seenHandoffs := map[string]bool{}

internal/app/app_test.go

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"fmt"
99
"io"
1010
"os"
11+
"os/exec"
1112
"path/filepath"
1213
"strings"
1314
"sync"
@@ -4308,6 +4309,146 @@ func TestReviewerPromptIncludesPlanReviewInstructionsAndOmitsLocalAIReviewNotes(
43084309
}
43094310
}
43104311

4312+
func TestReviewerPromptIncludesBranchParityChecklistAndFullDiff(t *testing.T) {
4313+
code := withTempProcessEnv(t, func(home, cwd string) int {
4314+
e := env{cwd: cwd, home: home, trustPath: filepath.Join(home, ".agents", "trust.json")}
4315+
task, err := openTask(e, []string{"Branch Parity"}, time.Date(2026, 6, 5, 10, 0, 0, 0, time.UTC))
4316+
if err != nil {
4317+
t.Fatal(err)
4318+
}
4319+
if _, err := addHandoff(e, []string{"--to", "reviewer", "Review sibling command branches; add branch checklist."}, time.Date(2026, 6, 5, 10, 1, 0, 0, time.UTC)); err != nil {
4320+
t.Fatal(err)
4321+
}
4322+
writeReviewerParityFixture(t, cwd)
4323+
if err := os.WriteFile(filepath.Join(cwd, "new_command.go"), []byte(strings.Join([]string{
4324+
"package fixture",
4325+
"",
4326+
"func newCommand() error {",
4327+
" return nil",
4328+
"}",
4329+
"",
4330+
}, "\n")), 0o644); err != nil {
4331+
t.Fatal(err)
4332+
}
4333+
4334+
prompt, err := buildRolePrompt(e, loopState{
4335+
Task: task,
4336+
NextRole: "reviewer",
4337+
NextAction: "review latest handoff",
4338+
})
4339+
if err != nil {
4340+
t.Fatal(err)
4341+
}
4342+
for _, want := range []string{
4343+
"## Git diff",
4344+
"func runCommand(name string, trusted bool) error",
4345+
`case "write":`,
4346+
`case "read":`,
4347+
"Untracked files:",
4348+
"new_command.go",
4349+
"diff --git a/new_command.go b/new_command.go",
4350+
"func newCommand() error",
4351+
"Start every reviewer run with a full-diff review",
4352+
"Enumerate the changed functions and their sibling branches or paths",
4353+
"success, error, early-return, 404, fallback, read counterpart, and write counterpart paths",
4354+
"When verifying a provided finding, inspect the whole enclosing function and sibling branches",
4355+
"Include an explicit branch checklist",
4356+
} {
4357+
if !strings.Contains(prompt, want) {
4358+
t.Fatalf("reviewer prompt missing %q:\n%s", want, prompt)
4359+
}
4360+
}
4361+
return 0
4362+
})
4363+
if code != 0 {
4364+
t.Fatalf("withTempProcessEnv code=%d", code)
4365+
}
4366+
}
4367+
4368+
func TestLocalAIReviewInputIncludesUntrackedFiles(t *testing.T) {
4369+
code := withTempProcessEnv(t, func(home, cwd string) int {
4370+
e := env{cwd: cwd, home: home, trustPath: filepath.Join(home, ".agents", "trust.json")}
4371+
task, err := openTask(e, []string{"Local Reviewer Untracked"}, time.Date(2026, 6, 5, 10, 0, 0, 0, time.UTC))
4372+
if err != nil {
4373+
t.Fatal(err)
4374+
}
4375+
writeReviewerParityFixture(t, cwd)
4376+
if err := os.WriteFile(filepath.Join(cwd, "untracked_review.go"), []byte(strings.Join([]string{
4377+
"package fixture",
4378+
"",
4379+
"func untrackedReviewPath() error {",
4380+
" return nil",
4381+
"}",
4382+
"",
4383+
}, "\n")), 0o644); err != nil {
4384+
t.Fatal(err)
4385+
}
4386+
4387+
input := localAIReviewInput(e, loopState{
4388+
Task: task,
4389+
NextAction: "review latest handoff",
4390+
})
4391+
for _, want := range []string{
4392+
"Git diff stat:",
4393+
"Untracked files:",
4394+
"untracked_review.go",
4395+
"Git diff:",
4396+
"diff --git a/untracked_review.go b/untracked_review.go",
4397+
"func untrackedReviewPath() error",
4398+
} {
4399+
if !strings.Contains(input, want) {
4400+
t.Fatalf("local AI reviewer input missing %q:\n%s", want, input)
4401+
}
4402+
}
4403+
return 0
4404+
})
4405+
if code != 0 {
4406+
t.Fatalf("withTempProcessEnv code=%d", code)
4407+
}
4408+
}
4409+
4410+
func writeReviewerParityFixture(t *testing.T, cwd string) {
4411+
t.Helper()
4412+
runGit(t, cwd, "init")
4413+
runGit(t, cwd, "config", "user.email", "agents@example.test")
4414+
runGit(t, cwd, "config", "user.name", "Agents Test")
4415+
before := strings.Join([]string{
4416+
"package fixture",
4417+
"",
4418+
"func runCommand(name string, trusted bool) error {",
4419+
" switch name {",
4420+
` case "write":`,
4421+
" return writeData()",
4422+
` case "read":`,
4423+
" return readData()",
4424+
" default:",
4425+
" return nil",
4426+
" }",
4427+
"}",
4428+
"",
4429+
"func writeData() error { return nil }",
4430+
"func readData() error { return nil }",
4431+
"",
4432+
}, "\n")
4433+
if err := os.WriteFile(filepath.Join(cwd, "commands.go"), []byte(before), 0o644); err != nil {
4434+
t.Fatal(err)
4435+
}
4436+
runGit(t, cwd, "add", "commands.go")
4437+
runGit(t, cwd, "commit", "-m", "base")
4438+
after := strings.Replace(before, "\t\treturn writeData()", "\t\tif !trusted {\n\t\t\treturn nil\n\t\t}\n\t\treturn writeData()", 1)
4439+
if err := os.WriteFile(filepath.Join(cwd, "commands.go"), []byte(after), 0o644); err != nil {
4440+
t.Fatal(err)
4441+
}
4442+
}
4443+
4444+
func runGit(t *testing.T, cwd string, args ...string) {
4445+
t.Helper()
4446+
cmd := exec.Command("git", append([]string{"-C", cwd}, args...)...)
4447+
if out, err := cmd.CombinedOutput(); err != nil {
4448+
t.Fatalf("git %v failed: %v\n%s", args, err, out)
4449+
}
4450+
}
4451+
43114452
func TestLocalAIFirstPassFindingsSkipConfiguredReviewer(t *testing.T) {
43124453
old := runFirstPassReviewWithLocalAI
43134454
defer func() { runFirstPassReviewWithLocalAI = old }()

internal/app/local_ai.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,8 @@ func runLocalAIReviewerItem(e env, state loopState, rc roleConfig, settings agen
344344
func runFirstPassReviewWithOpenAICompat(settings agentSettings, input string) (localAIReviewResult, error) {
345345
content, err := localAIChat(settings,
346346
"You are a reviewer in an ordered Agents review pipeline. Review only the provided task context. "+
347+
"Start from the full diff, enumerate changed functions and sibling branches/paths, check parity across success/error/early-return/404/fallback and read/write counterparts, and include that branch checklist in the summary. "+
348+
"When reviewing a provided finding, inspect the enclosing function and sibling branches before passing; return fail with one finding per real issue for any parity gap independent of seeded findings. "+
347349
"Return only JSON: {\"result\":\"pass|fail|escalate\",\"summary\":\"...\",\"findings\":[{\"severity\":\"low|medium|high\",\"text\":\"...\",\"evidence\":\"...\"}]}. "+
348350
"Use fail only for concrete, grounded issues. Use pass when there are no blocking issues. Use escalate when the context is insufficient or uncertain.",
349351
strings.TrimSpace(input),
@@ -364,6 +366,14 @@ func localAIReviewInput(e env, state loopState) string {
364366
if stat := gitDiffStat(e.cwd); strings.TrimSpace(stat) != "" {
365367
fmt.Fprintf(&b, "\nGit diff stat:\n%s\n", stat)
366368
}
369+
if diff := gitDiff(e.cwd); strings.TrimSpace(diff) != "" {
370+
fmt.Fprintf(&b, "\nGit diff:\n%s\n", diff)
371+
}
372+
fmt.Fprintln(&b, "\nReviewer branch checklist requirements:")
373+
fmt.Fprintln(&b, "- Start from the full diff before judging seeded findings.")
374+
fmt.Fprintln(&b, "- Enumerate changed functions and sibling branches/paths.")
375+
fmt.Fprintln(&b, "- Check parity across success, error, early-return, 404, fallback, read counterpart, and write counterpart paths.")
376+
fmt.Fprintln(&b, "- Record one finding per real issue independent of seeded findings.")
367377
return b.String()
368378
}
369379

internal/app/local_ai_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"net"
66
"net/http"
77
"net/http/httptest"
8+
"strings"
89
"testing"
910
)
1011

@@ -180,6 +181,16 @@ func TestRunFirstPassReviewWithOpenAICompat(t *testing.T) {
180181
if req.Model != "google/gemma-3-4b" || len(req.Messages) != 2 || req.Messages[1].Content != "review context" {
181182
t.Fatalf("unexpected request: %+v", req)
182183
}
184+
for _, want := range []string{
185+
"Start from the full diff",
186+
"enumerate changed functions and sibling branches/paths",
187+
"success/error/early-return/404/fallback and read/write counterparts",
188+
"one finding per real issue",
189+
} {
190+
if !strings.Contains(req.Messages[0].Content, want) {
191+
t.Fatalf("review system prompt missing %q:\n%s", want, req.Messages[0].Content)
192+
}
193+
}
183194
_, _ = w.Write([]byte(`{"choices":[{"message":{"content":"{\"result\":\"pass\",\"summary\":\"clean\",\"findings\":[]}"}}]}`))
184195
}))
185196
defer server.Close()

0 commit comments

Comments
 (0)