feat(manifest): run manifest coverage contract for review (#367)

cmd/opencodereview/budget_output_test.go

@@ -9,13 +9,17 @@ import (
 
 	"github.com/alibaba/open-code-review/internal/agent"
 	"github.com/alibaba/open-code-review/internal/model"
+	"github.com/alibaba/open-code-review/internal/session"
 )
 
-// TestEmitRunResult_JSONBudgetExceededStatus verifies that a provider signaling
-// BudgetExceeded()==true produces JSON with status=="budget_exceeded" AND
-// summary.budget_exceeded==true (INV-3 typed status), and that it takes
-// precedence over completed_with_warnings.
-func TestEmitRunResult_JSONBudgetExceededStatus(t *testing.T) {
+// TestEmitRunResult_JSONBudgetStopIsPartial pins the unified terminal-state
+// contract for a controlled budget stop that still covered something: the
+// top-level status comes solely from the manifest's coverage-derived
+// terminal_state ("partial"), and budgetExceeded never overrides it. The budget
+// reason stays observable through three independent outlets — summary.
+// budget_exceeded, the token_budget_reached warning, and the failed items'
+// classification — so nothing is lost by dropping the typed status.
+func TestEmitRunResult_JSONBudgetStopIsPartial(t *testing.T) {
 	ag := &mockResultProvider{
 		filesReviewed:  3,
 		inputTokens:    100,
@@ -24,6 +28,18 @@ func TestEmitRunResult_JSONBudgetExceededStatus(t *testing.T) {
 		warnings:       []agent.AgentWarning{{Type: "token_budget_reached", File: "big.go", Message: "stopped"}},
 		toolCalls:      map[string]int64{"file_read": 2},
 		budgetExceeded: true,
+		manifest: &session.RunManifest{
+			TerminalState: session.StatePartial,
+			Coverage: session.Coverage{
+				Selected:  []session.CoverageItem{{ItemID: "a"}, {ItemID: "b"}},
+				Completed: []session.CoverageItem{{ItemID: "a"}},
+				Failed: []session.CoverageItem{{
+					ItemID:         "b",
+					Classification: session.FailureBudget,
+					Reason:         "aggregate token budget reached before dispatch completed",
+				}},
+			},
+		},
 	}
 	got := captureStdout(t, func() {
 		err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil)
@@ -35,14 +51,24 @@ func TestEmitRunResult_JSONBudgetExceededStatus(t *testing.T) {
 	if err := json.Unmarshal([]byte(got), &out); err != nil {
 		t.Fatalf("unmarshal: %v", err)
 	}
-	if out.Status != "budget_exceeded" {
-		t.Errorf("status = %q, want budget_exceeded (must take precedence over completed_with_warnings)", out.Status)
+	if out.Status != string(session.StatePartial) {
+		t.Errorf("status = %q, want %q (must mirror manifest.terminal_state, not a typed budget status)",
+			out.Status, session.StatePartial)
 	}
 	if out.Summary == nil || !out.Summary.BudgetExceeded {
 		t.Errorf("summary.budget_exceeded = %v, want true", out.Summary)
 	}
+	if out.Manifest == nil {
+		t.Fatal("manifest must be published")
+	}
+	if out.Manifest.RunFailure != nil {
+		t.Errorf("a controlled budget stop must record no run_failure, got %+v", out.Manifest.RunFailure)
+	}
+	if len(out.Manifest.Coverage.Failed) != 1 || out.Manifest.Coverage.Failed[0].Classification != session.FailureBudget {
+		t.Errorf("coverage.failed = %+v, want one item classified budget", out.Manifest.Coverage.Failed)
+	}
 	// The token_budget_reached warning must still be present in the output so
-	// the reason is observable alongside the typed status.
+	// the reason is observable alongside the coverage-derived status.
 	var foundBudgetWarn bool
 	for _, w := range out.Warnings {
 		if w.Type == "token_budget_reached" {
@@ -55,11 +81,12 @@ func TestEmitRunResult_JSONBudgetExceededStatus(t *testing.T) {
 	}
 }
 
-// TestEmitRunResult_JSONBudgetExceededPrecedenceOverErrors verifies that
-// budget_exceeded takes precedence over completed_with_errors too — a budget
-// trip is a distinct typed terminal state (INV-3 lists completed_with_errors
-// as a status it must be distinct from).
-func TestEmitRunResult_JSONBudgetExceededPrecedenceOverErrors(t *testing.T) {
+// TestEmitRunResult_JSONBudgetDoesNotOverrideLegacyStatus guards the legacy
+// (manifest-less) path: budgetExceeded must not rewrite the warning-derived
+// status there either. Before the unified contract a budget trip forced
+// status=="budget_exceeded" and masked the fact that a subtask had errored;
+// now the error status survives and the budget shows up only in the summary.
+func TestEmitRunResult_JSONBudgetDoesNotOverrideLegacyStatus(t *testing.T) {
 	ag := &mockResultProvider{
 		filesReviewed:  1,
 		warnings:       []agent.AgentWarning{{Type: "subtask_error", File: "x.go", Message: "boom"}},
@@ -75,8 +102,11 @@ func TestEmitRunResult_JSONBudgetExceededPrecedenceOverErrors(t *testing.T) {
 	if err := json.Unmarshal([]byte(got), &out); err != nil {
 		t.Fatalf("unmarshal: %v", err)
 	}
-	if out.Status != "budget_exceeded" {
-		t.Errorf("status = %q, want budget_exceeded (must take precedence over completed_with_errors)", out.Status)
+	if out.Status != "completed_with_errors" {
+		t.Errorf("status = %q, want completed_with_errors (budgetExceeded must not override it)", out.Status)
+	}
+	if out.Summary == nil || !out.Summary.BudgetExceeded {
+		t.Errorf("summary.budget_exceeded = %v, want true", out.Summary)
 	}
 }
 
@@ -110,7 +140,7 @@ func TestEmitRunResult_JSONNoBudgetIsSuccess(t *testing.T) {
 
 // TestEmitFailureUsage_TextEmitsStructuredRecord verifies the non-budget
 // failure path emits a structured usage record to stderr with the token totals
-// and budget_exceeded=false (INV-4). Text format.
+// and budget_exceeded=false in text format.
 func TestEmitFailureUsage_TextEmitsStructuredRecord(t *testing.T) {
 	ag := &mockResultProvider{
 		filesReviewed: 4,
@@ -131,7 +161,7 @@ func TestEmitFailureUsage_TextEmitsStructuredRecord(t *testing.T) {
 }
 
 // TestEmitFailureUsage_JSONEmitsStructuredRecord verifies the JSON form emits a
-// parseable record to stderr with budget_exceeded=false (INV-4).
+// parseable record to stderr with budget_exceeded=false.
 func TestEmitFailureUsage_JSONEmitsStructuredRecord(t *testing.T) {
 	ag := &mockResultProvider{
 		filesReviewed: 2,

cmd/opencodereview/compat_test.go

@@ -20,6 +20,16 @@ func parseReviewFlags(args []string) (reviewOptions, error) {
 	return opts, err
 }
 
+// runReview provides test compatibility for the pre-cobra runReview(args)
+// entry point: parse flags, then execute the review.
+func runReview(args []string) error {
+	opts, err := parseReviewFlags(args)
+	if err != nil {
+		return err
+	}
+	return executeReview(opts)
+}
+
 // parseScanFlags provides test compatibility: parses args through a fresh
 // cobra command instance and returns the resulting scanOptions.
 func parseScanFlags(args []string) (scanOptions, error) {