Skip to content

Commit ff745d4

Browse files
committed
feat(review): --waive for resumed runs
Add --waive path/a.go,path/b.go (requires --resume): waived diffs are not dispatched, are recorded as review_item_waived, and count toward coverage so a waive can flip partial -> complete. A later resume treats a waived item as covered (indexed like a completed item, empty comments). No fingerprint-addressed or config-file waives — paths only (ponytail note). Tests: waive flips partial->complete, without waive stays partial, --waive without --resume errors.
1 parent 7c9c476 commit ff745d4

6 files changed

Lines changed: 97 additions & 2 deletions

File tree

cmd/opencodereview/flags.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,7 @@ type reviewOptions struct {
102102
to string
103103
commit string
104104
resume string
105+
waive string // --waive: comma-separated paths to skip (requires --resume)
105106
excludes string // --exclude: comma-separated gitignore-style patterns
106107
outputFormat string
107108
audience string // --audience: "human" (default) or "agent"
@@ -128,6 +129,7 @@ func parseReviewFlags(args []string) (reviewOptions, error) {
128129
a.StringVar(&opts.to, "to", "", "target ref to end diff at (e.g., 'feature-branch')")
129130
a.StringVarP(&opts.commit, "commit", "c", "", "single commit hash or tag to review (vs its parent)")
130131
a.StringVar(&opts.resume, "resume", "", "resume from a previous review session id")
132+
a.StringVar(&opts.waive, "waive", "", "comma-separated paths to waive (skip but count as covered); requires --resume")
131133
a.StringVar(&opts.excludes, "exclude", "", "comma-separated gitignore-style patterns to exclude; merged with rule.json excludes")
132134
a.StringVarP(&opts.outputFormat, "format", "f", "text", "output format: text or json")
133135
a.IntVar(&opts.concurrency, "concurrency", 8, "max concurrent file reviews")
@@ -169,6 +171,13 @@ func parseReviewFlags(args []string) (reviewOptions, error) {
169171
if opts.preview && opts.resume != "" {
170172
return opts, fmt.Errorf("--preview and --resume cannot be used together")
171173
}
174+
// Waiving only makes sense on a resumed run: it deliberately leaves diffs
175+
// unreviewed while still satisfying the coverage contract. There is no
176+
// fingerprint-addressed or config-file waive — paths only, and only with
177+
// --resume. (ponytail: path-scoped waives cover the operator use case.)
178+
if opts.waive != "" && opts.resume == "" {
179+
return opts, fmt.Errorf("--waive requires --resume")
180+
}
172181

173182
switch opts.audience {
174183
case "human", "agent":
@@ -246,7 +255,8 @@ Flags:
246255
--rule string path to JSON file with system review rules
247256
--timeout int concurrent task timeout in minutes (default 10)
248257
--to string target ref to end diff at (e.g., 'feature-branch')
249-
--tools string path to JSON tools config file (default: embedded)`)
258+
--tools string path to JSON tools config file (default: embedded)
259+
--waive string comma-separated paths to waive (skip but count as covered); requires --resume`)
250260
}
251261

252262
// --- config subcommand ---

cmd/opencodereview/flags_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,19 @@ func TestParseReviewFlags_PreviewWithResume(t *testing.T) {
5353
}
5454
}
5555

56+
func TestParseReviewFlags_WaiveRequiresResume(t *testing.T) {
57+
if _, err := parseReviewFlags([]string{"--from", "main", "--to", "feature", "--waive", "a.go"}); err == nil {
58+
t.Fatal("expected error for --waive without --resume")
59+
}
60+
opts, err := parseReviewFlags([]string{"--from", "main", "--to", "feature", "--resume", "s1", "--waive", "a.go,b.go"})
61+
if err != nil {
62+
t.Fatalf("--waive with --resume should be valid: %v", err)
63+
}
64+
if opts.waive != "a.go,b.go" {
65+
t.Errorf("waive = %q", opts.waive)
66+
}
67+
}
68+
5669
func TestParseReviewFlags_InvalidAudience(t *testing.T) {
5770
_, err := parseReviewFlags([]string{"--audience", "robot"})
5871
if err == nil {

cmd/opencodereview/review_cmd.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ func runReview(args []string) error {
127127
GitRunner: cc.GitRunner,
128128
Resume: resumeState,
129129
RunMeta: runMeta,
130+
WaivePaths: splitPaths(opts.waive),
130131
})
131132

132133
// Silence progress output during execution; restored before the trace

internal/agent/agent.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,11 @@ type Args struct {
125125
// is folded into the run manifest at Finalize. Ignored when Session is
126126
// supplied pre-built by the caller.
127127
RunMeta session.RunMeta
128+
129+
// WaivePaths lists repo-relative paths to waive on a resumed run: they are
130+
// not dispatched, are recorded as review_item_waived, and count toward
131+
// coverage. Only honored alongside Resume (the CLI enforces --resume).
132+
WaivePaths []string
128133
}
129134

130135
// Agent orchestrates the AI-powered code review. LLM tool-use loop / memory
@@ -481,6 +486,11 @@ func (a *Agent) applyResume(diffs []model.Diff) []model.Diff {
481486
return diffs
482487
}
483488

489+
waived := make(map[string]struct{}, len(a.args.WaivePaths))
490+
for _, p := range a.args.WaivePaths {
491+
waived[p] = struct{}{}
492+
}
493+
484494
mode := a.reviewMode()
485495
toDispatch := make([]model.Diff, 0, len(diffs))
486496
var reused int64
@@ -490,6 +500,12 @@ func (a *Agent) applyResume(diffs []model.Diff) []model.Diff {
490500
continue
491501
}
492502
fingerprint := reviewItemFingerprint(mode, d)
503+
// An explicit waive wins over both reuse and re-review: the operator
504+
// chose to skip this diff while still satisfying coverage.
505+
if _, ok := waived[effectivePath(d)]; ok {
506+
a.session.RecordReviewItemWaived(effectivePath(d), d.OldPath, d.NewPath, fingerprint)
507+
continue
508+
}
493509
item, ok := resume.Item(fingerprint)
494510
if !ok {
495511
toDispatch = append(toDispatch, d)

internal/agent/manifest_test.go

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,59 @@ func TestRunManifest_Scenarios(t *testing.T) {
184184
}
185185
}
186186

187+
// TestWaiveCoverage verifies a waived diff is not dispatched, is recorded as
188+
// covered, and flips an otherwise-partial run to complete.
189+
func TestWaiveCoverage(t *testing.T) {
190+
build := func(waive []string) *session.RunManifest {
191+
sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange})
192+
a := New(Args{
193+
LLMClient: routeClient{},
194+
Model: "test",
195+
Session: sess,
196+
Tools: tool.NewRegistry(),
197+
Template: template.Template{
198+
MaxTokens: 100000,
199+
MaxToolRequestTimes: 5,
200+
MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "{{diff}}"}}},
201+
},
202+
MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done"}}},
203+
// Resume must be non-nil for applyResume (and thus waive) to run.
204+
Resume: &session.ResumeState{SessionID: "prev", Items: map[string]session.ResumeItem{}},
205+
WaivePaths: waive,
206+
})
207+
a.currentDate = "d"
208+
a.diffs = []model.Diff{
209+
{NewPath: "ok.go", OldPath: "ok.go", Diff: "+ok", Insertions: 1},
210+
{NewPath: "flaky.go", OldPath: "flaky.go", Diff: "+DO_FAIL", Insertions: 1},
211+
}
212+
_, _ = a.dispatchSubtasks(context.Background())
213+
sess.Finalize()
214+
return sess.Manifest()
215+
}
216+
217+
// No waive: the failing diff leaves the run partial.
218+
noWaive := build(nil)
219+
if noWaive.State != session.StatePartial {
220+
t.Errorf("without waive: state = %q, want partial (files=%+v)", noWaive.State, noWaive.Files)
221+
}
222+
223+
// Waiving the failing diff flips the run to complete and records it as
224+
// waived rather than dispatched/failed.
225+
waived := build([]string{"flaky.go"})
226+
if waived.State != session.StateComplete {
227+
t.Errorf("with waive: state = %q, want complete (files=%+v)", waived.State, waived.Files)
228+
}
229+
if len(waived.Files.Waived) != 1 || waived.Files.Waived[0] != "flaky.go" {
230+
t.Errorf("waived set = %v, want [flaky.go]", waived.Files.Waived)
231+
}
232+
if len(waived.Files.Failed) != 0 {
233+
t.Errorf("waived diff must not be recorded as failed: %v", waived.Files.Failed)
234+
}
235+
if len(waived.Files.Completed) != 1 || waived.Files.Completed[0] != "ok.go" {
236+
t.Errorf("completed set = %v, want [ok.go]", waived.Files.Completed)
237+
}
238+
}
239+
187240
// TestArtifactChecksumStableAndOrderIndependent verifies the artifact hash is
188241
// deterministic regardless of diff ordering (fingerprints are sorted).
189242
func TestArtifactChecksumStableAndOrderIndependent(t *testing.T) {

internal/session/resume.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,9 @@ func (s *ResumeState) applyResumeLine(line []byte) error {
111111
switch rec.Type {
112112
case "session_start":
113113
s.applySessionStart(rec)
114-
case "review_item_done", "review_item_reused":
114+
case "review_item_done", "review_item_reused", "review_item_waived":
115+
// A waived item counts as covered for later resumes: index it like a
116+
// completed item (empty comments) so a subsequent resume skips it.
115117
if rec.Fingerprint == "" {
116118
return nil
117119
}

0 commit comments

Comments
 (0)