Skip to content

feat(session): versioned run manifest + partial-coverage contract #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chethanuk wants to merge 7 commits into
base: main
Choose a base branch
from
+2,145 −53
Open

feat(session): versioned run manifest + partial-coverage contract #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c8b7f35
feat(session): run manifest record, terminal-state computation, cover…
chethanuk Jul 15, 2026
9411ee4
feat(agent): selected set, artifact checksum, cancellation marking
chethanuk Jul 15, 2026
a6ff4a1
feat(cmd): populate run metadata (version, provider, hashes, repo ide…
chethanuk Jul 15, 2026
9ba43ff
feat(output): expose manifest in --format json
chethanuk Jul 15, 2026
7c9c476
feat(scan): manifest parity for full scans
chethanuk Jul 15, 2026
ff745d4
feat(review): --waive for resumed runs
chethanuk Jul 15, 2026
dbe57fa
chore(tests): acceptance matrix + provider-transition + secret guard;…
chethanuk Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions cmd/opencodereview/emit_run_result_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/open-code-review/open-code-review/internal/agent"
"github.com/open-code-review/open-code-review/internal/model"
"github.com/open-code-review/open-code-review/internal/session"
)

type mockResultProvider struct {
Expand All @@ -27,6 +28,7 @@ type mockResultProvider struct {
toolCalls map[string]int64
resumeInfo *agent.ResumeInfo
sessionID string
manifest *session.RunManifest
}

func (m *mockResultProvider) Diffs() []model.Diff { return m.diffs }
Expand All @@ -41,6 +43,7 @@ func (m *mockResultProvider) ProjectSummary() string { return m.projectS
func (m *mockResultProvider) ToolCalls() map[string]int64 { return m.toolCalls }
func (m *mockResultProvider) ResumeInfo() *agent.ResumeInfo { return m.resumeInfo }
func (m *mockResultProvider) SessionID() string { return m.sessionID }
func (m *mockResultProvider) Manifest() *session.RunManifest { return m.manifest }

func TestEmitRunResult_JSONNoFiles(t *testing.T) {
ag := &mockResultProvider{filesReviewed: 0}
Expand Down Expand Up @@ -280,6 +283,91 @@ func TestEmitRunResult_JSONNoFilesTraceID(t *testing.T) {
}
}

func TestEmitRunResult_JSONIncludesManifest(t *testing.T) {
ag := &mockResultProvider{
filesReviewed: 3,
inputTokens: 10,
outputTokens: 5,
totalTokens: 15,
manifest: &session.RunManifest{
SchemaVersion: session.ManifestSchemaVersion,
SessionID: "sess-1",
State: session.StatePartial,
Files: session.ManifestFiles{
Selected: []string{"a.go", "b.go"},
Completed: []string{"a.go"},
Failed: []string{"b.go"},
},
},
}
got := captureStdout(t, func() {
if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil {
t.Fatalf("unexpected error: %v", err)
}
})
var out jsonOutput
if err := json.Unmarshal([]byte(got), &out); err != nil {
t.Fatalf("unmarshal: %v", err)
}
if out.Manifest == nil {
t.Fatal("manifest missing from JSON output")
}
if out.Manifest.State != session.StatePartial {
t.Errorf("manifest.state = %q, want partial", out.Manifest.State)
}
if len(out.Manifest.Files.Selected) != 2 {
t.Errorf("manifest selected = %v", out.Manifest.Files.Selected)
}
// Legacy status must remain unchanged (manifest.state is the NEW contract).
if out.Status != "success" {
t.Errorf("legacy status = %q, want success (unchanged by manifest)", out.Status)
}
}

// TestLegacyStatusValuesUnchanged locks the legacy `status` field to its
// pre-manifest values across the success/warning/error/no-files paths, and
// asserts every path still carries the manifest. manifest.state is the new
// machine contract; status must not drift.
func TestLegacyStatusValuesUnchanged(t *testing.T) {
tests := []struct {
name string
filesReviewed int64
warnings []agent.AgentWarning
manifestState string
wantStatus string
wantManifestState string
}{
{"clean success", 2, nil, session.StatePartial, "success", session.StatePartial},
{"subtask errors", 2, []agent.AgentWarning{{Type: "subtask_error", File: "a.go", Message: "boom"}}, session.StatePartial, "completed_with_errors", session.StatePartial},
{"non-error warnings", 2, []agent.AgentWarning{{Type: "token_threshold_exceeded", Message: "big"}}, session.StatePartial, "completed_with_warnings", session.StatePartial},
{"no files skipped", 0, nil, session.StateSkipped, "skipped", session.StateSkipped},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ag := &mockResultProvider{
filesReviewed: tt.filesReviewed,
warnings: tt.warnings,
manifest: &session.RunManifest{State: tt.manifestState},
}
got := captureStdout(t, func() {
if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil {
t.Fatalf("err: %v", err)
}
})
var out jsonOutput
if err := json.Unmarshal([]byte(got), &out); err != nil {
t.Fatalf("unmarshal: %v", err)
}
if out.Status != tt.wantStatus {
t.Errorf("status = %q, want %q", out.Status, tt.wantStatus)
}
if out.Manifest == nil || out.Manifest.State != tt.wantManifestState {
t.Errorf("manifest.state should be present (%q) regardless of legacy status, got %+v", tt.wantManifestState, out.Manifest)
}
})
}
}

func TestEmitRunResult_JSONIncludesSessionID(t *testing.T) {
ag := &mockResultProvider{filesReviewed: 1, sessionID: "session-99"}
got := captureStdout(t, func() {
Expand Down
12 changes: 11 additions & 1 deletion cmd/opencodereview/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ type reviewOptions struct {
to string
commit string
resume string
waive string // --waive: comma-separated paths to skip (requires --resume)
excludes string // --exclude: comma-separated gitignore-style patterns
outputFormat string
audience string // --audience: "human" (default) or "agent"
Expand All @@ -128,6 +129,7 @@ func parseReviewFlags(args []string) (reviewOptions, error) {
a.StringVar(&opts.to, "to", "", "target ref to end diff at (e.g., 'feature-branch')")
a.StringVarP(&opts.commit, "commit", "c", "", "single commit hash or tag to review (vs its parent)")
a.StringVar(&opts.resume, "resume", "", "resume from a previous review session id")
a.StringVar(&opts.waive, "waive", "", "comma-separated paths to waive (skip but count as covered); requires --resume")
a.StringVar(&opts.excludes, "exclude", "", "comma-separated gitignore-style patterns to exclude; merged with rule.json excludes")
a.StringVarP(&opts.outputFormat, "format", "f", "text", "output format: text or json")
a.IntVar(&opts.concurrency, "concurrency", 8, "max concurrent file reviews")
Expand Down Expand Up @@ -169,6 +171,13 @@ func parseReviewFlags(args []string) (reviewOptions, error) {
if opts.preview && opts.resume != "" {
return opts, fmt.Errorf("--preview and --resume cannot be used together")
}
// Waiving only makes sense on a resumed run: it deliberately leaves diffs
// unreviewed while still satisfying the coverage contract. There is no
// fingerprint-addressed or config-file waive — paths only, and only with
// --resume. (ponytail: path-scoped waives cover the operator use case.)
if opts.waive != "" && opts.resume == "" {
return opts, fmt.Errorf("--waive requires --resume")
}

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

@codeant-ai codeant-ai Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The help text for --waive has an extra trailing backtick, so CLI usage output shows a malformed flag description. Remove the stray character to avoid confusing users and breaking copy/paste from help output. [typo]

Severity Level: Minor 🧹 
⚠️ Suggestion addresses non-issue; raw string usage intentional.
⚠️ CLI help already displays correct waive flag description.
Steps of Reproduction ✅ 
1. Run the CLI with review help, e.g. `ocr review --help`, which executes `runReview()` in
`cmd/opencodereview/review_cmd.go:21`. This calls `parseReviewFlags()` in
`cmd/opencodereview/flags.go:120`. When `-h`/`--help` is present, `ocrFlagSet.Parse()`
(flags.go:65-76) sets `a.showHelp`, causing `parseReviewFlags()` to return with
`opts.showHelp = true` (flags.go:149-151).

2. `runReview()` sees `opts.showHelp` and calls `printReviewUsage()` in
`cmd/opencodereview/flags.go:204`, which prints a raw multiline string via
`fmt.Println(`...`)` (flags.go:205-259).

3. The help text is defined as a Go raw string literal delimited by backticks: the opening
backtick is after `fmt.Println(` on line 205, and the closing backtick appears at the end
of the `--waive` line on line 259, followed by the closing parenthesis for `fmt.Println`.
At runtime, only the string content is printed; neither the backtick nor the closing `)`
is part of the output.

4. Observing the actual CLI output shows the `--waive` help line rendered correctly as
plain text without any stray backticks or parentheses. The pattern `fmt.Println(`raw
string` )` matches the established usage in this file (e.g., `printConfigUsage()` at
flags.go:299-352), so the suggestion to remove the trailing backtick is addressing a
non-issue and would in fact break the raw string literal.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** cmd/opencodereview/flags.go
**Line:** 259:259
**Comment:**
	*Typo: The help text for `--waive` has an extra trailing backtick, so CLI usage output shows a malformed flag description. Remove the stray character to avoid confusing users and breaking copy/paste from help output.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

}

// --- config subcommand ---
Expand Down
13 changes: 13 additions & 0 deletions cmd/opencodereview/flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,19 @@ func TestParseReviewFlags_PreviewWithResume(t *testing.T) {
}
}

func TestParseReviewFlags_WaiveRequiresResume(t *testing.T) {
if _, err := parseReviewFlags([]string{"--from", "main", "--to", "feature", "--waive", "a.go"}); err == nil {
t.Fatal("expected error for --waive without --resume")
}
opts, err := parseReviewFlags([]string{"--from", "main", "--to", "feature", "--resume", "s1", "--waive", "a.go,b.go"})
if err != nil {
t.Fatalf("--waive with --resume should be valid: %v", err)
}
if opts.waive != "a.go,b.go" {
t.Errorf("waive = %q", opts.waive)
}
}

func TestParseReviewFlags_InvalidAudience(t *testing.T) {
_, err := parseReviewFlags([]string{"--audience", "robot"})
if err == nil {
Expand Down
10 changes: 8 additions & 2 deletions cmd/opencodereview/output.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/open-code-review/open-code-review/internal/agent"
"github.com/open-code-review/open-code-review/internal/model"
"github.com/open-code-review/open-code-review/internal/session"
"github.com/open-code-review/open-code-review/internal/suggestdiff"
)

Expand Down Expand Up @@ -249,6 +250,9 @@ type jsonOutput struct {
ProjectSummary string `json:"project_summary,omitempty"`
Resume *agent.ResumeInfo `json:"resume,omitempty"`
SessionID string `json:"session_id,omitempty"`
// Manifest is the immutable run manifest (coverage + terminal state). It is
// the NEW machine contract; the legacy Status field above is unchanged.
Manifest *session.RunManifest `json:"manifest,omitempty"`
}

func outputJSON(comments []model.LlmComment) error {
Expand All @@ -266,7 +270,7 @@ func outputJSON(comments []model.LlmComment) error {

func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentWarning,
filesReviewed, inputTokens, outputTokens, totalTokens, cacheReadTokens, cacheWriteTokens int64,
duration time.Duration, projectSummary string, toolCalls map[string]int64, traceID string, resumeInfo *agent.ResumeInfo, sessionID string) error {
duration time.Duration, projectSummary string, toolCalls map[string]int64, traceID string, resumeInfo *agent.ResumeInfo, sessionID string, manifest *session.RunManifest) error {
out := jsonOutput{
Status: "success",
TraceID: traceID,
Expand All @@ -284,6 +288,7 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW
ProjectSummary: projectSummary,
Resume: resumeInfo,
SessionID: sessionID,
Manifest: manifest,
}
var total int64
for _, v := range toolCalls {
Expand Down Expand Up @@ -317,7 +322,7 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW
return enc.Encode(out)
}

func outputJSONNoFiles(traceID string) error {
func outputJSONNoFiles(traceID string, manifest *session.RunManifest) error {
out := jsonOutput{
Status: "skipped",
TraceID: traceID,
Expand All @@ -326,6 +331,7 @@ func outputJSONNoFiles(traceID string) error {
ToolCalls: &jsonToolCalls{
ByTool: map[string]int64{},
},
Manifest: manifest,
}
enc := json.NewEncoder(os.Stdout)
enc.SetIndent("", " ")
Expand Down
8 changes: 4 additions & 4 deletions cmd/opencodereview/output_helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func TestOutputJSONWithWarnings_NoCommentsSubtaskError(t *testing.T) {
os.Stdout = w

warnings := []agent.AgentWarning{{Type: "subtask_error", File: "x.go", Message: "fail"}}
err := outputJSONWithWarnings(nil, warnings, 1, 10, 5, 15, 0, 0, time.Second, "", nil, "abc123trace", nil, "")
err := outputJSONWithWarnings(nil, warnings, 1, 10, 5, 15, 0, 0, time.Second, "", nil, "abc123trace", nil, "", nil)
_ = w.Close()
os.Stdout = old

Expand Down Expand Up @@ -281,7 +281,7 @@ func TestOutputJSONWithWarnings(t *testing.T) {

comments := []model.LlmComment{{Path: "b.go", Content: "test"}}
warnings := []agent.AgentWarning{{Type: "subtask_error", File: "c.go", Message: "failed"}}
err := outputJSONWithWarnings(comments, warnings, 5, 100, 50, 150, 10, 5, 3*time.Second, "summary", map[string]int64{"file_read": 3}, "trace-xyz-789", nil, "")
err := outputJSONWithWarnings(comments, warnings, 5, 100, 50, 150, 10, 5, 3*time.Second, "summary", map[string]int64{"file_read": 3}, "trace-xyz-789", nil, "", nil)
_ = w.Close()
os.Stdout = old

Expand Down Expand Up @@ -319,7 +319,7 @@ func TestOutputJSONWithWarnings_NoCommentsNoErrors(t *testing.T) {
os.Stdout = w

warnings := []agent.AgentWarning{{Type: "warning", Message: "something"}}
err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 0, 0, time.Second, "", nil, "", nil, "")
err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 0, 0, time.Second, "", nil, "", nil, "", nil)
_ = w.Close()
os.Stdout = old

Expand Down Expand Up @@ -347,7 +347,7 @@ func TestOutputJSONNoFiles(t *testing.T) {
r, w, _ := os.Pipe()
os.Stdout = w

err := outputJSONNoFiles("test-trace-id-456")
err := outputJSONNoFiles("test-trace-id-456", nil)

_ = w.Close()
os.Stdout = old
Expand Down
9 changes: 9 additions & 0 deletions cmd/opencodereview/review_cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ func runReview(args []string) error {
rt.PlanToolDefs = append(rt.PlanToolDefs, mcpToolDefs...)
rt.MainToolDefs = append(rt.MainToolDefs, mcpToolDefs...)

var lang string
if rt.AppCfg != nil {
lang = rt.AppCfg.Language
}
runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, cc.Resolver, opts.concurrency,
resolveRange(cc.RepoDir, opts.from, opts.to, opts.commit))

ag := agent.New(agent.Args{
RepoDir: cc.RepoDir,
From: opts.from,
Expand All @@ -119,6 +126,8 @@ func runReview(args []string) error {
Background: opts.background,
GitRunner: cc.GitRunner,
Resume: resumeState,
RunMeta: runMeta,
WaivePaths: splitPaths(opts.waive),
})

// Silence progress output during execution; restored before the trace
Expand Down
Loading