diff --git a/cmd/opencodereview/budget_output_test.go b/cmd/opencodereview/budget_output_test.go index 3021e726..2c2ca0e6 100644 --- a/cmd/opencodereview/budget_output_test.go +++ b/cmd/opencodereview/budget_output_test.go @@ -45,7 +45,7 @@ func TestEmitRunResult_JSONBudgetStopIsPartial(t *testing.T) { }, } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -96,7 +96,7 @@ func TestEmitRunResult_JSONBudgetDoesNotOverrideLegacyStatus(t *testing.T) { budgetExceeded: true, } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -124,7 +124,7 @@ func TestEmitRunResult_JSONNoBudgetIsSuccess(t *testing.T) { totalTokens: 15, } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -154,7 +154,7 @@ func TestEmitFailureUsage_TextEmitsStructuredRecord(t *testing.T) { sessionID: "sess-fail-1", } got := captureStderr(t, func() { - emitFailureUsage(ag, 42*time.Second, "text", nil) + emitFailureUsage(ag, 42*time.Second, "text", nil, nil) }) for _, want := range []string{"usage on failure", "1500 total tokens", "5 tool calls", "budget_exceeded=false", "sess-fail-1"} { if !strings.Contains(got, want) { @@ -175,7 +175,7 @@ func TestEmitFailureUsage_JSONEmitsStructuredRecord(t *testing.T) { } identity := &jsonLLMIdentity{Provider: "openai", Model: "gpt-5.4"} got := captureStderr(t, func() { - emitFailureUsage(ag, 5*time.Second, "json", identity) + emitFailureUsage(ag, 5*time.Second, "json", identity, nil) }) var out jsonOutput if err := json.Unmarshal([]byte(got), &out); err != nil { @@ -215,7 +215,7 @@ func TestEmitFailureUsage_BudgetExceededPropagated(t *testing.T) { budgetExceeded: true, } got := captureStderr(t, func() { - emitFailureUsage(ag, 3*time.Second, "text", nil) + emitFailureUsage(ag, 3*time.Second, "text", nil, nil) }) if !strings.Contains(got, "budget_exceeded=true") { t.Errorf("text failure record must reflect budget_exceeded=true; got %q", got) @@ -228,7 +228,7 @@ func TestEmitFailureUsage_BudgetExceededPropagated(t *testing.T) { budgetExceeded: true, } gotJSON := captureStderr(t, func() { - emitFailureUsage(ag2, 3*time.Second, "json", nil) + emitFailureUsage(ag2, 3*time.Second, "json", nil, nil) }) var out jsonOutput if err := json.Unmarshal([]byte(gotJSON), &out); err != nil { @@ -249,7 +249,7 @@ func TestEmitRunResult_BudgetExceededFalseOmittedFromJSON(t *testing.T) { totalTokens: 10, } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, []model.LlmComment{}, time.Now(), "json", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, []model.LlmComment{}, time.Now(), "json", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/cmd/opencodereview/emit_run_result_test.go b/cmd/opencodereview/emit_run_result_test.go index eba53383..ce4fb1e4 100644 --- a/cmd/opencodereview/emit_run_result_test.go +++ b/cmd/opencodereview/emit_run_result_test.go @@ -16,6 +16,7 @@ import ( sdktrace "go.opentelemetry.io/otel/sdk/trace" "github.com/alibaba/open-code-review/internal/agent" + "github.com/alibaba/open-code-review/internal/llm" "github.com/alibaba/open-code-review/internal/model" "github.com/alibaba/open-code-review/internal/session" ) @@ -90,7 +91,7 @@ func TestEmitRunResult_JSONNoFiles(t *testing.T) { ag := &mockResultProvider{filesReviewed: 0} identity := &jsonLLMIdentity{Provider: "anthropic", Model: "claude-opus-4-6"} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, identity) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, identity, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -111,7 +112,7 @@ func TestEmitRunResult_JSONLLMIdentityNamedProvider(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1} identity := &jsonLLMIdentity{Provider: "anthropic", Model: "claude-opus-4-6"} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, identity); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, identity, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -128,7 +129,7 @@ func TestEmitRunResult_JSONLLMIdentityOmitsUnknownProvider(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1} identity := &jsonLLMIdentity{Model: "gpt-5-codex"} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, identity); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, identity, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -155,7 +156,7 @@ func TestEmitRunResult_JSONUsesManifestTerminalState(t *testing.T) { }, } got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -183,7 +184,7 @@ func TestEmitRunResult_JSONUsesManifestTerminalState(t *testing.T) { func TestEmitRunResult_JSONSkippedIncludesManifest(t *testing.T) { ag := &mockResultProvider{manifest: mockManifest(session.StateSkipped)} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -238,7 +239,7 @@ func TestEmitRunResult_JSONManifestMatchesPersistedSessionEnd(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, sessionID: sh.SessionID, manifest: sh.FinalManifest()} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -301,7 +302,7 @@ func TestEmitRunResult_JSONWithComments(t *testing.T) { } comments := []model.LlmComment{{Path: "main.go", Content: "fix", StartLine: 1, EndLine: 2}} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, comments, time.Now(), "json", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, comments, time.Now(), "json", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -330,7 +331,7 @@ func TestEmitRunResult_JSONWithResumeInfo(t *testing.T) { }, } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -347,7 +348,7 @@ func TestEmitRunResult_JSONWithResumeInfo(t *testing.T) { func TestEmitRunResult_TextNoComments(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -360,7 +361,7 @@ func TestEmitRunResult_TextNoComments(t *testing.T) { func TestEmitRunResult_TextPartialNeverLooksGood(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StatePartial)} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -378,7 +379,7 @@ func TestEmitRunResult_TextCompleteReportsFindingsAndWaived(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, manifest: manifest} comments := []model.LlmComment{{Path: "a.go", Content: "fix", StartLine: 1, EndLine: 1}} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, comments, time.Now(), "text", "developer", nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, comments, time.Now(), "text", "developer", nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -392,7 +393,7 @@ func TestEmitRunResult_TextCompleteReportsFindingsAndWaived(t *testing.T) { func TestEmitRunResult_TextDoesNotPrintSuccessfulSessionHint(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, sessionID: "session-123"} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -406,7 +407,7 @@ func TestEmitRunResult_TextWithComments(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1} comments := []model.LlmComment{{Path: "a.go", Content: "rename", StartLine: 5, EndLine: 10}} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, comments, time.Now(), "text", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, comments, time.Now(), "text", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -425,7 +426,7 @@ func TestEmitRunResult_TextWithProjectSummary(t *testing.T) { projectSummary: "All tests pass, code quality is good.", } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -442,7 +443,7 @@ func TestEmitRunResult_AgentTextRestoresQuiet(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1} q := newQuietHandle("text", "agent") got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "agent", q, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "agent", q, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -462,7 +463,7 @@ func TestEmitRunResult_AgentJSONDoesNotRestore(t *testing.T) { } q := newQuietHandle("json", "agent") got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "agent", q, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "agent", q, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -477,7 +478,7 @@ func TestEmitRunResult_AgentJSONDoesNotRestore(t *testing.T) { func TestEmitRunResult_NilQuietHandle(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "agent", nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "agent", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -501,7 +502,7 @@ func TestEmitRunResult_JSONTraceIDFromContext(t *testing.T) { totalTokens: 15, } got := captureStdout(t, func() { - err := emitRunResult(ctx, ag, nil, time.Now(), "json", "developer", nil, nil) + err := emitRunResult(ctx, ag, nil, time.Now(), "json", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -526,7 +527,7 @@ func TestEmitRunResult_JSONNoFilesTraceID(t *testing.T) { ag := &mockResultProvider{filesReviewed: 0} got := captureStdout(t, func() { - err := emitRunResult(ctx, ag, nil, time.Now(), "json", "developer", nil, nil) + err := emitRunResult(ctx, ag, nil, time.Now(), "json", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -546,7 +547,7 @@ func TestEmitRunResult_JSONNoFilesTraceID(t *testing.T) { func TestEmitRunResult_JSONIncludesSessionID(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1, sessionID: "session-99"} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -559,3 +560,164 @@ func TestEmitRunResult_JSONIncludesSessionID(t *testing.T) { t.Errorf("session_id = %q, want session-99", out.SessionID) } } + +// --- retry report at the emit boundary (#368 P5) --- +// These cases pin how a frozen retry report reaches the two run exits, +// emitRunResult and emitFailureUsage. The report's own rendering is covered by +// retry_report_output_test.go; retryReportFixture lives there too. + +func TestEmitRunResult_JSONCarriesRetryReport(t *testing.T) { + ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StateComplete)} + rep := retryReportFixture() + got := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, rep); err != nil { + t.Fatalf("emitRunResult: %v", err) + } + }) + var out jsonOutput + if err := json.Unmarshal([]byte(got), &out); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if out.RetryReport == nil { + t.Fatalf("retry_report missing from JSON output: %s", got) + } + if out.RetryReport.SchemaVersion != llm.RetryReportSchemaVersion { + t.Errorf("schema_version = %q", out.RetryReport.SchemaVersion) + } + if out.RetryReport.TotalRequests != 12 || out.RetryReport.FailedRequests != 1 { + t.Errorf("aggregates not carried through: %+v", out.RetryReport) + } +} + +// A run with nothing to report must emit exactly the pre-#368 JSON shape. +func TestEmitRunResult_JSONOmitsRetryReportWhenNil(t *testing.T) { + ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StateComplete)} + got := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil); err != nil { + t.Fatalf("emitRunResult: %v", err) + } + }) + if strings.Contains(got, "retry_report") { + t.Errorf("nil report must not appear in JSON, got %s", got) + } +} + +// Order is part of the terminal contract: comments/manifest first, then the +// retry report, then the project summary. +func TestEmitRunResult_TextReportOrder(t *testing.T) { + ag := &mockResultProvider{ + filesReviewed: 2, + manifest: mockManifest(session.StateComplete), + projectSummary: "PROJECT-SUMMARY-MARKER", + } + got := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, retryReportFixture()); err != nil { + t.Fatalf("emitRunResult: %v", err) + } + }) + report := strings.Index(got, "LLM retry report:") + summary := strings.Index(got, "PROJECT-SUMMARY-MARKER") + if report < 0 { + t.Fatalf("report missing from text output: %s", got) + } + if summary < 0 || report > summary { + t.Errorf("report must precede the project summary\n%s", got) + } + if !strings.Contains(got, "- config.go / main_task #1: provider(402) -> failed") { + t.Errorf("per-request lines missing: %s", got) + } +} + +func TestEmitRunResult_TextOmitsReportWhenNil(t *testing.T) { + ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StateComplete)} + got := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, nil); err != nil { + t.Fatalf("emitRunResult: %v", err) + } + }) + if strings.Contains(got, "LLM retry report") { + t.Errorf("nil report must print nothing, got %s", got) + } +} + +// JSON mode must keep stdout a single JSON document, so the text renderer never +// runs there. +func TestEmitRunResult_JSONHasNoReportText(t *testing.T) { + ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StateComplete)} + got := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, retryReportFixture()); err != nil { + t.Fatalf("emitRunResult: %v", err) + } + }) + if strings.Contains(got, "LLM retry report:") { + t.Errorf("JSON mode must not emit the terminal summary: %s", got) + } + dec := json.NewDecoder(strings.NewReader(got)) + var first jsonOutput + if err := dec.Decode(&first); err != nil { + t.Fatalf("decode: %v", err) + } + if dec.More() { + t.Error("stdout must carry exactly one JSON document") + } +} + +func TestEmitFailureUsage_JSONCarriesRetryReport(t *testing.T) { + ag := &mockResultProvider{filesReviewed: 1, sessionID: "sess-1"} + got := captureStderr(t, func() { + emitFailureUsage(ag, time.Second, "json", nil, retryReportFixture()) + }) + var out jsonOutput + if err := json.Unmarshal([]byte(got), &out); err != nil { + t.Fatalf("unmarshal: %v\n%s", err, got) + } + if out.Status != "failed" { + t.Errorf("status = %q, want failed", out.Status) + } + if out.RetryReport == nil || out.RetryReport.FailedRequests != 1 { + t.Fatalf("retry_report missing or wrong on the failure exit: %s", got) + } +} + +func TestEmitFailureUsage_TextCarriesRetryReport(t *testing.T) { + ag := &mockResultProvider{filesReviewed: 1} + got := captureStderr(t, func() { + emitFailureUsage(ag, time.Second, "text", nil, retryReportFixture()) + }) + usage := strings.Index(got, "[ocr] usage on failure:") + report := strings.Index(got, "LLM retry report:") + if usage < 0 || report < 0 || usage > report { + t.Errorf("report must follow the usage line on stderr:\n%s", got) + } +} + +func TestEmitFailureUsage_NilReportUnchanged(t *testing.T) { + ag := &mockResultProvider{filesReviewed: 1} + got := captureStderr(t, func() { + emitFailureUsage(ag, time.Second, "text", nil, nil) + }) + if strings.Contains(got, "LLM retry report") { + t.Errorf("nil report must print nothing, got %q", got) + } +} + +// warningsForOutput is unrelated to the report, but the text exit now writes +// between the warnings block and the project summary; keep a case where both +// warnings and a report are present so the two cannot interleave. +func TestEmitRunResult_TextReportWithWarnings(t *testing.T) { + ag := &mockResultProvider{ + filesReviewed: 1, + warnings: []agent.AgentWarning{{Type: "subtask_error", File: "b.go", Message: "boom"}}, + } + got := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, retryReportFixture()); err != nil { + t.Fatalf("emitRunResult: %v", err) + } + }) + if !strings.Contains(got, "LLM retry report:") { + t.Errorf("report missing: %s", got) + } + if strings.Count(got, "LLM retry report:") != 1 { + t.Errorf("report emitted more than once: %s", got) + } +} diff --git a/cmd/opencodereview/llm_cmd.go b/cmd/opencodereview/llm_cmd.go index cad3e337..65adce6e 100644 --- a/cmd/opencodereview/llm_cmd.go +++ b/cmd/opencodereview/llm_cmd.go @@ -81,7 +81,9 @@ func runLLMTest() error { timeout = time.Duration(task.Timeout) * time.Second } - llmClient := llm.NewLLMClient(ep) + // No retry collector: llm test is a connectivity probe, not a review, and the + // retry report only describes ocr review. + llmClient := llm.NewLLMClient(ep, nil) messages := make([]llm.Message, 0, len(task.Messages)) for _, m := range task.Messages { diff --git a/cmd/opencodereview/manual_e2e_retry_test.go b/cmd/opencodereview/manual_e2e_retry_test.go new file mode 100644 index 00000000..104018cc --- /dev/null +++ b/cmd/opencodereview/manual_e2e_retry_test.go @@ -0,0 +1,182 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +//go:build manual_e2e + +// Manual end-to-end verification harness for #368 P5 (see +// docs/368/LLM请求重试实施路线图.md, P5 开工决策 8). It is excluded from the +// default build: run it explicitly with +// +// go test -tags manual_e2e -run TestManualE2ERetryReport -v ./cmd/opencodereview/ +// +// The harness mirrors executeReview's wiring (loadCommonContext → +// loadLLMRuntime → agent.New → ag.Run → RunManifest → Freeze) against a fake +// Anthropic server so the chain "real SDK retry → observer → boundary +// Finalize → Collector → Freeze → both output exits" can be inspected BEFORE +// any P5 production code exists. +package main + +import ( + "context" + "encoding/json" + "testing" + + "github.com/alibaba/open-code-review/internal/agent" + "github.com/alibaba/open-code-review/internal/llm" + "github.com/alibaba/open-code-review/internal/session" + "github.com/alibaba/open-code-review/internal/tool" +) + +// The fake Anthropic server, the fixture repo and the OCR_LLM_* wiring live in +// retry_fake_llm_test.go, shared with the automated end-to-end tests so the +// manual harness cannot drift from what those consider a retryable server. + +type manualResult struct { + report *llm.RetryReport + freezeErr error + manifest *session.RunManifest + runErr error + sessionID string + attempts map[string]int +} + +// runManualReview mirrors executeReview's wiring up to the point P5 will hook +// into, and returns everything the P5 output layer will read. +func runManualReview(t *testing.T, srv *fakeLLM) manualResult { + t.Helper() + + repoDir := retryTestRepo(t) + startFakeLLM(t, srv) + + cc, err := loadCommonContext(repoDir, "", 0, 4, true) + if err != nil { + t.Fatalf("loadCommonContext: %v", err) + } + rt, err := loadLLMRuntime(cc.Template, "", llm.ResolveOptions{}) + if err != nil { + t.Fatalf("loadLLMRuntime: %v", err) + } + if rt.RetryCollector == nil { + t.Fatal("llmRuntime.RetryCollector is nil; P2 wiring regressed") + } + + mode := tool.ParseReviewMode("HEAD~1", "HEAD", "") + fileReader := &tool.FileReader{RepoDir: cc.RepoDir, Mode: mode, Ref: "HEAD", Runner: cc.GitRunner} + tools := buildToolRegistry(rt.Collector, fileReader) + + ag := agent.New(agent.Args{ + RepoDir: cc.RepoDir, + From: "HEAD~1", + To: "HEAD", + ReviewMode: session.ReviewModeRange, + Template: *cc.Template, + SystemRule: cc.Resolver, + FileFilter: cc.FileFilter, + LLMClient: rt.Client, + Tools: tools, + PlanToolDefs: rt.PlanToolDefs, + MainToolDefs: rt.MainToolDefs, + CommentCollector: rt.Collector, + CommentWorkerPool: agent.NewCommentWorkerPool(2), + MaxConcurrency: 2, + ConcurrentTaskTimeout: 120, + Model: rt.Model, + Provider: rt.Provider, + GitRunner: cc.GitRunner, + RuntimeConfig: rt.RuntimeConfig, + }) + + comments, runErr := ag.Run(context.Background()) + manifest := ag.RunManifest() + + // P5 决策 7: the frozen run ID is the session's in-memory UUID, not the + // persistence-gated ag.SessionID(). + runID := ag.Session().SessionID + report, freezeErr := rt.RetryCollector.Freeze(runID) + + t.Logf("comments=%d runErr=%v", len(comments), runErr) + t.Logf("ag.Session().SessionID=%q ag.SessionID()=%q", runID, ag.SessionID()) + if manifest != nil { + t.Logf("manifest.terminal_state=%s", manifest.TerminalState) + } else { + t.Log("manifest=nil") + } + + attempts := srv.attemptCounts() + t.Logf("real HTTP attempts per file: %v", attempts) + + if freezeErr != nil { + t.Logf("Freeze error: %v", freezeErr) + } else if report == nil { + t.Log("Freeze returned (nil, nil): nothing worth reporting") + } else { + raw, _ := json.MarshalIndent(map[string]any{"retry_report": report}, "", " ") + t.Logf("frozen retry_report:\n%s", raw) + } + + return manualResult{ + report: report, + freezeErr: freezeErr, + manifest: manifest, + runErr: runErr, + sessionID: runID, + attempts: attempts, + } +} + +func TestManualE2ERetryReport(t *testing.T) { + t.Run("clean_first_try_success", func(t *testing.T) { + res := runManualReview(t, newFakeLLM()) + if res.freezeErr != nil { + t.Fatalf("Freeze must succeed on a clean run: %v", res.freezeErr) + } + if res.report != nil { + t.Fatalf("clean run must produce no report, got %+v", res.report) + } + if res.runErr != nil { + t.Fatalf("clean run must not fail: %v", res.runErr) + } + if res.sessionID == "" { + t.Fatal("session UUID must be non-empty") + } + }) + + t.Run("recovered_and_failed", func(t *testing.T) { + srv := newFakeLLM() + srv.rateLimitOnce["a.go"] = true + srv.hardFail["b.go"] = true + res := runManualReview(t, srv) + if res.freezeErr != nil { + t.Fatalf("Freeze: %v", res.freezeErr) + } + if res.report == nil { + t.Fatal("expected a report when one request was rate limited and another failed") + } + if res.report.RecoveredRequests == 0 { + t.Errorf("expected at least one recovered request: %+v", res.report) + } + if res.report.FailedRequests == 0 { + t.Errorf("expected at least one failed request: %+v", res.report) + } + if got := res.attempts["a.go"]; got < 2 { + t.Errorf("SDK should have retried a.go, saw %d HTTP attempts", got) + } + }) + + t.Run("all_files_fail", func(t *testing.T) { + srv := newFakeLLM() + srv.hardFail["a.go"] = true + srv.hardFail["b.go"] = true + res := runManualReview(t, srv) + if res.freezeErr != nil { + t.Fatalf("Freeze: %v", res.freezeErr) + } + if res.report == nil { + t.Fatal("expected a report when every file failed") + } + // P5 决策 3: this is the exit where emitRunResult and emitFailureUsage + // can both run, so record which one the report must travel through. + t.Logf("manifest==nil? %v ; runErr!=nil? %v -> emitRunResult runs: %v", + res.manifest == nil, res.runErr != nil, res.manifest != nil || res.runErr == nil) + }) +} diff --git a/cmd/opencodereview/output.go b/cmd/opencodereview/output.go index 0beb8938..9975b494 100644 --- a/cmd/opencodereview/output.go +++ b/cmd/opencodereview/output.go @@ -6,12 +6,14 @@ package main import ( "encoding/json" "fmt" + "io" "os" "strings" "time" "unicode" "github.com/alibaba/open-code-review/internal/agent" + "github.com/alibaba/open-code-review/internal/llm" "github.com/alibaba/open-code-review/internal/model" "github.com/alibaba/open-code-review/internal/session" "github.com/alibaba/open-code-review/internal/suggestdiff" @@ -291,6 +293,11 @@ type jsonOutput struct { Resume *agent.ResumeInfo `json:"resume,omitempty"` SessionID string `json:"session_id,omitempty"` Manifest *session.RunManifest `json:"manifest,omitempty"` + // RetryReport is the frozen LLM retry report (ocr.llm-retry-report/v1). + // Reuses llm.RetryReport's own field/tag definitions rather than mirroring + // them here, and sits last with omitempty so a first-try-success run emits + // byte-identical JSON to before #368. + RetryReport *llm.RetryReport `json:"retry_report,omitempty"` } func outputJSON(comments []model.LlmComment) error { @@ -309,7 +316,8 @@ 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, - manifest *session.RunManifest, budgetExceeded bool, llmIdentity *jsonLLMIdentity) error { + manifest *session.RunManifest, budgetExceeded bool, llmIdentity *jsonLLMIdentity, + retryReport *llm.RetryReport) error { publishedWarnings := warningsForOutput(warnings, manifest) out := jsonOutput{ Status: "success", @@ -331,6 +339,7 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW Resume: resumeInfo, SessionID: sessionID, Manifest: manifest, + RetryReport: retryReport, } var total int64 for _, v := range toolCalls { @@ -375,6 +384,64 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW return enc.Encode(out) } +// outputRetryReportText renders the frozen retry report as the terminal +// summary. It is a run result, not a warning, so it goes to the same writer as +// the review result rather than to stderr; JSON mode never calls this (stdout +// must stay a single JSON document). +// +// Nothing here is free text from a provider: only stable classes, numeric +// status codes, the file path and the task type — no API keys, headers, +// bodies, prompts, URLs or raw SDK error strings. Every request in the report +// is listed; the report already only contains requests that erred or retried, +// so there is no separate terminal truncation contract to reason about. +func outputRetryReportText(w io.Writer, rep *llm.RetryReport) { + if rep == nil { + return + } + retryWord := "retries" + if rep.TotalRetries == 1 { + retryWord = "retry" + } + fmt.Fprintf(w, "\nLLM retry report: %d/%d requests retried, %d %s, %d recovered, %d failed, %d cancelled\n", + rep.RetriedRequests, rep.TotalRequests, rep.TotalRetries, retryWord, + rep.RecoveredRequests, rep.FailedRequests, rep.CancelledRequests) + for _, r := range rep.Requests { + fmt.Fprintf(w, "- %s / %s #%d: %s\n", + sanitizeTerminal(r.FilePath), sanitizeTerminal(r.TaskType), + r.RequestNo, retryAttemptChain(r)) + } +} + +// retryAttemptChain renders one logical request's attempts as +// "rate_limited(429) -> overloaded(529) -> success". +// +// A trailing request-level outcome is appended for failed and, when the last +// attempt does not already say so, cancelled. A recovered or succeeded request +// already ends in a "success" attempt, so repeating the outcome there would be +// noise, whereas a request that never succeeded would otherwise end on its last +// error with no sign of how it finished. cancelled in particular is a routine +// outcome (background memory compression is deliberately abandoned at the end +// of every file), so it must be visibly distinct from a provider failure. +func retryAttemptChain(r llm.RequestReport) string { + parts := make([]string, 0, len(r.Attempts)+1) + for _, a := range r.Attempts { + switch { + case a.Outcome == llm.AttemptSuccess: + parts = append(parts, "success") + case a.StatusCode > 0: + parts = append(parts, fmt.Sprintf("%s(%d)", a.ErrorClass, a.StatusCode)) + default: + parts = append(parts, string(a.ErrorClass)) + } + } + if r.Outcome == llm.OutcomeFailed || + (r.Outcome == llm.OutcomeCancelled && + (len(parts) == 0 || parts[len(parts)-1] != string(llm.OutcomeCancelled))) { + parts = append(parts, string(r.Outcome)) + } + return strings.Join(parts, " -> ") +} + func manifestMessage(manifest *session.RunManifest, findings int) string { if manifest == nil { return "" @@ -437,7 +504,14 @@ func outputJSONNoFiles(traceID string, llmIdentity *jsonLLMIdentity) error { // therefore always carries exactly one JSON document); otherwise a single // human-readable [ocr] line. It must never return an error that masks the // original failure — all writes are best-effort. -func emitFailureUsage(ag ResultProvider, duration time.Duration, outputFormat string, llmIdentity *jsonLLMIdentity) { +// +// retryReport must be nil whenever emitRunResult already ran: a constructed +// manifest is publishable even on a failed run, so both this record and the +// normal result exit can execute for the same run, and the report belongs to +// exactly one of them. Pass the frozen report here only when the normal exit +// was skipped, so the report is never duplicated and never silently dropped. +func emitFailureUsage(ag ResultProvider, duration time.Duration, outputFormat string, llmIdentity *jsonLLMIdentity, + retryReport *llm.RetryReport) { var toolTotal int64 for _, v := range ag.ToolCalls() { toolTotal += v @@ -461,7 +535,8 @@ func emitFailureUsage(ag ResultProvider, duration time.Duration, outputFormat st Total: toolTotal, ByTool: ag.ToolCalls(), }, - SessionID: ag.SessionID(), + SessionID: ag.SessionID(), + RetryReport: retryReport, } enc := json.NewEncoder(os.Stderr) enc.SetIndent("", " ") @@ -475,6 +550,9 @@ func emitFailureUsage(ag ResultProvider, duration time.Duration, outputFormat st fmt.Fprintf(os.Stderr, ", session %s", id) } fmt.Fprintln(os.Stderr) + // Text mode has no structured envelope, so the report follows the usage + // line on the same stream. + outputRetryReportText(os.Stderr, retryReport) } // outputPreview renders a preview in the requested output format. Any format diff --git a/cmd/opencodereview/output_helpers_test.go b/cmd/opencodereview/output_helpers_test.go index a33e26c0..f0976f3c 100644 --- a/cmd/opencodereview/output_helpers_test.go +++ b/cmd/opencodereview/output_helpers_test.go @@ -173,7 +173,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, "", nil, false, nil) + err := outputJSONWithWarnings(nil, warnings, 1, 10, 5, 15, 0, 0, time.Second, "", nil, "abc123trace", nil, "", nil, false, nil, nil) _ = w.Close() os.Stdout = old @@ -286,7 +286,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, "", nil, false, 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, false, nil, nil) _ = w.Close() os.Stdout = old @@ -324,7 +324,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, "", nil, false, nil) + err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 0, 0, time.Second, "", nil, "", nil, "", nil, false, nil, nil) _ = w.Close() os.Stdout = old diff --git a/cmd/opencodereview/retry_fake_llm_test.go b/cmd/opencodereview/retry_fake_llm_test.go new file mode 100644 index 00000000..0713f821 --- /dev/null +++ b/cmd/opencodereview/retry_fake_llm_test.go @@ -0,0 +1,196 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "bytes" + "fmt" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/alibaba/open-code-review/internal/llm" +) + +// retryTestFixedTime is an arbitrary fixed instant for hand-fed attempt +// timestamps, so derived durations stay deterministic. +var retryTestFixedTime = time.Date(2026, 8, 7, 12, 0, 0, 0, time.UTC) + +// Shared fake-LLM fixture for the #368 retry-report tests. Used by both the +// automated end-to-end tests and the tag-gated manual harness +// (manual_e2e_retry_test.go), so the two cannot drift apart in what they +// consider a retryable server. + +// fakeLLM serves the Anthropic messages API. Per reviewed file it can inject a +// 429 (which the SDK must retry on its own) or a permanent 402 (which it must +// not retry). +type fakeLLM struct { + mu sync.Mutex + // attemptsByFile counts real HTTP attempts per file, so a test can assert + // the SDK retried rather than OCR re-requesting. + attemptsByFile map[string]int + // rateLimitOnce lists files whose first attempt returns 429. + rateLimitOnce map[string]bool + // hardFail lists files whose every attempt returns 402. + hardFail map[string]bool +} + +// newFakeLLM returns a server that succeeds on the first attempt for every +// file. rateLimitOnce and hardFail are set by the caller before use. +func newFakeLLM() *fakeLLM { + return &fakeLLM{ + attemptsByFile: map[string]int{}, + rateLimitOnce: map[string]bool{}, + hardFail: map[string]bool{}, + } +} + +// attemptCounts snapshots the per-file real HTTP attempt counts. +func (f *fakeLLM) attemptCounts() map[string]int { + f.mu.Lock() + defer f.mu.Unlock() + out := make(map[string]int, len(f.attemptsByFile)) + for k, v := range f.attemptsByFile { + out[k] = v + } + return out +} + +// markers maps a per-file token that appears only in that file's diff onto the +// file name. The prompt's change_files section lists the *other* file's path, +// so matching on the path itself would misattribute requests; the marker only +// occurs in the reviewed file's diff body. +var markers = map[string]string{"MARKER_ALPHA": "a.go", "MARKER_BETA": "b.go"} + +func (f *fakeLLM) fileOf(body []byte) string { + for marker, name := range markers { + if bytes.Contains(body, []byte(marker)) { + return name + } + } + return "unknown" +} + +func (f *fakeLLM) ServeHTTP(w http.ResponseWriter, r *http.Request) { + body := new(bytes.Buffer) + _, _ = body.ReadFrom(r.Body) + raw := body.Bytes() + file := f.fileOf(raw) + hasTools := bytes.Contains(raw, []byte(`"tools"`)) + + f.mu.Lock() + f.attemptsByFile[file]++ + n := f.attemptsByFile[file] + rateLimit := f.rateLimitOnce[file] && n == 1 + fail := f.hardFail[file] + f.mu.Unlock() + + w.Header().Set("Content-Type", "application/json") + w.Header().Set("request-id", fmt.Sprintf("req_%s_%d", strings.TrimSuffix(file, ".go"), n)) + + switch { + case fail: + w.WriteHeader(http.StatusPaymentRequired) + _, _ = w.Write([]byte(`{"type":"error","error":{"type":"invalid_request_error","message":"payment required"}}`)) + return + case rateLimit: + // Retry-After: 1 keeps the SDK's observed backoff deterministic enough to + // assert on without making the test wait long. + w.Header().Set("Retry-After", "1") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte(`{"type":"error","error":{"type":"rate_limit_error","message":"slow down"}}`)) + return + } + + // Main-task rounds carry tool definitions; the plan phase does not. + if hasTools { + _, _ = w.Write([]byte(`{"id":"msg_1","type":"message","role":"assistant","model":"claude-test", + "content":[{"type":"tool_use","id":"tu_1","name":"task_done","input":{"state":"DONE"}}], + "stop_reason":"tool_use","usage":{"input_tokens":10,"output_tokens":5}}`)) + return + } + _, _ = w.Write([]byte(`{"id":"msg_1","type":"message","role":"assistant","model":"claude-test", + "content":[{"type":"text","text":"plan: read the diff, then call task_done"}], + "stop_reason":"end_turn","usage":{"input_tokens":10,"output_tokens":5}}`)) +} + +// retryTestGit runs git in dir with a fixed identity so the fixture repo does +// not depend on the developer's git config. +func retryTestGit(t *testing.T, dir string, args ...string) { + t.Helper() + full := append([]string{ + "-c", "user.email=ocr@example.test", + "-c", "user.name=ocr", + "-c", "commit.gpgsign=false", + }, args...) + cmd := exec.Command("git", full...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } +} + +// retryTestRepo builds a two-file repo with one reviewable commit range +// (HEAD~1..HEAD), each file carrying its own marker in the second commit only. +func retryTestRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + retryTestGit(t, dir, "init", "-q", "-b", "main") + for _, name := range []string{"a.go", "b.go"} { + body := fmt.Sprintf("package p\n\nfunc %s() int { return 1 }\n", strings.TrimSuffix(name, ".go")) + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + retryTestGit(t, dir, "add", ".") + retryTestGit(t, dir, "commit", "-q", "-m", "base") + for marker, name := range markers { + body := fmt.Sprintf("package p\n\n// changed %s\nfunc %s() int {\n\treturn 2\n}\n", + marker, strings.TrimSuffix(name, ".go")) + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + retryTestGit(t, dir, "add", ".") + retryTestGit(t, dir, "commit", "-q", "-m", "change") + return dir +} + +// startFakeLLM starts srv and points the OCR_LLM_* endpoint resolution at it, +// with HOME/XDG_CONFIG_HOME redirected so the developer's real config and +// session directory (both under $HOME/.opencodereview) are never touched. +func startFakeLLM(t *testing.T, srv *fakeLLM) { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + + server := httptest.NewServer(srv) + t.Cleanup(server.Close) + + t.Setenv("OCR_LLM_URL", server.URL+"/v1/messages") + t.Setenv("OCR_LLM_TOKEN", "test-token") + t.Setenv("OCR_LLM_MODEL", "claude-test") + t.Setenv("OCR_LLM_PROTOCOL", "anthropic") + t.Setenv("OCR_LLM_AUTH_HEADER", "x-api-key") + t.Setenv("OCR_LLM_TIMEOUT", "30") +} + +// poisonedRetryCollector returns a collector holding one request that recorded +// an attempt and was never finalized, which is exactly the invariant violation +// Freeze refuses to publish. It is the only way to reach the construction-error +// branch from the outside: every production path finalizes on every exit. +func poisonedRetryCollector() *llm.RetryCollector { + c := llm.NewRetryCollector() + m := llm.RequestMeta{Model: "claude-test", FilePath: "ghost.go", TaskType: "main_task", RequestNo: 1} + base := retryTestFixedTime + c.RecordAttempt(m, llm.AttemptRecord{}, base, base) + return c +} diff --git a/cmd/opencodereview/retry_report_e2e_test.go b/cmd/opencodereview/retry_report_e2e_test.go new file mode 100644 index 00000000..e5727fb3 --- /dev/null +++ b/cmd/opencodereview/retry_report_e2e_test.go @@ -0,0 +1,320 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/llm" + "github.com/alibaba/open-code-review/internal/session" +) + +// End-to-end coverage of the #368 P5 wiring: a real review run against the fake +// Anthropic server, driven through runReview so review_cmd.go's own Freeze / +// publish decisions are the thing under test rather than a re-implementation of +// them. See docs/368/LLM请求重试实施路线图.md, P5 开工决策 3/4/7/8. + +// runReviewCapturingBoth runs a review and returns (stdout, stderr, error). +// Both streams are needed because the report has two possible exits and the +// contract is that exactly one of them carries it. +func runReviewCapturingBoth(t *testing.T, repoDir, format string) (string, string, error) { + t.Helper() + var err error + var out string + errOut := captureStderr(t, func() { + out = captureStdout(t, func() { + err = runReview([]string{"--repo", repoDir, "--from", "HEAD~1", "--to", "HEAD", "--format", format}) + }) + }) + return out, errOut, err +} + +func TestReviewE2E_CleanRunEmitsNoRetryReport(t *testing.T) { + repoDir := retryTestRepo(t) + startFakeLLM(t, newFakeLLM()) + + out, errOut, err := runReviewCapturingBoth(t, repoDir, "json") + if err != nil { + t.Fatalf("review must succeed: %v\nstderr: %s", err, errOut) + } + if strings.Contains(out, "retry_report") { + t.Errorf("a first-try-success run must not emit retry_report:\n%s", out) + } + if strings.Contains(errOut, "LLM retry report") { + t.Errorf("nothing should reach the failure exit either:\n%s", errOut) + } +} + +func TestReviewE2E_RecoveredAndFailedReachesJSONExit(t *testing.T) { + repoDir := retryTestRepo(t) + srv := newFakeLLM() + srv.rateLimitOnce["a.go"] = true + srv.hardFail["b.go"] = true + startFakeLLM(t, srv) + + out, errOut, err := runReviewCapturingBoth(t, repoDir, "json") + // One file failed and one succeeded, so coverage is partial and the run + // exits 0. + if err != nil { + t.Fatalf("partial coverage must exit 0: %v\nstderr: %s", err, errOut) + } + + var got jsonOutput + if e := json.Unmarshal([]byte(out), &got); e != nil { + t.Fatalf("unmarshal stdout: %v\n%s", e, out) + } + rep := got.RetryReport + if rep == nil { + t.Fatalf("retry_report missing from the normal JSON exit:\n%s", out) + } + if rep.SchemaVersion != llm.RetryReportSchemaVersion { + t.Errorf("schema_version = %q", rep.SchemaVersion) + } + if rep.TotalRequests < 2 { + t.Errorf("total_requests = %d, want at least the two reviewed files", rep.TotalRequests) + } + if rep.RetriedRequests != 1 || rep.TotalRetries != 1 { + t.Errorf("expected exactly one retried request with one retry, got %+v", rep) + } + if rep.RecoveredRequests != 1 { + t.Errorf("recovered_requests = %d, want 1", rep.RecoveredRequests) + } + if rep.FailedRequests == 0 { + t.Errorf("failed_requests = 0, want the hard-failing file counted: %+v", rep) + } + if n := srv.attemptCounts()["a.go"]; n < 2 { + t.Errorf("the SDK, not OCR, must have retried a.go; saw %d HTTP attempts", n) + } + + // The retried request must show the 429 and the recovery, and must carry the + // observed backoff the Retry-After header asked for. + var recovered *llm.RequestReport + for i := range rep.Requests { + if rep.Requests[i].Outcome == llm.OutcomeRecovered { + recovered = &rep.Requests[i] + } + } + if recovered == nil { + t.Fatalf("no recovered request listed: %+v", rep.Requests) + } + if len(recovered.Attempts) != 2 { + t.Fatalf("recovered request must list 2 attempts, got %+v", recovered.Attempts) + } + first, second := recovered.Attempts[0], recovered.Attempts[1] + if first.ErrorClass != llm.ErrorClassRateLimited || first.StatusCode != 429 { + t.Errorf("first attempt = %+v, want rate_limited/429", first) + } + if first.FailurePhase != llm.FailurePhaseHTTP { + t.Errorf("failure_phase = %q, want http", first.FailurePhase) + } + if second.Outcome != llm.AttemptSuccess { + t.Errorf("second attempt = %+v, want success", second) + } + if second.ObservedBackoffMS < 1000 { + t.Errorf("observed_backoff_ms = %d, want at least the 1s Retry-After", second.ObservedBackoffMS) + } + + // Whatever the JSON says must also be readable in the run's stderr-free + // stdout stream as a single document. + dec := json.NewDecoder(strings.NewReader(out)) + if e := dec.Decode(new(jsonOutput)); e != nil { + t.Fatalf("decode: %v", e) + } + if dec.More() { + t.Error("stdout must carry exactly one JSON document") + } +} + +func TestReviewE2E_RetryReportReachesTextExit(t *testing.T) { + repoDir := retryTestRepo(t) + srv := newFakeLLM() + srv.rateLimitOnce["a.go"] = true + srv.hardFail["b.go"] = true + startFakeLLM(t, srv) + + out, errOut, err := runReviewCapturingBoth(t, repoDir, "text") + if err != nil { + t.Fatalf("partial coverage must exit 0: %v\nstderr: %s", err, errOut) + } + if !strings.Contains(out, "LLM retry report:") { + t.Fatalf("terminal summary missing from stdout:\n%s", out) + } + if !strings.Contains(out, "rate_limited(429) -> success") { + t.Errorf("attempt chain missing from the terminal summary:\n%s", out) + } + if !strings.Contains(out, "provider(402) -> failed") { + t.Errorf("failed request missing from the terminal summary:\n%s", out) + } + // The summary is a run result, so it must not be duplicated onto stderr. + if strings.Contains(errOut, "LLM retry report") { + t.Errorf("report must not also appear on stderr:\n%s", errOut) + } + // It must carry no secret material: the token, the endpoint URL and raw + // provider error text all stay out. + for _, forbidden := range []string{"test-token", "x-api-key", "127.0.0.1", "payment required", "slow down"} { + if strings.Contains(out, forbidden) { + t.Errorf("terminal output leaked %q:\n%s", forbidden, out) + } + } +} + +// Every file failing still produces a manifest (terminal_state=failed), so the +// normal exit runs and the failure-usage record must not repeat the report. +// This is the dedup rule of 决策 3, and the only case where both exits execute. +func TestReviewE2E_AllFilesFailPublishesReportOnce(t *testing.T) { + repoDir := retryTestRepo(t) + srv := newFakeLLM() + srv.hardFail["a.go"] = true + srv.hardFail["b.go"] = true + startFakeLLM(t, srv) + + out, errOut, err := runReviewCapturingBoth(t, repoDir, "json") + if err == nil { + t.Fatalf("a fully failed review must exit non-zero\nstdout: %s", out) + } + + var got jsonOutput + if e := json.Unmarshal([]byte(out), &got); e != nil { + t.Fatalf("unmarshal stdout: %v\n%s", e, out) + } + if got.RetryReport == nil { + t.Fatalf("retry_report missing from the normal exit:\n%s", out) + } + if got.RetryReport.FailedRequests == 0 { + t.Errorf("failed_requests = 0 on an all-failed run: %+v", got.RetryReport) + } + // The failure-usage record on stderr is emitted for the same run; it must not + // carry a second copy. + if strings.Contains(errOut, "retry_report") { + t.Errorf("report duplicated onto the failure exit:\n%s", errOut) + } + if !strings.Contains(errOut, `"status": "failed"`) { + t.Errorf("failure usage record missing from stderr:\n%s", errOut) + } +} + +func TestReviewE2E_AllFilesFailTextPublishesReportOnce(t *testing.T) { + repoDir := retryTestRepo(t) + srv := newFakeLLM() + srv.hardFail["a.go"] = true + srv.hardFail["b.go"] = true + startFakeLLM(t, srv) + + out, errOut, err := runReviewCapturingBoth(t, repoDir, "text") + if err == nil { + t.Fatal("a fully failed review must exit non-zero") + } + if !strings.Contains(out, "LLM retry report:") { + t.Fatalf("report missing from stdout:\n%s", out) + } + if strings.Contains(errOut, "LLM retry report:") { + t.Errorf("report duplicated onto stderr:\n%s", errOut) + } + if !strings.Contains(errOut, "[ocr] usage on failure:") { + t.Errorf("failure usage line missing:\n%s", errOut) + } +} + +// 决策 4: a report-construction error is a publish-side failure. It must not be +// reported as a failed review, must not print a --resume hint, must not emit a +// failure-usage record, and must not publish a partial report — while the +// review's own result is still published. +func TestReviewE2E_FreezeErrorIsAPublishError(t *testing.T) { + repoDir := retryTestRepo(t) + startFakeLLM(t, newFakeLLM()) + + orig := newRetryCollector + newRetryCollector = poisonedRetryCollector + t.Cleanup(func() { newRetryCollector = orig }) + + out, errOut, err := runReviewCapturingBoth(t, repoDir, "json") + if err == nil { + t.Fatalf("a report construction error must surface\nstdout: %s", out) + } + if !strings.Contains(err.Error(), "freeze retry report") { + t.Errorf("error = %v, want it to name the freeze step", err) + } + if strings.Contains(err.Error(), "review failed") { + t.Errorf("a publish-side error must not be reported as a failed review: %v", err) + } + if strings.Contains(errOut, "--resume") { + t.Errorf("no resume hint belongs on a successful review:\n%s", errOut) + } + if strings.Contains(errOut, `"status": "failed"`) { + t.Errorf("no failure usage record belongs on a successful review:\n%s", errOut) + } + + var got jsonOutput + if e := json.Unmarshal([]byte(out), &got); e != nil { + t.Fatalf("the review result must still be published: %v\n%s", e, out) + } + if got.RetryReport != nil { + t.Errorf("a self-contradictory report must not be published: %+v", got.RetryReport) + } + if got.Manifest == nil { + t.Errorf("the manifest must still be published:\n%s", out) + } +} + +// 决策 7: run_id is the session's in-memory UUID rather than the +// persistence-gated ag.SessionID(), so a session whose JSONL file could not be +// created still yields a report with usable logical_request_ids — while +// session_id stays empty and no --resume hint is printed, because there is +// nothing to resume from. +func TestReviewE2E_ReportSurvivesSessionPersistenceFailure(t *testing.T) { + repoDir := retryTestRepo(t) + srv := newFakeLLM() + srv.rateLimitOnce["a.go"] = true + srv.hardFail["b.go"] = true + startFakeLLM(t, srv) + + // Block session file creation by occupying the session subdirectory with a + // regular file, so the writer's MkdirAll fails. The path is derived from + // session.SessionsDir rather than hardcoded so it follows the subdirectory + // name, and only the *session* level is blocked — $HOME/.opencodereview must + // stay a real directory or global rule loading fails first and the run never + // reaches an LLM request. + sessionsDir, err := session.SessionsDir(repoDir) + if err != nil { + t.Fatal(err) + } + blocked := filepath.Dir(sessionsDir) // $HOME/.opencodereview/ + if err := os.MkdirAll(filepath.Dir(blocked), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(blocked, []byte("not a directory"), 0o600); err != nil { + t.Fatal(err) + } + out, errOut, err := runReviewCapturingBoth(t, repoDir, "json") + // The session-delivery failure is itself reported, but a manifest was still + // constructed, so the normal exit publishes the complete result first. + if err == nil { + t.Fatal("a session delivery failure must surface") + } + var got jsonOutput + if e := json.Unmarshal([]byte(out), &got); e != nil { + t.Fatalf("the result must still be published: %v\n%s", e, out) + } + if got.RetryReport == nil { + t.Fatalf("report must still be produced without persistence:\n%s", out) + } + if got.SessionID != "" { + t.Errorf("session_id = %q, want empty when the session was not persisted", got.SessionID) + } + if strings.Contains(errOut, "--resume") { + t.Errorf("no resume target exists, so no hint may be printed:\n%s", errOut) + } + if strings.Contains(errOut, "retry_report") { + t.Errorf("report duplicated onto the failure exit:\n%s", errOut) + } + for _, r := range got.RetryReport.Requests { + if r.LogicalRequestID == "" { + t.Errorf("logical_request_id must still be derivable: %+v", r) + } + } +} diff --git a/cmd/opencodereview/retry_report_render_test.go b/cmd/opencodereview/retry_report_render_test.go new file mode 100644 index 00000000..fc22a397 --- /dev/null +++ b/cmd/opencodereview/retry_report_render_test.go @@ -0,0 +1,321 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "bytes" + "context" + "encoding/json" + "strings" + "testing" + "time" + + "github.com/alibaba/open-code-review/internal/llm" + "github.com/alibaba/open-code-review/internal/session" +) + +// retryReportFixture is the report used by most rendering assertions: one +// request recovered after two errors, one request that never succeeded, and a +// total_requests larger than the listed set (the first-try successes are +// counted but not listed). +func retryReportFixture() *llm.RetryReport { + return &llm.RetryReport{ + SchemaVersion: llm.RetryReportSchemaVersion, + TotalRequests: 12, + RetriedRequests: 1, + TotalRetries: 2, + RecoveredRequests: 1, + FailedRequests: 1, + Requests: []llm.RequestReport{ + { + LogicalRequestID: "aaa", + Model: "claude-test", + FilePath: "payment.go", + TaskType: "main_task", + RequestNo: 2, + Outcome: llm.OutcomeRecovered, + Attempts: []llm.AttemptRecord{ + {Number: 1, Outcome: llm.AttemptError, ErrorClass: llm.ErrorClassRateLimited, FailurePhase: llm.FailurePhaseHTTP, StatusCode: 429}, + {Number: 2, Outcome: llm.AttemptError, ErrorClass: llm.ErrorClassOverloaded, FailurePhase: llm.FailurePhaseHTTP, StatusCode: 529}, + {Number: 3, Outcome: llm.AttemptSuccess}, + }, + }, + { + LogicalRequestID: "bbb", + Model: "claude-test", + FilePath: "config.go", + TaskType: "main_task", + RequestNo: 1, + Outcome: llm.OutcomeFailed, + Attempts: []llm.AttemptRecord{ + {Number: 1, Outcome: llm.AttemptError, ErrorClass: llm.ErrorClassProvider, FailurePhase: llm.FailurePhaseHTTP, StatusCode: 402}, + }, + }, + }, + } +} + +// The expected rendering is fixed by docs/368/LLM请求重试实施细节.md §5, so the +// text contract is asserted whole rather than by substring. +const wantRetryReportText = ` +LLM retry report: 1/12 requests retried, 2 retries, 1 recovered, 1 failed, 0 cancelled +- payment.go / main_task #2: rate_limited(429) -> overloaded(529) -> success +- config.go / main_task #1: provider(402) -> failed +` + +func TestOutputRetryReportText_RecoveredAndFailed(t *testing.T) { + var buf bytes.Buffer + outputRetryReportText(&buf, retryReportFixture()) + if got := buf.String(); got != wantRetryReportText { + t.Errorf("text report mismatch\n got: %q\nwant: %q", got, wantRetryReportText) + } +} + +func TestOutputRetryReportText_NilWritesNothing(t *testing.T) { + var buf bytes.Buffer + outputRetryReportText(&buf, nil) + if buf.Len() != 0 { + t.Errorf("nil report must write nothing, got %q", buf.String()) + } +} + +func TestOutputRetryReportText_SingularRetry(t *testing.T) { + rep := retryReportFixture() + rep.TotalRetries = 1 + var buf bytes.Buffer + outputRetryReportText(&buf, rep) + if !strings.Contains(buf.String(), "1 retry,") { + t.Errorf("total_retries=1 must read %q, got %q", "1 retry,", buf.String()) + } +} + +// A retry that ended in success without any error attempt is a real outcome: +// an HTTP 200 carrying x-should-retry: true makes the SDK attempt again. The +// summary then shows a retry with zero recovered and zero failed, which is +// expected rather than a bug (see the roadmap's risk table). +func TestOutputRetryReportText_SucceededAfterRetry(t *testing.T) { + rep := &llm.RetryReport{ + SchemaVersion: llm.RetryReportSchemaVersion, + TotalRequests: 1, + RetriedRequests: 1, + TotalRetries: 1, + Requests: []llm.RequestReport{{ + LogicalRequestID: "aaa", + Model: "claude-test", + FilePath: "payment.go", + TaskType: "main_task", + RequestNo: 2, + Outcome: llm.OutcomeSucceeded, + Attempts: []llm.AttemptRecord{ + {Number: 1, Outcome: llm.AttemptSuccess}, + {Number: 2, Outcome: llm.AttemptSuccess}, + }, + }}, + } + want := "\nLLM retry report: 1/1 requests retried, 1 retry, 0 recovered, 0 failed, 0 cancelled\n" + + "- payment.go / main_task #2: success -> success\n" + var buf bytes.Buffer + outputRetryReportText(&buf, rep) + if got := buf.String(); got != want { + t.Errorf("text report mismatch\n got: %q\nwant: %q", got, want) + } +} + +// A cancelled request keeps its clean attempt and is told apart from a provider +// failure only by the trailing outcome, so that suffix is part of the contract. +func TestOutputRetryReportText_CancelledSuffix(t *testing.T) { + rep := &llm.RetryReport{ + SchemaVersion: llm.RetryReportSchemaVersion, + TotalRequests: 1, + CancelledRequests: 1, + Requests: []llm.RequestReport{{ + LogicalRequestID: "aaa", + Model: "claude-test", + FilePath: "payment.go", + TaskType: "memory_compression_task", + RequestNo: 1, + Outcome: llm.OutcomeCancelled, + Attempts: []llm.AttemptRecord{{Number: 1, Outcome: llm.AttemptSuccess}}, + }}, + } + want := "\nLLM retry report: 0/1 requests retried, 0 retries, 0 recovered, 0 failed, 1 cancelled\n" + + "- payment.go / memory_compression_task #1: success -> cancelled\n" + var buf bytes.Buffer + outputRetryReportText(&buf, rep) + if got := buf.String(); got != want { + t.Errorf("text report mismatch\n got: %q\nwant: %q", got, want) + } +} + +func TestRetryAttemptChain_CancelledAttemptNotDuplicated(t *testing.T) { + r := llm.RequestReport{ + Outcome: llm.OutcomeCancelled, + Attempts: []llm.AttemptRecord{{ + Number: 1, Outcome: llm.AttemptError, + ErrorClass: llm.ErrorClassCancelled, FailurePhase: llm.FailurePhaseContext, + }}, + } + if got, want := retryAttemptChain(r), "cancelled"; got != want { + t.Errorf("chain = %q, want %q", got, want) + } +} + +// An attempt with no HTTP response has no status code to show, so the class is +// rendered bare rather than as "network(0)". +func TestRetryAttemptChain_NoStatusCode(t *testing.T) { + r := llm.RequestReport{ + Outcome: llm.OutcomeFailed, + Attempts: []llm.AttemptRecord{ + {Number: 1, Outcome: llm.AttemptError, ErrorClass: llm.ErrorClassNetwork, FailurePhase: llm.FailurePhaseTransport}, + }, + } + if got, want := retryAttemptChain(r), "network -> failed"; got != want { + t.Errorf("chain = %q, want %q", got, want) + } +} + +func TestOutputRetryReportText_SanitizesControlChars(t *testing.T) { + rep := retryReportFixture() + rep.Requests[0].FilePath = "pay\x1b[31mment.go" + rep.Requests[0].TaskType = "main\x07_task" + var buf bytes.Buffer + outputRetryReportText(&buf, rep) + if strings.ContainsAny(buf.String(), "\x1b\x07") { + t.Errorf("control characters must be stripped, got %q", buf.String()) + } +} + +// The report carries only aggregates, stable classes, status codes and request +// identity. This pins the emitted JSON key set so a future field cannot quietly +// add a prompt, URL, header or raw provider error string. +func TestRetryReportJSON_KeySetIsAllowlisted(t *testing.T) { + raw, err := json.Marshal(retryReportFixture()) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var top map[string]json.RawMessage + if err := json.Unmarshal(raw, &top); err != nil { + t.Fatalf("unmarshal: %v", err) + } + allowedTop := map[string]bool{ + "schema_version": true, "total_requests": true, "retried_requests": true, + "total_retries": true, "recovered_requests": true, "failed_requests": true, + "cancelled_requests": true, + "requests": true, + } + for k := range top { + if !allowedTop[k] { + t.Errorf("unexpected top-level key %q in retry report", k) + } + } + + var reqs []map[string]json.RawMessage + if err := json.Unmarshal(top["requests"], &reqs); err != nil { + t.Fatalf("unmarshal requests: %v", err) + } + allowedReq := map[string]bool{ + "logical_request_id": true, "provider": true, "model": true, + "file_path": true, "task_type": true, "request_no": true, + "outcome": true, "attempts": true, + } + allowedAttempt := map[string]bool{ + "attempt": true, "outcome": true, "error_class": true, "failure_phase": true, + "status_code": true, "request_id": true, "retry_after_ms": true, + "observed_backoff_ms": true, "duration_to_headers_ms": true, + "sdk_retry_directive": true, + } + for _, r := range reqs { + for k := range r { + if !allowedReq[k] { + t.Errorf("unexpected request key %q in retry report", k) + } + } + var attempts []map[string]json.RawMessage + if err := json.Unmarshal(r["attempts"], &attempts); err != nil { + t.Fatalf("unmarshal attempts: %v", err) + } + for _, a := range attempts { + for k := range a { + if !allowedAttempt[k] { + t.Errorf("unexpected attempt key %q in retry report", k) + } + } + } + } +} + +// provider is required and must survive as an empty string: an OCR_LLM_* +// endpoint has no provider name, and omitting the key there would make an +// unnamed endpoint indistinguishable from a missing field. +func TestRetryReportJSON_EmptyProviderKept(t *testing.T) { + raw, err := json.Marshal(retryReportFixture().Requests[0]) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if !bytes.Contains(raw, []byte(`"provider":""`)) { + t.Errorf("empty provider must still be emitted, got %s", raw) + } +} + +// Both exits read the same frozen value, so a single collector Freeze must +// render identically through the terminal and the JSON output. Built through +// the real collector rather than a literal so the rendered numbers are ones +// Freeze itself validated. +func TestRetryReport_TerminalAndJSONReadSameFrozenResult(t *testing.T) { + c := llm.NewRetryCollector() + base := time.Date(2026, 8, 7, 12, 0, 0, 0, time.UTC) + + recovered := llm.RequestMeta{Model: "claude-test", FilePath: "a.go", TaskType: "main_task", RequestNo: 1} + c.RecordAttempt(recovered, llm.AttemptRecord{ + ErrorClass: llm.ErrorClassRateLimited, FailurePhase: llm.FailurePhaseHTTP, StatusCode: 429, + }, base, base.Add(10*time.Millisecond)) + c.RecordAttempt(recovered, llm.AttemptRecord{}, base.Add(time.Second), base.Add(time.Second+10*time.Millisecond)) + c.Finalize(recovered, nil, false) + + failed := llm.RequestMeta{Model: "claude-test", FilePath: "b.go", TaskType: "main_task", RequestNo: 1} + c.RecordAttempt(failed, llm.AttemptRecord{ + ErrorClass: llm.ErrorClassProvider, FailurePhase: llm.FailurePhaseHTTP, StatusCode: 402, + }, base, base.Add(5*time.Millisecond)) + c.Finalize(failed, context.DeadlineExceeded, false) + + rep, err := c.Freeze("run-uuid") + if err != nil { + t.Fatalf("Freeze: %v", err) + } + if rep == nil { + t.Fatal("expected a report") + } + + var text bytes.Buffer + outputRetryReportText(&text, rep) + + ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StateComplete)} + jsonGot := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, rep); err != nil { + t.Fatalf("emitRunResult: %v", err) + } + }) + var out jsonOutput + if err := json.Unmarshal([]byte(jsonGot), &out); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if out.RetryReport == nil { + t.Fatal("retry_report missing") + } + + wantHeader := "LLM retry report: 1/2 requests retried, 1 retry, 1 recovered, 1 failed, 0 cancelled" + if !strings.Contains(text.String(), wantHeader) { + t.Errorf("terminal header = %q, want it to contain %q", text.String(), wantHeader) + } + if out.RetryReport.RetriedRequests != 1 || out.RetryReport.TotalRequests != 2 || + out.RetryReport.TotalRetries != 1 || out.RetryReport.RecoveredRequests != 1 || + out.RetryReport.FailedRequests != 1 { + t.Errorf("JSON aggregates disagree with the frozen report: %+v", out.RetryReport) + } + for _, r := range out.RetryReport.Requests { + if !strings.Contains(text.String(), r.FilePath) { + t.Errorf("%s listed in JSON but not in the terminal summary:\n%s", r.FilePath, text.String()) + } + } +} diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index f4d8244a..b26e8644 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -232,6 +232,24 @@ func executeReview(opts reviewOptions) error { comments, runErr := ag.Run(ctx) manifest := ag.RunManifest() + + // Freeze the retry report at the same boundary as the manifest: ag.Run has + // returned and joined its background work, so every request this run made is + // finalized and the report can no longer change. run_id is the session's + // in-memory UUID (ag.Session().SessionID) rather than ag.SessionID(), which + // returns "" when persistence failed — the report's logical_request_id must + // stay stable and unique per run even for an unpersisted session. + retryReport, freezeErr := rt.RetryCollector.Freeze(ag.Session().SessionID) + if freezeErr != nil { + // A construction error means the collector's invariants were violated, so + // the report is self-contradictory and must not be published at all + // (Freeze already returned nil). It joins the *publish* error, never + // runErr: the review itself may have fully succeeded, and folding it into + // runErr would make reviewResultError report a bogus "review failed", + // write a failure usage record and print a misleading --resume hint. + freezeErr = fmt.Errorf("freeze retry report: %w", freezeErr) + } + resultErr := reviewResultError(runErr, manifest) if resultErr != nil { span.SetStatus(codes.Error, resultErr.Error()) @@ -242,21 +260,30 @@ func executeReview(opts reviewOptions) error { // session delivery failed. Emit it first, then return the independent process // error so JSON consumers retain the complete coverage diagnosis. var emitErr error - if manifest != nil || runErr == nil { - emitErr = emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, q, llmIdentity) + emitted := manifest != nil || runErr == nil + if emitted { + emitErr = emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, q, llmIdentity, retryReport) if emitErr != nil { emitErr = fmt.Errorf("emit review result: %w", emitErr) } } if resultErr != nil { q.Restore() - emitFailureUsage(ag, time.Since(startTime), opts.outputFormat, llmIdentity) + // The report has exactly one exit per run. emitRunResult already published + // it whenever it ran (which it does even for a fully failed run, since a + // failed manifest is still publishable), so the failure-usage path gets it + // only when that call was skipped entirely. + failureReport := retryReport + if emitted { + failureReport = nil + } + emitFailureUsage(ag, time.Since(startTime), opts.outputFormat, llmIdentity, failureReport) if id := ag.SessionID(); id != "" { fmt.Fprintf(os.Stderr, "[ocr] Session: %s (retry with: --resume %s)\n", id, id) } - return errors.Join(resultErr, emitErr) + return errors.Join(resultErr, emitErr, freezeErr) } - return emitErr + return errors.Join(emitErr, freezeErr) } func reviewResultError(runErr error, manifest *session.RunManifest) error { diff --git a/cmd/opencodereview/scan_cmd.go b/cmd/opencodereview/scan_cmd.go index 07af88fd..9481a9cf 100644 --- a/cmd/opencodereview/scan_cmd.go +++ b/cmd/opencodereview/scan_cmd.go @@ -231,7 +231,7 @@ func executeScan(opts scanOptions) error { return fmt.Errorf("scan failed: %w", err) } - return emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, q, llmIdentity) + return emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, q, llmIdentity, nil) } func loadScanResumeState(repoDir string, opts scanOptions, scanPaths []string) (*session.ResumeState, error) { diff --git a/cmd/opencodereview/shared.go b/cmd/opencodereview/shared.go index 88c37533..ce502c7f 100644 --- a/cmd/opencodereview/shared.go +++ b/cmd/opencodereview/shared.go @@ -157,7 +157,14 @@ type llmRuntime struct { PlanToolDefs []llm.ToolDef MainToolDefs []llm.ToolDef Collector *tool.CommentCollector - AppCfg *Config + // RetryCollector observes every LLM HTTP attempt this run makes. It is + // created here rather than on the session or the agent because the client is + // built before either exists, and it is per-run rather than package-level so + // two runs in one process cannot share data. scan gets one too; its requests + // carry no RequestMeta, so every attempt is dropped and the frozen report is + // nil. + RetryCollector *llm.RetryCollector + AppCfg *Config // RuntimeConfig holds the allowlisted, non-secret runtime settings (protocol, // sanitized endpoint host, language, timeout) derived from the resolved // endpoint and app config, for the run manifest's runtime_config_sha256. It @@ -165,6 +172,13 @@ type llmRuntime struct { RuntimeConfig agent.RuntimeConfig } +// newRetryCollector builds the per-run retry collector. It is a variable so a +// test can hand back a collector whose invariants are already violated, which is +// the only way to exercise the Freeze construction-error branch from the +// outside: every production path finalizes every logical request on every exit, +// so a well-behaved run can never produce one. +var newRetryCollector = llm.NewRetryCollector + // loadLLMRuntime loads tool defs from toolConfigPath, reads the app config // from the user's default config path (applying the configured language to // tpl — defaulting when the config file is absent), resolves the LLM @@ -199,14 +213,17 @@ func loadLLMRuntime(tpl *template.Template, toolConfigPath string, resolveOpts l return nil, fmt.Errorf("resolve LLM endpoint: %w", err) } + retryCollector := newRetryCollector() + return &llmRuntime{ - Client: llm.NewLLMClient(ep), - Model: ep.Model, - Provider: ep.Provider, - PlanToolDefs: planToolDefs, - MainToolDefs: mainToolDefs, - Collector: tool.NewCommentCollector(), - AppCfg: appCfg, + Client: llm.NewLLMClient(ep, retryCollector), + Model: ep.Model, + Provider: ep.Provider, + PlanToolDefs: planToolDefs, + MainToolDefs: mainToolDefs, + Collector: tool.NewCommentCollector(), + RetryCollector: retryCollector, + AppCfg: appCfg, RuntimeConfig: agent.RuntimeConfig{ Protocol: ep.Protocol, EndpointHost: sanitizeEndpointHost(ep.URL), @@ -331,6 +348,13 @@ type resumeInfoProvider interface { // // q is the silencing handle returned by newQuietHandle; pass nil if no // silencing was set up (in which case the early restore is a no-op). +// +// retryReport is the frozen LLM retry report, or nil when there is nothing to +// report (a clean run, or a caller that produces no report at all — `ocr scan` +// never freezes one). It is passed as a parameter rather than added to +// ResultProvider because the collector belongs to llmRuntime, not to the +// agent; putting it on the interface would force internal/scan.Agent to +// implement a method that is always nil. func emitRunResult( ctx context.Context, ag ResultProvider, @@ -339,6 +363,7 @@ func emitRunResult( outputFormat, audience string, q *quietHandle, llmIdentity *jsonLLMIdentity, + retryReport *llm.RetryReport, ) error { comments = diff.ResolveLineNumbers(comments, ag.Diffs()) @@ -375,9 +400,13 @@ func emitRunResult( return outputJSONWithWarnings(comments, ag.Warnings(), ag.FilesReviewed(), ag.TotalInputTokens(), ag.TotalOutputTokens(), ag.TotalTokensUsed(), ag.TotalCacheReadTokens(), ag.TotalCacheWriteTokens(), duration, - ag.ProjectSummary(), ag.ToolCalls(), traceID, resumeInfo, ag.SessionID(), manifest, ag.BudgetExceeded(), llmIdentity) + ag.ProjectSummary(), ag.ToolCalls(), traceID, resumeInfo, ag.SessionID(), manifest, ag.BudgetExceeded(), llmIdentity, retryReport) } outputTextWithWarnings(comments, ag.Warnings(), manifest) + // Between the comments/warnings block and the project summary: the report is + // run-level diagnostics about how the comments were obtained, so it reads + // after them but must not separate the summary from the end of output. + outputRetryReportText(os.Stdout, retryReport) if summary := ag.ProjectSummary(); summary != "" { fmt.Printf("\n\n──────── Project Summary ────────\n\n%s\n", summary) } diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 4a11c571..bd35814c 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -113,8 +113,10 @@ type Args struct { // injected into plan and main_task prompts via {{requirement_background}}. Background string - // Model is the user-configured model name used as fallback when - // template phases (plan/memory_compression) don't specify one. + // Model is the resolved model name used by every LLM request this run + // makes. The template carries no per-phase model override, so plan, + // main_task, memory compression, re-location and review filter all send + // this value. Model string // Provider is the configured provider name (e.g. "openai", "anthropic", or a @@ -227,10 +229,35 @@ func New(args Args) *Agent { CommentWorkerPool: args.CommentWorkerPool, Session: args.Session, DiffLookup: a.findDiff, + // Non-nil only here: the same Runner serves scan, whose requests must + // stay out of the retry report. See newRequestMeta. + NewRequestMeta: a.newRequestMeta, }) return a } +// newRequestMeta builds the retry-report identity for one logical LLM request. +// +// It is the single place provider and model are read for that purpose — the +// llmloop Runner receives it as Deps.NewRequestMeta, and the two agent-local +// requests (plan, review filter) call it directly — so the two values cannot +// drift apart between the five review request types. +// +// filePath must be the same string passed to GetOrCreateFileSession and +// requestNo the RequestNo of the record created there, because those three +// fields plus taskType are how the report joins against the session JSONL. +// Provider is intentionally passed through as-is: empty is the real value for an +// unnamed endpoint, and must not be replaced by the protocol. +func (a *Agent) newRequestMeta(filePath string, taskType session.TaskType, requestNo int) llm.RequestMeta { + return llm.RequestMeta{ + Provider: a.args.Provider, + Model: a.args.Model, + FilePath: filePath, + TaskType: string(taskType), + RequestNo: requestNo, + } +} + // Run executes the full review pipeline: parse diffs -> plan per file -> LLM tool-loop -> collect comments. func (a *Agent) Run(ctx context.Context) ([]model.LlmComment, error) { // Step 1: Parse diffs @@ -323,6 +350,13 @@ func (a *Agent) Run(ctx context.Context) ([]model.LlmComment, error) { if len(comments) > 0 { telemetry.RecordCommentsGenerated(ctx, int64(len(comments))) } + // Join background memory compression before anything freezes run-level + // state. Those jobs are cancelled rather than awaited when a conversation + // ends, so their LLM request can still be in flight here; a retry report + // frozen at the command boundary would then see an un-finalized request + // and be discarded wholesale. Cheap in the normal case — every job has + // already been cancelled by now. + a.runner.WaitBackground() // Freeze coverage into the immutable manifest before session_end embeds it, // so the CLI and the persisted session serialize the identical object. A // persistence failure is a delivery error in its own right: when the review @@ -1258,9 +1292,10 @@ func (a *Agent) executeReviewFilter(ctx context.Context, d model.Diff, newPath s fs := a.session.GetOrCreateFileSession(newPath) rec := fs.AppendTaskRecord(session.ReviewFilterTask, messages) startTime := time.Now() + reqCtx := llm.WithRequestMeta(ctx, a.newRequestMeta(newPath, session.ReviewFilterTask, rec.RequestNo)) _, llmSpan := telemetry.StartLLMSpan(ctx, a.args.Model) - resp, err := a.args.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{ + resp, err := a.args.LLMClient.CompletionsWithCtx(reqCtx, llm.ChatRequest{ Model: a.args.Model, Messages: messages, MaxTokens: a.args.Template.CompletionTokenLimit(), @@ -1481,9 +1516,10 @@ func (a *Agent) executePlanPhase(ctx context.Context, newPath, rawDiff, changeFi fs := a.session.GetOrCreateFileSession(newPath) rec := fs.AppendTaskRecord(session.PlanTask, messages) startTime := time.Now() + reqCtx := llm.WithRequestMeta(ctx, a.newRequestMeta(newPath, session.PlanTask, rec.RequestNo)) _, llmSpan := telemetry.StartLLMSpan(ctx, a.args.Model) - resp, err := a.args.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{ + resp, err := a.args.LLMClient.CompletionsWithCtx(reqCtx, llm.ChatRequest{ Model: a.args.Model, Messages: messages, MaxTokens: a.args.Template.CompletionTokenLimit(), diff --git a/internal/agent/retry_identity_test.go b/internal/agent/retry_identity_test.go new file mode 100644 index 00000000..aa5d1973 --- /dev/null +++ b/internal/agent/retry_identity_test.go @@ -0,0 +1,179 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package agent + +import ( + "context" + "sync" + "testing" + + "github.com/alibaba/open-code-review/internal/config/template" + "github.com/alibaba/open-code-review/internal/llm" + "github.com/alibaba/open-code-review/internal/model" + "github.com/alibaba/open-code-review/internal/session" + "github.com/alibaba/open-code-review/internal/tool" +) + +// metaCaptureClient records the RequestMeta carried by each request's context. +// Identity travels in the context, so the client is the only place it becomes +// observable from outside package llm. +type metaCaptureClient struct { + mu sync.Mutex + metas []llm.RequestMeta + haveMeta []bool + reply string +} + +func (c *metaCaptureClient) CompletionsWithCtx(ctx context.Context, _ llm.ChatRequest) (*llm.ChatResponse, error) { + meta, ok := llm.RequestMetaFromContext(ctx) + c.mu.Lock() + c.metas = append(c.metas, meta) + c.haveMeta = append(c.haveMeta, ok) + c.mu.Unlock() + + reply := c.reply + return &llm.ChatResponse{ + Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &reply}}}, + Usage: &llm.UsageInfo{PromptTokens: 1, CompletionTokens: 1}, + }, nil +} + +func (c *metaCaptureClient) only(t *testing.T) (llm.RequestMeta, bool) { + t.Helper() + c.mu.Lock() + defer c.mu.Unlock() + if len(c.metas) != 1 { + t.Fatalf("got %d requests, want 1", len(c.metas)) + } + return c.metas[0], c.haveMeta[0] +} + +// TestExecutePlanPhase_Identity checks the plan request carries the identity of +// the record executePlanPhase created for it. The empty-provider case is covered +// too: an unnamed endpoint legitimately has no provider label, and that must not +// suppress identity. +func TestExecutePlanPhase_Identity(t *testing.T) { + for _, provider := range []string{"openai", ""} { + name := provider + if name == "" { + name = "empty-provider" + } + t.Run(name, func(t *testing.T) { + sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: "diff"}) + client := &metaCaptureClient{reply: "plan output"} + a := New(Args{ + LLMClient: client, + Provider: provider, + Model: "test", + Session: sess, + Template: template.Template{ + PlanTask: &template.LlmConversation{ + Messages: []template.ChatMessage{{Role: "user", Content: "plan {{diff}}"}}, + }, + MaxTokens: 10000, + MaxToolRequestTimes: 5, + MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "t"}}}, + }, + }) + a.currentDate = "2026-08-07 10:00" + + if _, err := a.executePlanPhase(context.Background(), "main.go", "+x", "", ""); err != nil { + t.Fatalf("executePlanPhase: %v", err) + } + + meta, ok := client.only(t) + if !ok { + t.Fatal("plan request carried no identity") + } + want := llm.RequestMeta{ + Provider: provider, + Model: "test", + FilePath: "main.go", + TaskType: string(session.PlanTask), + RequestNo: 1, + } + if meta != want { + t.Errorf("meta = %+v, want %+v", meta, want) + } + + // The report joins against the session JSONL, so the meta must match + // the record that was actually written, not merely look plausible. + recs := sess.GetOrCreateFileSession("main.go").TaskRecords[session.PlanTask] + if len(recs) != 1 { + t.Fatalf("session holds %d plan records, want 1", len(recs)) + } + if meta.RequestNo != recs[0].RequestNo { + t.Errorf("meta RequestNo = %d, record = %d", meta.RequestNo, recs[0].RequestNo) + } + }) + } +} + +// TestExecuteReviewFilter_Identity is the review-filter counterpart. The filter +// only runs when comments exist for the file, so one is seeded first. +func TestExecuteReviewFilter_Identity(t *testing.T) { + sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: "diff"}) + collector := tool.NewCommentCollector() + collector.Add(model.LlmComment{Path: "a.go", Content: "keep this"}) + + client := &metaCaptureClient{reply: `[]`} + a := New(Args{ + LLMClient: client, + Provider: "openai", + Model: "test", + Session: sess, + CommentCollector: collector, + Template: template.Template{ + ReviewFilterTask: &template.LlmConversation{ + Messages: []template.ChatMessage{{Role: "user", Content: "filter {{comments}} {{path}} {{diff}}"}}, + }, + MaxTokens: 10000, + MaxToolRequestTimes: 5, + MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "t"}}}, + }, + }) + + a.executeReviewFilter(context.Background(), model.Diff{NewPath: "a.go", Diff: "+x"}, "a.go") + + meta, ok := client.only(t) + if !ok { + t.Fatal("review filter request carried no identity") + } + want := llm.RequestMeta{ + Provider: "openai", + Model: "test", + FilePath: "a.go", + TaskType: string(session.ReviewFilterTask), + RequestNo: 1, + } + if meta != want { + t.Errorf("meta = %+v, want %+v", meta, want) + } + + recs := sess.GetOrCreateFileSession("a.go").TaskRecords[session.ReviewFilterTask] + if len(recs) != 1 { + t.Fatalf("session holds %d review filter records, want 1", len(recs)) + } + if meta.RequestNo != recs[0].RequestNo { + t.Errorf("meta RequestNo = %d, record = %d", meta.RequestNo, recs[0].RequestNo) + } +} + +// TestNewRequestMeta_IsSingleSourceOfProviderAndModel guards the reason the +// helper exists: five request types read provider and model through it, so a +// change to Args must not leave some of them behind. +func TestNewRequestMeta_IsSingleSourceOfProviderAndModel(t *testing.T) { + a := New(Args{Provider: "my-gateway", Model: "m1"}) + got := a.newRequestMeta("dir/f.go", session.MainTask, 3) + want := llm.RequestMeta{ + Provider: "my-gateway", + Model: "m1", + FilePath: "dir/f.go", + TaskType: string(session.MainTask), + RequestNo: 3, + } + if got != want { + t.Errorf("newRequestMeta = %+v, want %+v", got, want) + } +} diff --git a/internal/diff/relocation.go b/internal/diff/relocation.go index 14ed9320..9c3e8994 100644 --- a/internal/diff/relocation.go +++ b/internal/diff/relocation.go @@ -16,21 +16,18 @@ import ( "github.com/alibaba/open-code-review/internal/telemetry" ) -// ReLocateComment calls the LLM to regenerate a precise existing_code snippet -// when text-based matching fails, then retries ResolveComment with the new snippet. -// Returns (success, response, requestMessages) so the caller can record session -// history and track token usage. Response and messages are nil on early exits. -func ReLocateComment( - ctx context.Context, - cm *model.LlmComment, - d *model.Diff, - client llm.LLMClient, - task *template.LlmConversation, - modelName string, - maxTokens int, -) (bool, *llm.ChatResponse, []llm.Message) { +// BuildReLocationMessages renders the re-location prompt for cm against d. +// Returns nil when the task template is absent or empty, which the caller +// treats as "no re-location attempt": no session record, no request. +// +// This is split out of ReLocateComment so the caller can create the +// ReLocationTask session record — and therefore know its RequestNo — before any +// HTTP call happens. It is pure prompt construction: no client, no session, no +// request identity. Keeping it that way is what stops observability concerns +// from sinking into package diff. +func BuildReLocationMessages(cm *model.LlmComment, d *model.Diff, task *template.LlmConversation) []llm.Message { if task == nil || len(task.Messages) == 0 { - return false, nil, nil + return nil } messages := make([]llm.Message, 0, len(task.Messages)) @@ -41,6 +38,26 @@ func ReLocateComment( content = strings.ReplaceAll(content, "{suggestion_content}", cm.Content) messages = append(messages, llm.NewTextMessage(m.Role, content)) } + return messages +} + +// ReLocateComment calls the LLM to regenerate a precise existing_code snippet +// when text-based matching fails, then retries ResolveComment with the new +// snippet. messages comes from BuildReLocationMessages; the caller has already +// recorded it in the session, so only (success, response) are returned here. +// Response is nil when the request failed. +func ReLocateComment( + ctx context.Context, + cm *model.LlmComment, + d *model.Diff, + client llm.LLMClient, + messages []llm.Message, + modelName string, + maxTokens int, +) (bool, *llm.ChatResponse) { + if len(messages) == 0 { + return false, nil + } startTime := time.Now() _, llmSpan := telemetry.StartLLMSpan(ctx, modelName) @@ -54,7 +71,7 @@ func ReLocateComment( telemetry.RecordLLMResult(llmSpan, duration, 0, err) llmSpan.End() fmt.Fprintf(stdout.Writer(), "[ocr] Re-location LLM call failed for %s: %v\n", cm.Path, err) - return false, nil, messages + return false, nil } var totalTokens int64 if resp.Usage != nil { @@ -65,16 +82,16 @@ func ReLocateComment( code := extractCodeBlock(resp.Content()) if code == "" { - return false, resp, messages + return false, resp } original := cm.ExistingCode cm.ExistingCode = code if ResolveComment(cm, d) { - return true, resp, messages + return true, resp } cm.ExistingCode = original - return false, resp, messages + return false, resp } // extractCodeBlock extracts the content of the first fenced code block from text. diff --git a/internal/diff/relocation_test.go b/internal/diff/relocation_test.go index 763faf4a..3df0cf52 100644 --- a/internal/diff/relocation_test.go +++ b/internal/diff/relocation_test.go @@ -14,11 +14,13 @@ import ( ) type mockLLMClient struct { - response *llm.ChatResponse - err error + response *llm.ChatResponse + err error + callCount int } func (m *mockLLMClient) CompletionsWithCtx(_ context.Context, req llm.ChatRequest) (*llm.ChatResponse, error) { + m.callCount++ return m.response, m.err } @@ -110,16 +112,18 @@ func TestReLocateComment_LLMReturnsValidCode(t *testing.T) { response: newMockResponse("Here is the code:\n```go\nx := 1\ny := 2\n```\n"), } - ok, resp, msgs := ReLocateComment(context.Background(), &cm, d, client, makeTask(), "test-model", 1000) + msgs := BuildReLocationMessages(&cm, d, makeTask()) + if len(msgs) == 0 { + t.Fatal("expected non-empty messages") + } + + ok, resp := ReLocateComment(context.Background(), &cm, d, client, msgs, "test-model", 1000) if !ok { t.Fatal("expected re-location to succeed") } if resp == nil { t.Fatal("expected non-nil response") } - if len(msgs) == 0 { - t.Fatal("expected non-empty messages") - } if cm.StartLine == 0 || cm.EndLine == 0 { t.Fatalf("expected non-zero lines after re-location, got %d-%d", cm.StartLine, cm.EndLine) } @@ -137,16 +141,13 @@ func TestReLocateComment_LLMReturnsInvalidContent(t *testing.T) { response: newMockResponse("I cannot find the code."), } - ok, resp, msgs := ReLocateComment(context.Background(), &cm, d, client, makeTask(), "test-model", 1000) + ok, resp := ReLocateComment(context.Background(), &cm, d, client, BuildReLocationMessages(&cm, d, makeTask()), "test-model", 1000) if ok { t.Fatal("expected re-location to fail for invalid LLM response") } if resp == nil { t.Fatal("expected non-nil response even on failure") } - if len(msgs) == 0 { - t.Fatal("expected non-empty messages") - } if cm.StartLine != 0 || cm.EndLine != 0 { t.Fatal("lines should remain 0-0") } @@ -162,36 +163,112 @@ func TestReLocateComment_LLMError(t *testing.T) { client := &mockLLMClient{err: errors.New("network error")} - ok, resp, msgs := ReLocateComment(context.Background(), &cm, d, client, makeTask(), "test-model", 1000) + ok, resp := ReLocateComment(context.Background(), &cm, d, client, BuildReLocationMessages(&cm, d, makeTask()), "test-model", 1000) if ok { t.Fatal("expected false on LLM error") } if resp != nil { t.Fatal("expected nil response on error") } - if len(msgs) == 0 { - t.Fatal("expected non-empty messages even on error") +} + +// TestBuildReLocationMessages_Rendering pins what the split moved out of +// ReLocateComment: the prompt the model receives, byte for byte. Splitting the +// function was for request ordering, so the rendering must be unchanged. +func TestBuildReLocationMessages_Rendering(t *testing.T) { + cm := model.LlmComment{ + Path: "main.go", + Content: "unused variable", + ExistingCode: "x := 1", + } + d := makeDiff() + task := &template.LlmConversation{ + Messages: []template.ChatMessage{ + {Role: "system", Content: "you are a helper"}, + {Role: "user", Content: "diff:\n{diff}\ncode:\n{existing_code}\nsuggestion:\n{suggestion_content}"}, + }, + } + + msgs := BuildReLocationMessages(&cm, d, task) + if len(msgs) != 2 { + t.Fatalf("got %d messages, want 2", len(msgs)) + } + if msgs[0].Role != "system" || msgs[0].ExtractText() != "you are a helper" { + t.Errorf("message 0 = %q/%q", msgs[0].Role, msgs[0].ExtractText()) + } + want := "diff:\n" + d.Diff + "\ncode:\nx := 1\nsuggestion:\nunused variable" + if msgs[1].Role != "user" || msgs[1].ExtractText() != want { + t.Errorf("message 1 = %q/%q, want user/%q", msgs[1].Role, msgs[1].ExtractText(), want) + } +} + +func TestBuildReLocationMessages_NilOrEmptyTask(t *testing.T) { + cm := model.LlmComment{ + Path: "main.go", + Content: "test", + ExistingCode: "bad code", + } + d := makeDiff() + + if msgs := BuildReLocationMessages(&cm, d, nil); msgs != nil { + t.Fatalf("expected nil messages for nil task, got %d", len(msgs)) + } + if msgs := BuildReLocationMessages(&cm, d, &template.LlmConversation{}); msgs != nil { + t.Fatalf("expected nil messages for task without messages, got %d", len(msgs)) + } +} + +// TestReLocateComment_CodeBlockStillUnresolvable covers the rollback branch: the +// model returned a well-formed snippet that still does not appear in the diff, so +// the original ExistingCode must be restored rather than left overwritten. +func TestReLocateComment_CodeBlockStillUnresolvable(t *testing.T) { + const original = "totally wrong code" + cm := model.LlmComment{ + Path: "main.go", + Content: "unused variable", + ExistingCode: original, + } + d := makeDiff() + + client := &mockLLMClient{ + response: newMockResponse("```go\nnot in the diff either\n```"), + } + + ok, resp := ReLocateComment(context.Background(), &cm, d, client, BuildReLocationMessages(&cm, d, makeTask()), "test-model", 1000) + if ok { + t.Fatal("expected false when the new snippet still does not match") + } + if resp == nil { + t.Fatal("expected non-nil response") + } + if cm.ExistingCode != original { + t.Errorf("ExistingCode = %q, want the original %q restored", cm.ExistingCode, original) + } + if cm.StartLine != 0 || cm.EndLine != 0 { + t.Errorf("lines = %d-%d, want 0-0", cm.StartLine, cm.EndLine) } } -func TestReLocateComment_NilTask(t *testing.T) { +// The caller skips the session record and the request entirely when there are no +// messages, so ReLocateComment must not reach the client on that path. +func TestReLocateComment_NoMessages(t *testing.T) { cm := model.LlmComment{ Path: "main.go", Content: "test", ExistingCode: "bad code", } d := makeDiff() - client := &mockLLMClient{} + client := &mockLLMClient{response: newMockResponse("```go\nx := 1\n```")} - ok, resp, msgs := ReLocateComment(context.Background(), &cm, d, client, nil, "test-model", 1000) + ok, resp := ReLocateComment(context.Background(), &cm, d, client, nil, "test-model", 1000) if ok { - t.Fatal("expected false when task is nil") + t.Fatal("expected false when there are no messages") } if resp != nil { - t.Fatal("expected nil response when task is nil") + t.Fatal("expected nil response when there are no messages") } - if msgs != nil { - t.Fatal("expected nil messages when task is nil") + if client.callCount != 0 { + t.Fatalf("expected no LLM call, got %d", client.callCount) } } diff --git a/internal/llm/client.go b/internal/llm/client.go index b9afe944..54cb307e 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -205,6 +205,16 @@ type ClientConfig struct { ExtraBody map[string]any // Vendor-specific fields merged into every request body ExtraHeaders map[string]string // Extra HTTP headers sent with every request RetryCodes []int // Additional HTTP status codes that trigger retry + + // retryCollector receives one record per real HTTP attempt. It is + // unexported because it is not configuration: it is a handle on the current + // run, owned by llmRuntime and set only by NewLLMClient, and the three + // exported constructors keep their signatures because of it. + // + // A nil collector is fully inert: no middleware is mounted and nothing about + // the request path changes. That is the state for llm test, and for any + // caller that builds a client without one. + retryCollector *RetryCollector } // retryCodesMiddleware returns an HTTP middleware that forces the SDK to retry @@ -243,16 +253,21 @@ func retryCodesMiddleware(codes []int) func(*http.Request, func(*http.Request) ( // The defensive default keeps legacy callers that somehow bypass resolver // normalization working (they previously got OpenAIClient for any non-anthropic // protocol). -func NewLLMClient(ep ResolvedEndpoint) LLMClient { +// +// collector observes every HTTP attempt the returned client makes; pass nil to +// build a client that is not observed. It is a parameter rather than a field on +// ResolvedEndpoint because it belongs to the run, not to the endpoint. +func NewLLMClient(ep ResolvedEndpoint, collector *RetryCollector) LLMClient { cfg := ClientConfig{ - URL: ep.URL, - APIKey: ep.Token, - Model: ep.Model, - AuthHeader: ep.AuthHeader, - Timeout: ep.Timeout, - ExtraBody: ep.ExtraBody, - ExtraHeaders: ep.ExtraHeaders, - RetryCodes: ep.RetryCodes, + URL: ep.URL, + APIKey: ep.Token, + Model: ep.Model, + AuthHeader: ep.AuthHeader, + Timeout: ep.Timeout, + ExtraBody: ep.ExtraBody, + ExtraHeaders: ep.ExtraHeaders, + RetryCodes: ep.RetryCodes, + retryCollector: collector, } switch ep.Protocol { case ProtocolAnthropic: @@ -363,6 +378,9 @@ func NewOpenAIClient(cfg ClientConfig) *OpenAIClient { if mw := retryCodesMiddleware(cfg.RetryCodes); mw != nil { opts = append(opts, openaiopt.WithMiddleware(mw)) } + if cfg.retryCollector != nil { + opts = append(opts, openaiopt.WithMiddleware(newRetryObserver(cfg.retryCollector))) + } return &OpenAIClient{ cfg: cfg, @@ -381,7 +399,23 @@ type ChatRequest struct { } // CompletionsWithCtx sends a chat completion request with context support for cancellation and timeout. -func (c *OpenAIClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) (*ChatResponse, error) { +// +// The deferred finalizeRequest is the client boundary for the retry report: it is +// the only place that knows the logical request is over, and it covers every exit +// path including the streaming branch, the EOF recovery and a panic. Results are +// named so the defer can read the error actually returned. +func (c *OpenAIClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) (resp *ChatResponse, err error) { + defer func() { + // A panic still has to finalize, or the entry stays unfinalized and Freeze + // drops the whole run's report. The panic value itself is re-raised + // unchanged so agent.go's per-file recovery behaves exactly as before. + if r := recover(); r != nil { + finalizeRequest(ctx, c.cfg.retryCollector, errRequestPanicked) + panic(r) + } + finalizeRequest(ctx, c.cfg.retryCollector, err) + }() + model := req.Model if model == "" { model = c.cfg.Model @@ -408,6 +442,15 @@ func (c *OpenAIClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) sdkResp, err := c.sdk.Chat.Completions.New(ctx, params, opts...) if errors.Is(err, io.ErrUnexpectedEOF) { + // The truncated response was observed as HTTP 200, so it is corrected here + // rather than in the defer: this SDK call ends now, and a second one is + // about to append its own attempts under the same logical request. + // + // Both corrections sit ahead of their ctx early return. Placing them after + // would leave a truncated attempt recorded as a success whenever the parent + // context was cancelled in between — no invariant would complain, since a + // cancelled request needs no error attempt, but the record would be wrong. + reviseAttempt(ctx, c.cfg.retryCollector, ErrorClassNetwork, FailurePhaseResponseDecode) if ctxErr := ctx.Err(); ctxErr != nil { return nil, ctxErr } @@ -416,6 +459,9 @@ func (c *OpenAIClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) sdkResp = retryResp err = nil } else { + if errors.Is(retryErr, io.ErrUnexpectedEOF) { + reviseAttempt(ctx, c.cfg.retryCollector, ErrorClassNetwork, FailurePhaseResponseDecode) + } if ctxErr := ctx.Err(); ctxErr != nil { return nil, ctxErr } @@ -431,7 +477,29 @@ func (c *OpenAIClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) return c.mapOpenAIResponse(sdkResp), nil } +// completionsStreaming consumes an SSE completion and corrects the retry report +// when the stream fails after it was already established. +// +// The correction lives in this one wrapper instead of at the inner function's +// four error returns: a mid-stream failure is invisible to the observer, which +// saw only the HTTP 200 that opened the stream. It must not finalize the logical +// request — CompletionsWithCtx returns this call directly, so its defer already +// does, and a second Finalize would be recorded as a violation. +// +// A stream that never opened needs nothing here: stream.Err() then carries the +// non-2xx *apierror.Error, the observer already recorded that attempt as an +// error, and ReviseLastAttempt's precondition makes this a no-op. func (c *OpenAIClient) completionsStreaming(ctx context.Context, params openai.ChatCompletionNewParams, opts ...openaiopt.RequestOption) (*ChatResponse, error) { + resp, err := c.completionsStreamingInner(ctx, params, opts...) + if err != nil { + class, phase := classifyStreamError(err) + reviseAttempt(ctx, c.cfg.retryCollector, class, phase) + return nil, err + } + return resp, nil +} + +func (c *OpenAIClient) completionsStreamingInner(ctx context.Context, params openai.ChatCompletionNewParams, opts ...openaiopt.RequestOption) (*ChatResponse, error) { stream := c.sdk.Chat.Completions.NewStreaming(ctx, params, opts...) defer stream.Close() @@ -474,18 +542,18 @@ func (c *OpenAIClient) completionsStreaming(ctx context.Context, params openai.C builder.WriteString(reasoningContent) } if !accumulator.AddChunk(chunk) { - return nil, fmt.Errorf("OpenAI streaming response contained inconsistent chunks") + return nil, &streamIntegrityError{reason: "contained inconsistent chunks"} } } if err := stream.Err(); err != nil { return nil, err } if len(choiceOrder) == 0 { - return nil, fmt.Errorf("OpenAI streaming response contained no choices") + return nil, &streamIntegrityError{reason: "contained no choices"} } for _, index := range choiceOrder { if !finishedChoices[index] { - return nil, fmt.Errorf("OpenAI streaming response ended before choice %d finished", index) + return nil, &streamIntegrityError{reason: fmt.Sprintf("ended before choice %d finished", index)} } } @@ -685,6 +753,9 @@ func NewAnthropicClient(cfg ClientConfig) *AnthropicClient { if mw := retryCodesMiddleware(cfg.RetryCodes); mw != nil { opts = append(opts, option.WithMiddleware(mw)) } + if cfg.retryCollector != nil { + opts = append(opts, option.WithMiddleware(newRetryObserver(cfg.retryCollector))) + } return &AnthropicClient{ cfg: cfg, @@ -693,7 +764,20 @@ func NewAnthropicClient(cfg ClientConfig) *AnthropicClient { } // CompletionsWithCtx sends a chat completion request with context support. -func (c *AnthropicClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) (*ChatResponse, error) { +// +// The deferred finalizeRequest is this client's boundary for the retry report; +// see the OpenAI counterpart for why it is deferred and why the results are +// named. A parameter-building failure returns before any HTTP attempt, so +// Finalize finds no entry and the request stays out of the report entirely. +func (c *AnthropicClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) (resp *ChatResponse, err error) { + defer func() { + if r := recover(); r != nil { + finalizeRequest(ctx, c.cfg.retryCollector, errRequestPanicked) + panic(r) + } + finalizeRequest(ctx, c.cfg.retryCollector, err) + }() + model := req.Model if model == "" { model = c.cfg.Model diff --git a/internal/llm/client_test.go b/internal/llm/client_test.go index d9dfb941..6315456d 100644 --- a/internal/llm/client_test.go +++ b/internal/llm/client_test.go @@ -1336,7 +1336,7 @@ func TestNewLLMClient_Dispatch(t *testing.T) { Model: "test-model", Protocol: tt.protocol, } - client := NewLLMClient(ep) + client := NewLLMClient(ep, nil) got := typeName(client) if got != tt.want { t.Errorf("NewLLMClient(protocol=%q) = %s, want %s", tt.protocol, got, tt.want) @@ -1355,7 +1355,7 @@ func TestNewLLMClient_OpenAIAliasDispatchesToOpenAIClient(t *testing.T) { Model: "test-model", Protocol: NormalizeProtocol("openai"), } - client := NewLLMClient(ep) + client := NewLLMClient(ep, nil) if got := typeName(client); got != "*llm.OpenAIClient" { t.Errorf("NormalizeProtocol(\"openai\") dispatched to %s, want *llm.OpenAIClient", got) } diff --git a/internal/llm/resolver_test.go b/internal/llm/resolver_test.go index cc42f14b..8c7f7054 100644 --- a/internal/llm/resolver_test.go +++ b/internal/llm/resolver_test.go @@ -1757,7 +1757,7 @@ func TestNewLLMClient_TimeoutForwarded(t *testing.T) { Timeout: 2 * time.Minute, } - client := NewLLMClient(ep) + client := NewLLMClient(ep, nil) if client == nil { t.Fatal("NewLLMClient returned nil") } @@ -1781,7 +1781,7 @@ func TestNewLLMClient_DefaultTimeout(t *testing.T) { // Timeout not set — should default to 5 minutes } - client := NewLLMClient(ep) + client := NewLLMClient(ep, nil) if oc, ok := client.(*OpenAIClient); ok { if oc.cfg.Timeout != 5*time.Minute { t.Errorf("OpenAIClient cfg.Timeout = %v, want default %v", oc.cfg.Timeout, 5*time.Minute) diff --git a/internal/llm/responses_client.go b/internal/llm/responses_client.go index a8438ecd..1f050ea9 100644 --- a/internal/llm/responses_client.go +++ b/internal/llm/responses_client.go @@ -49,6 +49,9 @@ func NewOpenAIResponsesClient(cfg ClientConfig) *OpenAIResponsesClient { if mw := retryCodesMiddleware(cfg.RetryCodes); mw != nil { opts = append(opts, openaiopt.WithMiddleware(mw)) } + if cfg.retryCollector != nil { + opts = append(opts, openaiopt.WithMiddleware(newRetryObserver(cfg.retryCollector))) + } return &OpenAIResponsesClient{ cfg: cfg, @@ -77,7 +80,19 @@ func ensureResponsesEndpoint(cfg *ClientConfig) { // CompletionsWithCtx sends a Responses API request and maps the result back to // the shared ChatResponse shape. -func (c *OpenAIResponsesClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) (*ChatResponse, error) { +// +// The deferred finalizeRequest is this client's boundary for the retry report; +// see the OpenAI Chat Completions counterpart for why it is deferred and why the +// results are named. +func (c *OpenAIResponsesClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) (resp *ChatResponse, err error) { + defer func() { + if r := recover(); r != nil { + finalizeRequest(ctx, c.cfg.retryCollector, errRequestPanicked) + panic(r) + } + finalizeRequest(ctx, c.cfg.retryCollector, err) + }() + model := req.Model if model == "" { model = c.cfg.Model @@ -111,9 +126,18 @@ func (c *OpenAIResponsesClient) CompletionsWithCtx(ctx context.Context, req Chat // a dead response as success. switch sdkResp.Status { case responses.ResponseStatusFailed, responses.ResponseStatusCancelled: - return nil, fmt.Errorf("openai-responses request did not complete: status=%s", sdkResp.Status) + err = fmt.Errorf("openai-responses request did not complete: status=%s", sdkResp.Status) case responses.ResponseStatusQueued, responses.ResponseStatusInProgress: - return nil, fmt.Errorf("openai-responses returned non-terminal status=%s (background/async mode is not supported)", sdkResp.Status) + err = fmt.Errorf("openai-responses returned non-terminal status=%s (background/async mode is not supported)", sdkResp.Status) + } + if err != nil { + // Correct the attempt here, where the status is known. The observer saw + // only the HTTP 200 that carried this dead response object, and nothing + // downstream would catch the omission: a request whose outcome is failed is + // listed with no error attempt at all, producing self-consistent counts + // over a record that misstates what happened. + reviseAttempt(ctx, c.cfg.retryCollector, ErrorClassProvider, FailurePhaseResponseStatus) + return nil, err } return c.mapResponsesResponse(sdkResp), nil diff --git a/internal/llm/retry_boundary.go b/internal/llm/retry_boundary.go new file mode 100644 index 00000000..a2916611 --- /dev/null +++ b/internal/llm/retry_boundary.go @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "encoding/json" + "errors" + "io" + + "github.com/openai/openai-go/v3/packages/ssestream" +) + +// errRequestPanicked finalizes a logical request whose client call panicked. +// +// The panic itself is re-raised unchanged; this sentinel exists only so the +// request has a recorded outcome. Without it the entry stays unfinalized and +// Freeze fails for the whole run: agent.go recovers per file and keeps going, so +// one panicking file would erase every other file's retry records. +var errRequestPanicked = errors.New("llm request panicked") + +// streamIntegrityError is a stream-completeness failure detected by OCR itself +// rather than reported by the transport or the SDK. +// +// It exists because classification may only read HTTP status and Go error types, +// never message text. The three conditions it carries used to be bare +// fmt.Errorf values, which were indistinguishable from any other error at the +// client boundary. The rendered messages are unchanged. +type streamIntegrityError struct{ reason string } + +func (e *streamIntegrityError) Error() string { return "OpenAI streaming response " + e.reason } + +// classifyBoundaryError classifies an error that only surfaced after the SDK +// retry loop finished, and reports whether it recognized it at all. +// +// The false return is the point: an unrecognized error is left alone rather than +// bucketed as unknown, because the only remaining way to tell errors apart here +// would be their message text. A missing correction leaves the attempt as the +// observed HTTP 200, which is at least a fact; a guessed one is not. +// +// Both JSON error types are matched with errors.As because the SDKs wrap them +// ("error parsing response json: %w"). Decoding happens after the retry loop in +// both SDKs, so this path never coincides with an SDK retry. +func classifyBoundaryError(err error) (ErrorClass, FailurePhase, bool) { + switch { + case err == nil: + return "", "", false + case errors.Is(err, context.Canceled): + return ErrorClassCancelled, FailurePhaseContext, true + case errors.Is(err, context.DeadlineExceeded): + return ErrorClassTimeout, FailurePhaseContext, true + case errors.Is(err, io.ErrUnexpectedEOF): + return ErrorClassNetwork, FailurePhaseResponseDecode, true + } + + var syntaxErr *json.SyntaxError + var typeErr *json.UnmarshalTypeError + if errors.As(err, &syntaxErr) || errors.As(err, &typeErr) { + return ErrorClassUnknown, FailurePhaseResponseDecode, true + } + return "", "", false +} + +// classifyStreamError classifies a failure that surfaced while consuming an +// already-established SSE stream. Unlike classifyBoundaryError it always returns +// a classification, because the caller already knows the failure came from the +// stream. +// +// Context errors keep FailurePhaseContext: cancelling mid-stream fails at the +// context, not in the stream, and the class and phase are independent enough +// that no cancelled/stream pairing is needed. +// +// Anything else is unknown/stream rather than the network/transport default that +// classifyAttempt would give. That default is sound for an opaque error at the +// transport phase, where the network stack is the overwhelmingly likely source; +// after HTTP 200 the same error could just as easily come from decoding or from +// the provider, so calling it network would be an unverified claim about the +// transport. +func classifyStreamError(err error) (ErrorClass, FailurePhase) { + var integrityErr *streamIntegrityError + var streamErr *ssestream.StreamError + switch { + case errors.As(err, &integrityErr), errors.As(err, &streamErr): + return ErrorClassProvider, FailurePhaseStream + case errors.Is(err, context.Canceled): + return ErrorClassCancelled, FailurePhaseContext + case errors.Is(err, context.DeadlineExceeded): + return ErrorClassTimeout, FailurePhaseContext + } + return ErrorClassUnknown, FailurePhaseStream +} + +// reviseAttempt applies a client-boundary correction to the last attempt of the +// logical request identified by ctx. +// +// Requests without identity (scan, llm test) and a nil collector are no-ops, so +// no call site needs to check either. The correction is idempotent by way of +// ReviseLastAttempt's precondition: once the last attempt is an error, every +// later correction does nothing, which is what lets the inline stream and +// Responses corrections coexist with the generic one in the boundary defer. +func reviseAttempt(ctx context.Context, collector *RetryCollector, class ErrorClass, phase FailurePhase) { + if collector == nil { + return + } + meta, ok := RequestMetaFromContext(ctx) + if !ok { + return + } + collector.ReviseLastAttempt(meta, class, phase) +} + +// finalizeRequest is the shared body of the client-boundary defer: correct the +// last attempt if the error only became visible after the SDK returned, then +// decide the request-level outcome. +// +// The order is load-bearing. Finalizing first would make the correction a +// "revised after Finalize" violation, which fails Freeze and drops the entire +// run's report — strictly worse than not correcting at all. +// +// parentCancelled reads the context the business layer passed in, and only +// context.Canceled counts: the per-attempt deadline from WithRequestTimeout must +// surface as failed, since attempt-level timeout already expresses it and +// conflating it with a user abort would misreport why the run stopped. +// +// Callers must invoke this exactly once per logical request. A second call is +// recorded as a violation, which is why only CompletionsWithCtx defers it and +// completionsStreaming must not defer its own. +func finalizeRequest(ctx context.Context, collector *RetryCollector, reqErr error) { + if collector == nil { + return + } + meta, ok := RequestMetaFromContext(ctx) + if !ok { + return + } + if class, phase, recognized := classifyBoundaryError(reqErr); recognized { + collector.ReviseLastAttempt(meta, class, phase) + } + collector.Finalize(meta, reqErr, errors.Is(ctx.Err(), context.Canceled)) +} diff --git a/internal/llm/retry_boundary_test.go b/internal/llm/retry_boundary_test.go new file mode 100644 index 00000000..1a1cc71f --- /dev/null +++ b/internal/llm/retry_boundary_test.go @@ -0,0 +1,733 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/openai/openai-go/v3/packages/ssestream" +) + +// The four blind spots share one shape: the observer sees HTTP 200, the failure +// surfaces afterwards, and nothing downstream would notice the omission — +// validateReport does not require an error attempt under outcome failed. So each +// path gets its own server and its own test rather than a shared fixture; a +// single table would let one missing correction hide behind another's coverage. + +func openAIClient(url string, c *RetryCollector, extraBody map[string]any) *OpenAIClient { + return NewOpenAIClient(ClientConfig{ + URL: url + "/v1", + APIKey: "test-key", + Model: "gpt-test", + ExtraBody: extraBody, + retryCollector: c, + }) +} + +func responsesClient(url string, c *RetryCollector) *OpenAIResponsesClient { + return NewOpenAIResponsesClient(ClientConfig{ + URL: url + "/v1", + APIKey: "test-key", + Model: "gpt-test", + retryCollector: c, + }) +} + +// freezeOne freezes c and returns the single listed request. +func freezeOne(t *testing.T, c *RetryCollector) RequestReport { + t.Helper() + rep, err := c.Freeze("test-run-id") + if err != nil { + t.Fatalf("Freeze: %v", err) + } + if rep == nil { + t.Fatal("Freeze returned no report, want one") + } + if len(rep.Requests) != 1 { + t.Fatalf("Freeze listed %d requests, want 1", len(rep.Requests)) + } + return rep.Requests[0] +} + +// --- blind spot 1: truncated body after HTTP 200 --- + +// A body shorter than its Content-Length fails during the read that happens +// after the SDK's retry loop, so the SDK never retries it and the observer only +// ever saw the 200. This is also the three-attempt sequence from the design's +// timing diagram: 429, truncated 200, then the client's own re-call. It pins +// numbering across two SDK calls under one logical request, which is why the +// correction cannot assert "the first EOF corrects attempt 1". +func TestBoundaryCorrectsTruncatedResponse(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch requests.Add(1) { + case 1: + w.Header().Set("Retry-After-Ms", "0") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"error":{"message":"slow down","type":"rate_limit_error"}}`) + case 2: + // Headers, then a pause, then fewer bytes than announced. The pause + // is what makes attempt 3's observed_backoff_ms measurable: the + // attempt ends when headers arrive, so the gap covers the stalled + // body read plus the client-side re-call. + w.Header().Set("Content-Length", "4096") + w.WriteHeader(http.StatusOK) + w.(http.Flusher).Flush() + time.Sleep(60 * time.Millisecond) + _, _ = fmt.Fprint(w, openAIOKBody[:20]) + default: + _, _ = fmt.Fprint(w, openAIOKBody) + } + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + if _, err := ping(metaCtx(m), openAIClient(server.URL, c, nil)); err != nil { + t.Fatalf("CompletionsWithCtx: %v", err) + } + + got := attemptsFor(t, c, m) + if len(got) != 3 { + t.Fatalf("got %d attempts, want 3 (429, truncated 200, re-call)", len(got)) + } + for i, a := range got { + if a.Number != i+1 { + t.Errorf("attempt %d numbered %d — numbering must not restart with the second SDK call", i+1, a.Number) + } + } + if got[0].ErrorClass != ErrorClassRateLimited { + t.Errorf("attempt 1 = %s, want rate_limited", got[0].ErrorClass) + } + if got[1].Outcome != AttemptError || got[1].ErrorClass != ErrorClassNetwork || got[1].FailurePhase != FailurePhaseResponseDecode { + t.Errorf("attempt 2 = %s %s/%s, want error network/response_decode", + got[1].Outcome, got[1].ErrorClass, got[1].FailurePhase) + } + if got[1].StatusCode != http.StatusOK { + t.Errorf("attempt 2 status_code = %d, want the observed 200 kept", got[1].StatusCode) + } + if got[2].Outcome != AttemptSuccess { + t.Errorf("attempt 3 = %+v, want success", got[2]) + } + // The re-call is not an SDK backoff, but the field only ever claimed to be a + // measured interval. Fixed here so it is not "fixed" into a zero later. + if got[2].ObservedBackoffMS < 30 { + t.Errorf("attempt 3 observed_backoff_ms = %d, want >= 30 (the server stalled 60ms before truncating)", + got[2].ObservedBackoffMS) + } + + if req := freezeOne(t, c); req.Outcome != OutcomeRecovered { + t.Errorf("outcome = %s, want recovered", req.Outcome) + } +} + +// The truncation correction is applied before the EOF branch consults ctx, so a +// request that ends cancelled still shows why the 200 was not usable. Nothing +// would flag the alternative: a cancelled request is not required to carry an +// error attempt, so the report would simply claim the truncated 200 was fine. +// +// The cancellation is placed in the re-call rather than between the two SDK +// calls: a body read that is racing a cancel returns whichever arrives first, so +// asserting on the gap itself could only ever be flaky. +func TestBoundaryKeepsTruncationCorrectionWhenRecallIsCancelled(t *testing.T) { + var requests atomic.Int32 + release := make(chan struct{}) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if requests.Add(1) == 1 { + w.Header().Set("Content-Length", "4096") + w.WriteHeader(http.StatusOK) + _, _ = fmt.Fprint(w, openAIOKBody[:20]) + return + } + // Hold the re-call open until the test is done with it. An HTTP/1 server + // does not cancel r.Context() while a handler is running, so waiting on + // that instead would block Close forever. + <-release + })) + defer server.Close() + defer close(release) + + c := NewRetryCollector() + m := testMeta() + ctx, cancel := context.WithCancel(metaCtx(m)) + defer cancel() + stop := time.AfterFunc(200*time.Millisecond, cancel) + defer stop.Stop() + + _, err := ping(ctx, openAIClient(server.URL, c, nil)) + if !errors.Is(err, context.Canceled) { + t.Fatalf("CompletionsWithCtx err = %v, want context.Canceled", err) + } + + got := attemptsFor(t, c, m) + if len(got) < 1 { + t.Fatal("no attempts recorded") + } + if got[0].Outcome != AttemptError || got[0].ErrorClass != ErrorClassNetwork || got[0].FailurePhase != FailurePhaseResponseDecode { + t.Errorf("attempt 1 = %s %s/%s, want error network/response_decode kept through the cancellation", + got[0].Outcome, got[0].ErrorClass, got[0].FailurePhase) + } + + if req := freezeOne(t, c); req.Outcome != OutcomeCancelled { + t.Errorf("outcome = %s, want cancelled", req.Outcome) + } +} + +// --- blind spot 2: any other decode failure after HTTP 200 --- + +// A 200 whose body is not JSON at all fails in the SDK's post-retry-loop decode +// and arrives wrapped ("error parsing response json: %w"), so errors.As finds the +// *json.SyntaxError. The class is unknown rather than network: only the JSON +// error type is known here, and inferring more would mean reading the message. +func TestBoundaryCorrectsDecodeFailure(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Content-Type must stay JSON: with anything else the SDK reports a + // destination-type mismatch instead, which is a different error entirely. + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, "not json at all") + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + _, err := ping(metaCtx(m), anthropicClient(server.URL, c, nil)) + if err == nil { + t.Fatal("CompletionsWithCtx succeeded, want a decode error") + } + + got := attemptsFor(t, c, m) + if len(got) != 1 { + t.Fatalf("got %d attempts, want 1 (a decode failure is never retried)", len(got)) + } + if got[0].Outcome != AttemptError || got[0].ErrorClass != ErrorClassUnknown || got[0].FailurePhase != FailurePhaseResponseDecode { + t.Errorf("attempt 1 = %s %s/%s, want error unknown/response_decode", + got[0].Outcome, got[0].ErrorClass, got[0].FailurePhase) + } + + if req := freezeOne(t, c); req.Outcome != OutcomeFailed { + t.Errorf("outcome = %s, want failed", req.Outcome) + } +} + +// --- blind spot 3: a stream that fails after it opened --- + +// The stream opens with HTTP 200 and then ends without any choice reaching a +// finish_reason. OCR detects that itself, which is why the three integrity +// conditions carry a dedicated error type: a bare fmt.Errorf would be +// indistinguishable from every other error under "classify by Go type only". +func TestBoundaryCorrectsMidStreamFailure(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + _, _ = fmt.Fprint(w, "data: {\"id\":\"chatcmpl-test\",\"object\":\"chat.completion.chunk\","+ + "\"model\":\"gpt-test\",\"choices\":[{\"index\":0,\"delta\":"+ + "{\"role\":\"assistant\",\"content\":\"ok\"}}]}\n\n") + _, _ = fmt.Fprint(w, "data: [DONE]\n\n") + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + _, err := ping(metaCtx(m), openAIClient(server.URL, c, map[string]any{"stream": true})) + if err == nil { + t.Fatal("CompletionsWithCtx succeeded, want an incomplete-stream error") + } + var integrityErr *streamIntegrityError + if !errors.As(err, &integrityErr) { + t.Fatalf("err = %T, want *streamIntegrityError", err) + } + + got := attemptsFor(t, c, m) + if len(got) != 1 { + t.Fatalf("got %d attempts, want 1", len(got)) + } + if got[0].Outcome != AttemptError || got[0].ErrorClass != ErrorClassProvider || got[0].FailurePhase != FailurePhaseStream { + t.Errorf("attempt 1 = %s %s/%s, want error provider/stream", + got[0].Outcome, got[0].ErrorClass, got[0].FailurePhase) + } + + if req := freezeOne(t, c); req.Outcome != OutcomeFailed { + t.Errorf("outcome = %s, want failed", req.Outcome) + } +} + +// A stream that never opened needs no correction: stream.Err() carries the +// non-2xx *apierror.Error, the observer already recorded the attempt as an error, +// and ReviseLastAttempt's precondition makes the wrapper a no-op. Asserted so the +// precondition is not later "simplified" away on the streaming path. +func TestBoundaryKeepsHTTPClassOnStreamThatNeverOpened(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusForbidden) + _, _ = fmt.Fprint(w, `{"error":{"message":"nope","type":"permission_error"}}`) + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + if _, err := ping(metaCtx(m), openAIClient(server.URL, c, map[string]any{"stream": true})); err == nil { + t.Fatal("CompletionsWithCtx succeeded, want a 403 error") + } + + got := attemptsFor(t, c, m) + if len(got) != 1 { + t.Fatalf("got %d attempts, want 1", len(got)) + } + if got[0].ErrorClass != ErrorClassAuthentication || got[0].FailurePhase != FailurePhaseHTTP { + t.Errorf("attempt 1 = %s/%s, want authentication/http kept, not rewritten to a stream failure", + got[0].ErrorClass, got[0].FailurePhase) + } +} + +// --- blind spot 4: a Responses object that is not a success --- + +// The Responses API answers HTTP 200 with the failure inside the object, so the +// SDK returns a nil Go error. Missing this correction is silent: the request is +// listed as failed with a single success attempt and no error_class at all — +// self-consistent counts over a record that misstates what happened. +func TestBoundaryCorrectsResponsesStatus(t *testing.T) { + for _, status := range []string{"failed", "cancelled", "queued", "in_progress"} { + t.Run(status, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(w, `{"id":"resp_test","object":"response","model":"gpt-test",`+ + `"status":%q,"output":[]}`, status) + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + _, err := ping(metaCtx(m), responsesClient(server.URL, c)) + if err == nil { + t.Fatalf("CompletionsWithCtx succeeded, want an error for status=%s", status) + } + + got := attemptsFor(t, c, m) + if len(got) != 1 { + t.Fatalf("got %d attempts, want 1", len(got)) + } + if got[0].Outcome != AttemptError || got[0].ErrorClass != ErrorClassProvider || got[0].FailurePhase != FailurePhaseResponseStatus { + t.Errorf("attempt 1 = %s %s/%s, want error provider/response_status", + got[0].Outcome, got[0].ErrorClass, got[0].FailurePhase) + } + if got[0].StatusCode != http.StatusOK { + t.Errorf("attempt 1 status_code = %d, want the observed 200 kept", got[0].StatusCode) + } + + if req := freezeOne(t, c); req.Outcome != OutcomeFailed { + t.Errorf("outcome = %s, want failed", req.Outcome) + } + }) + } +} + +// --- the counter-example: corrections must not overwrite an HTTP class --- + +// A 5xx whose error body is corrupt makes the SDK return the raw JSON error +// instead of an *apierror.Error, so the boundary sees a decode failure for a +// request whose attempts are all classified from their status code. The status +// code is the stronger fact; a corrupt body only costs diagnostic richness. +func TestBoundaryKeepsHTTPClassOnCorruptErrorBody(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Retry-After-Ms", "0") + w.WriteHeader(http.StatusInternalServerError) + _, _ = fmt.Fprint(w, "not json at all") + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + _, err := ping(metaCtx(m), anthropicClient(server.URL, c, nil)) + if err == nil { + t.Fatal("CompletionsWithCtx succeeded, want a 500 error") + } + + got := attemptsFor(t, c, m) + if len(got) != 6 { + t.Fatalf("got %d attempts, want 6 (1 + WithMaxRetries(5))", len(got)) + } + last := got[len(got)-1] + if last.ErrorClass != ErrorClassProvider || last.FailurePhase != FailurePhaseHTTP { + t.Errorf("last attempt = %s/%s, want provider/http — a corrupt error body must not rewrite it to unknown/response_decode", + last.ErrorClass, last.FailurePhase) + } + if last.StatusCode != http.StatusInternalServerError { + t.Errorf("last attempt status_code = %d, want 500", last.StatusCode) + } + + if req := freezeOne(t, c); req.Outcome != OutcomeFailed { + t.Errorf("outcome = %s, want failed", req.Outcome) + } +} + +// --- outcome is decided at Finalize, not read off the last attempt --- + +// Cancelling while the SDK sleeps between attempts produces no new attempt, so +// the sequence still ends in a 429 error while the request outcome is cancelled. +// This is the case that makes inferring the outcome from the last attempt wrong. +func TestBoundaryCancelDuringBackoffIsCancelled(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + w.Header().Set("Content-Type", "application/json") + // The SDK honors a server hint verbatim, so this is long enough that the + // cancel below always lands inside the wait rather than in an attempt. + w.Header().Set("Retry-After-Ms", "3000") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"type":"error","error":{"type":"rate_limit_error","message":"slow down"}}`) + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + ctx, cancel := context.WithCancel(metaCtx(m)) + defer cancel() + stop := time.AfterFunc(100*time.Millisecond, cancel) + defer stop.Stop() + + _, err := ping(ctx, anthropicClient(server.URL, c, nil)) + if !errors.Is(err, context.Canceled) { + t.Fatalf("CompletionsWithCtx err = %v, want context.Canceled", err) + } + if n := requests.Load(); n != 1 { + t.Errorf("server saw %d requests, want 1 (the cancel lands in the backoff)", n) + } + + got := attemptsFor(t, c, m) + if len(got) != 1 { + t.Fatalf("got %d attempts, want 1 — a cancelled wait must not invent an attempt", len(got)) + } + if got[0].ErrorClass != ErrorClassRateLimited { + t.Errorf("attempt 1 = %s, want the 429 classification untouched", got[0].ErrorClass) + } + + rep, freezeErr := c.Freeze("test-run-id") + if freezeErr != nil { + t.Fatalf("Freeze: %v", freezeErr) + } + if rep.Requests[0].Outcome != OutcomeCancelled { + t.Errorf("outcome = %s, want cancelled even though the last attempt is an HTTP error", + rep.Requests[0].Outcome) + } + if rep.CancelledRequests != 1 || rep.FailedRequests != 0 || rep.RecoveredRequests != 0 { + t.Errorf("cancelled %d failed %d recovered %d, want 1/0/0", + rep.CancelledRequests, rep.FailedRequests, rep.RecoveredRequests) + } +} + +// The same backoff wait as the test above, ended by the per-attempt timeout +// instead of a user abort — the pair that pins the cancelled/failed split. +// +// Both SDKs build the per-attempt context inside the retry loop and then wait +// for the backoff on that same context (requestconfig.go:467-509 for openai-go, +// :434-478 for anthropic-sdk-go), and retryDelay adopts a server hint verbatim +// with no upper bound. A hint longer than ClientConfig.Timeout therefore always +// expires the wait: the SDK returns context.DeadlineExceeded without making a +// second attempt. +// +// No new code serves this path — it is asserted because three separate pieces +// have to hold at once. The last attempt stays the 429 (the boundary correction +// recognizes DeadlineExceeded but no-ops on an attempt that is already an +// error), the parent context is untouched so rule 2 cannot fire, and rule 3 +// gives failed. Getting any one of them wrong turns a provider throttling us +// past our own timeout into a reported user cancellation. +func TestBoundaryRetryAfterOutlivingAttemptTimeoutIsFailed(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + w.Header().Set("Content-Type", "application/json") + // An order of magnitude past the client timeout below, so the wait can + // only ever end at the deadline. + w.Header().Set("Retry-After-Ms", "3000") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"type":"error","error":{"type":"rate_limit_error","message":"slow down"}}`) + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + client := NewAnthropicClient(ClientConfig{ + URL: server.URL + "/v1/messages", + APIKey: "test-key", + Model: "claude-test", + AuthHeader: "x-api-key", + // Reaches the SDK as WithRequestTimeout, which is per attempt and not a + // budget for the logical request. + Timeout: 200 * time.Millisecond, + retryCollector: c, + }) + + // No deadline and no cancel on the parent: the only clock in play is the + // per-attempt one. + _, err := ping(metaCtx(m), client) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("CompletionsWithCtx err = %v, want context.DeadlineExceeded", err) + } + if n := requests.Load(); n != 1 { + t.Errorf("server saw %d requests, want 1 — the hint outlives the attempt timeout", n) + } + + got := attemptsFor(t, c, m) + if len(got) != 1 { + t.Fatalf("got %d attempts, want 1 — an expired wait must not invent an attempt", len(got)) + } + if got[0].ErrorClass != ErrorClassRateLimited || got[0].FailurePhase != FailurePhaseHTTP { + t.Errorf("attempt 1 = %s/%s, want rate_limited/http kept — the timeout correction must no-op here", + got[0].ErrorClass, got[0].FailurePhase) + } + if got[0].StatusCode != http.StatusTooManyRequests || got[0].RetryAfterMS != 3000 { + t.Errorf("attempt 1 status %d retry_after_ms %d, want 429/3000", + got[0].StatusCode, got[0].RetryAfterMS) + } + + rep, freezeErr := c.Freeze("test-run-id") + if freezeErr != nil { + t.Fatalf("Freeze: %v", freezeErr) + } + if rep.Requests[0].Outcome != OutcomeFailed { + t.Errorf("outcome = %s, want failed — an attempt deadline is not a user abort", + rep.Requests[0].Outcome) + } + if rep.FailedRequests != 1 { + t.Errorf("failed_requests = %d, want 1", rep.FailedRequests) + } +} + +// A parent deadline is not a user abort: only context.Canceled reaches +// parentCancelled, so an expired parent context is failed. Conflating the two +// would let the SDK's own per-attempt timeout report the run as cancelled. +func TestBoundaryDeadlineExceededIsFailed(t *testing.T) { + release := make(chan struct{}) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-release // outlive the parent deadline; see the note in the test above + })) + defer server.Close() + defer close(release) + + c := NewRetryCollector() + m := testMeta() + ctx, cancel := context.WithTimeout(metaCtx(m), 150*time.Millisecond) + defer cancel() + + _, err := ping(ctx, anthropicClient(server.URL, c, nil)) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("CompletionsWithCtx err = %v, want context.DeadlineExceeded", err) + } + + got := attemptsFor(t, c, m) + if len(got) != 1 { + t.Fatalf("got %d attempts, want 1", len(got)) + } + if got[0].ErrorClass != ErrorClassTimeout || got[0].FailurePhase != FailurePhaseContext { + t.Errorf("attempt 1 = %s/%s, want timeout/context", got[0].ErrorClass, got[0].FailurePhase) + } + + if req := freezeOne(t, c); req.Outcome != OutcomeFailed { + t.Errorf("outcome = %s, want failed — DeadlineExceeded is not a cancellation", req.Outcome) + } +} + +// A request that fails before any HTTP attempt (here: unparsable tool call +// arguments) has no entry to finalize, so it is absent from the report rather +// than listed with an empty attempt list. As with the no-identity case, the +// assertion needs a second, real request: with nothing but zero-attempt requests +// the collector has no entries and Freeze returns (nil, nil). +func TestBoundarySkipsRequestWithoutAttempt(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if requests.Add(1) == 1 { + w.Header().Set("Retry-After-Ms", "0") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"type":"error","error":{"type":"rate_limit_error","message":"slow down"}}`) + return + } + _, _ = fmt.Fprint(w, anthropicOKBody) + })) + defer server.Close() + + c := NewRetryCollector() + client := anthropicClient(server.URL, c, nil) + + empty := testMeta() + empty.FilePath = "never-sent.go" + _, err := client.CompletionsWithCtx(metaCtx(empty), ChatRequest{ + Messages: []Message{{ + Role: "assistant", + ToolCalls: []ToolCall{{ID: "call_1", Function: FunctionCall{Name: "read", Arguments: "{not json"}}}, + }}, + }) + if err == nil { + t.Fatal("CompletionsWithCtx succeeded, want a parameter-building error") + } + if n := requests.Load(); n != 0 { + t.Fatalf("server saw %d requests, want 0", n) + } + + m := testMeta() + if _, err := ping(metaCtx(m), client); err != nil { + t.Fatalf("CompletionsWithCtx: %v", err) + } + + rep, freezeErr := c.Freeze("test-run-id") + if freezeErr != nil { + t.Fatalf("Freeze: %v", freezeErr) + } + if rep.TotalRequests != 1 || len(rep.Requests) != 1 { + t.Errorf("total_requests %d, listed %d, want 1/1 — a request with no attempt must not appear", + rep.TotalRequests, len(rep.Requests)) + } + if rep.Requests[0].FilePath == empty.FilePath { + t.Errorf("the zero-attempt request was listed") + } +} + +// --- classification units --- + +// The false return is the contract: an error the boundary cannot recognize is +// left alone rather than bucketed as unknown, because the only remaining way to +// tell errors apart there is their message text. +func TestClassifyBoundaryError(t *testing.T) { + cases := []struct { + name string + err error + wantClass ErrorClass + wantPhase FailurePhase + recognized bool + }{ + {name: "nil"}, + { + name: "cancelled", err: fmt.Errorf("wrapped: %w", context.Canceled), + wantClass: ErrorClassCancelled, wantPhase: FailurePhaseContext, recognized: true, + }, + { + name: "deadline", err: fmt.Errorf("wrapped: %w", context.DeadlineExceeded), + wantClass: ErrorClassTimeout, wantPhase: FailurePhaseContext, recognized: true, + }, + { + name: "truncated body", err: fmt.Errorf("error reading response body: %w", io.ErrUnexpectedEOF), + wantClass: ErrorClassNetwork, wantPhase: FailurePhaseResponseDecode, recognized: true, + }, + { + name: "json syntax", err: fmt.Errorf("error parsing response json: %w", &json.SyntaxError{}), + wantClass: ErrorClassUnknown, wantPhase: FailurePhaseResponseDecode, recognized: true, + }, + { + name: "json type", err: fmt.Errorf("error parsing response json: %w", &json.UnmarshalTypeError{}), + wantClass: ErrorClassUnknown, wantPhase: FailurePhaseResponseDecode, recognized: true, + }, + {name: "opaque", err: errors.New("something went wrong")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + class, phase, recognized := classifyBoundaryError(tc.err) + if recognized != tc.recognized { + t.Fatalf("recognized = %v, want %v", recognized, tc.recognized) + } + if class != tc.wantClass || phase != tc.wantPhase { + t.Errorf("got %s/%s, want %s/%s", class, phase, tc.wantClass, tc.wantPhase) + } + }) + } +} + +// Unlike the boundary classifier this one always answers, because the caller +// already knows the failure came from an established stream. The fallback is +// unknown/stream rather than classifyAttempt's network/transport default: after +// HTTP 200 the error could just as easily come from decoding or the provider, so +// naming the transport would be an unverified claim. +func TestClassifyStreamError(t *testing.T) { + cases := []struct { + name string + err error + wantClass ErrorClass + wantPhase FailurePhase + }{ + { + name: "integrity", err: &streamIntegrityError{reason: "contained no choices"}, + wantClass: ErrorClassProvider, wantPhase: FailurePhaseStream, + }, + { + name: "sse stream error", err: fmt.Errorf("wrapped: %w", &ssestream.StreamError{}), + wantClass: ErrorClassProvider, wantPhase: FailurePhaseStream, + }, + { + name: "cancelled keeps the context phase", err: fmt.Errorf("wrapped: %w", context.Canceled), + wantClass: ErrorClassCancelled, wantPhase: FailurePhaseContext, + }, + { + name: "deadline keeps the context phase", err: fmt.Errorf("wrapped: %w", context.DeadlineExceeded), + wantClass: ErrorClassTimeout, wantPhase: FailurePhaseContext, + }, + { + name: "opaque", err: errors.New("something went wrong"), + wantClass: ErrorClassUnknown, wantPhase: FailurePhaseStream, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + class, phase := classifyStreamError(tc.err) + if class != tc.wantClass || phase != tc.wantPhase { + t.Errorf("got %s/%s, want %s/%s", class, phase, tc.wantClass, tc.wantPhase) + } + }) + } +} + +// The three integrity conditions keep their exact rendered messages: the type +// exists to make them classifiable, not to change what a user reads. +func TestStreamIntegrityErrorMessage(t *testing.T) { + err := &streamIntegrityError{reason: "contained no choices"} + if got, want := err.Error(), "OpenAI streaming response contained no choices"; got != want { + t.Errorf("Error() = %q, want %q", got, want) + } +} + +// A panic must still finalize, or the entry stays unfinalized and Freeze drops +// the whole run's report — one panicking file would erase every other file's +// retry records. No hostile server input makes the three clients panic, which is +// why the sentinel path is asserted here rather than through httptest. +func TestFinalizeRequestWithPanicSentinel(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + ctx := metaCtx(m) + c.RecordAttempt(m, AttemptRecord{StatusCode: http.StatusOK}, time.Time{}, time.Time{}) + + finalizeRequest(ctx, c, errRequestPanicked) + + if req := freezeOne(t, c); req.Outcome != OutcomeFailed { + t.Errorf("outcome = %s, want failed", req.Outcome) + } +} + +// A nil collector and a context without identity are both no-ops, so no call +// site in the three clients needs to guard either. +func TestBoundaryHelpersAreInertWithoutCollectorOrMeta(t *testing.T) { + m := testMeta() + reviseAttempt(metaCtx(m), nil, ErrorClassNetwork, FailurePhaseResponseDecode) + finalizeRequest(metaCtx(m), nil, errors.New("boom")) + + c := NewRetryCollector() + reviseAttempt(context.Background(), c, ErrorClassNetwork, FailurePhaseResponseDecode) + finalizeRequest(context.Background(), c, errors.New("boom")) + + c.mu.Lock() + entries := len(c.entries) + c.mu.Unlock() + if entries != 0 { + t.Errorf("collector holds %d entries, want 0", entries) + } +} diff --git a/internal/llm/retry_meta.go b/internal/llm/retry_meta.go new file mode 100644 index 00000000..05eda40b --- /dev/null +++ b/internal/llm/retry_meta.go @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "strconv" + "strings" +) + +// logicalRequestIDVersion is the domain prefix mixed into every +// logical_request_id. Changing the field set of RequestMeta must bump this +// prefix so IDs from two different field sets can never collide. +const logicalRequestIDVersion = "ocr.llm-request/v1" + +// RequestMeta identifies one logical LLM request. A logical request is a single +// business-level call — one main_task turn, one re-location, one memory +// compression — which may expand into several real HTTP attempts inside the SDK +// retry loop. +// +// The fields mirror the existing session.TaskRecord so the retry report joins +// against the session JSONL without introducing a second source of truth: +// +// - FilePath is the raw session path (session.FileSession.FilePath), not the +// manifest-normalized path. manifestPaths collapses the "/dev/null" +// sentinel for item_id derivation; normalizing here would break the join. +// - TaskType is the session.TaskType string. +// - RequestNo is session.TaskRecord.RequestNo, a sequence starting at 1 that +// is scoped per (FilePath, TaskType). That scoping is what makes the tuple +// unique within a run. +// +// run_id is deliberately absent. It is only needed to compute +// logical_request_id, which happens in RetryCollector.Freeze, so the collector +// can be constructed before the session exists — the client is built in +// loadLLMRuntime, well before agent.New. +type RequestMeta struct { + // Provider is the resolved provider label. It is legitimately empty for an + // unnamed endpoint (only llm.ResolvedEndpoint built from the provider + // config path carries a name), so empty is a valid value and never means + // "missing meta". It must not be replaced by Protocol. + Provider string + Model string + FilePath string + TaskType string + RequestNo int +} + +// valid reports whether m can identify a logical request. +// +// Provider may be empty; Model, FilePath and TaskType may not. No field may +// contain NUL, because NUL is the canonical encoding separator: accepting it +// would let two different metas produce the same digest. +func (m RequestMeta) valid() bool { + if m.RequestNo <= 0 { + return false + } + if strings.ContainsRune(m.Provider, 0) { + return false + } + for _, s := range []string{m.Model, m.FilePath, m.TaskType} { + if s == "" || strings.ContainsRune(s, 0) { + return false + } + } + return true +} + +// logicalRequestID computes the canonical logical_request_id for m under runID. +// +// Fields are NUL-terminated in a fixed order rather than concatenated, because +// FilePath, Provider and Model can hold arbitrary printable bytes and a plain +// concatenation would let distinct metas collide. RequestNo is encoded as +// decimal ASCII so no byte order is involved. Bytes participate as-is: no case +// or path normalization, since the ID only has to be stable within one run. +// +// Every field is terminated, including the last one, and RequestNo goes through +// the same loop as the strings. A trailing separator is redundant today, but a +// field appended after an unterminated RequestNo would collide immediately +// (request_no 12 + "abc" against request_no 1 + "2abc"), so the encoding is +// written so that adding a field cannot introduce that. +// +// The precondition is that m is valid: an invalid meta may contain NUL and +// therefore forge another meta's byte stream. It holds because the only metas +// reaching here are RetryCollector entry keys, and RecordAttempt validates +// before creating an entry. This is not re-checked here, because the reporting +// path must never panic or fail a review. +func (m RequestMeta) logicalRequestID(runID string) string { + h := sha256.New() + for _, field := range []string{ + logicalRequestIDVersion, + runID, + m.Provider, + m.Model, + m.FilePath, + m.TaskType, + strconv.Itoa(m.RequestNo), + } { + h.Write([]byte(field)) + h.Write([]byte{0}) + } + return hex.EncodeToString(h.Sum(nil)) +} + +// describe renders m for internal construction errors raised by +// RetryCollector.Freeze. +// +// Those errors reach the user through runErr, and with --concurrency > 1 a run +// holds hundreds of logical requests: without identity there is no way to tell +// which one tripped the invariant. Only fields that RequestReport already emits +// are included, so this adds no disclosure surface. +func (m RequestMeta) describe() string { + return fmt.Sprintf("file=%s task=%s request_no=%d", m.FilePath, m.TaskType, m.RequestNo) +} + +// requestMetaKey is the private context key type for RequestMeta. It is a +// struct{} rather than a string so no other package can collide with it. +type requestMetaKey struct{} + +// WithRequestMeta attaches m to ctx so the retry observer can associate every +// HTTP attempt made under that context with one logical request. It is the only +// way request identity reaches the observer: LLMClient is a single-method +// interface (CompletionsWithCtx), so no call site signature changes. +// +// An invalid meta is not attached and ctx is returned unchanged. The attempt +// then follows the same path as any request without identity — dropped by the +// collector, absent from the report — instead of introducing a new failure mode +// that could suppress the whole report or fail the review. +func WithRequestMeta(ctx context.Context, m RequestMeta) context.Context { + if ctx == nil || !m.valid() { + return ctx + } + return context.WithValue(ctx, requestMetaKey{}, m) +} + +// RequestMetaFromContext returns the RequestMeta attached to ctx, if any. +// +// It is exported so the packages that stamp identity — internal/llmloop and +// internal/agent — can assert in their own tests that a request reached the +// client carrying the right meta, and that scan's requests carry none. A +// package-local export_test.go cannot serve that: it is only visible to tests +// of package llm, and none of the call sites live here. The accessor is +// read-only and stays inside internal/, so it adds no mutation path and no +// repository-external API. +func RequestMetaFromContext(ctx context.Context) (RequestMeta, bool) { + if ctx == nil { + return RequestMeta{}, false + } + m, ok := ctx.Value(requestMetaKey{}).(RequestMeta) + return m, ok +} diff --git a/internal/llm/retry_meta_test.go b/internal/llm/retry_meta_test.go new file mode 100644 index 00000000..2cab666f --- /dev/null +++ b/internal/llm/retry_meta_test.go @@ -0,0 +1,175 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "strings" + "testing" +) + +func testMeta() RequestMeta { + return RequestMeta{ + Provider: "anthropic", + Model: "claude-sonnet-4-6", + FilePath: "payment.go", + TaskType: "main_task", + RequestNo: 1, + } +} + +func TestRequestMetaValid(t *testing.T) { + cases := []struct { + name string + mut func(*RequestMeta) + want bool + }{ + {"complete", func(*RequestMeta) {}, true}, + {"empty provider is valid", func(m *RequestMeta) { m.Provider = "" }, true}, + {"missing model", func(m *RequestMeta) { m.Model = "" }, false}, + {"missing file path", func(m *RequestMeta) { m.FilePath = "" }, false}, + {"missing task type", func(m *RequestMeta) { m.TaskType = "" }, false}, + {"zero request no", func(m *RequestMeta) { m.RequestNo = 0 }, false}, + {"negative request no", func(m *RequestMeta) { m.RequestNo = -1 }, false}, + {"NUL in provider", func(m *RequestMeta) { m.Provider = "anth\x00ropic" }, false}, + {"NUL in model", func(m *RequestMeta) { m.Model = "cla\x00ude" }, false}, + {"NUL in file path", func(m *RequestMeta) { m.FilePath = "pay\x00.go" }, false}, + {"NUL in task type", func(m *RequestMeta) { m.TaskType = "main\x00" }, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := testMeta() + tc.mut(&m) + if got := m.valid(); got != tc.want { + t.Fatalf("valid() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestLogicalRequestIDIsDeterministic(t *testing.T) { + m := testMeta() + if a, b := m.logicalRequestID("run-1"), m.logicalRequestID("run-1"); a != b { + t.Fatalf("same input produced different IDs: %s vs %s", a, b) + } + if len(m.logicalRequestID("run-1")) != 64 { + t.Fatalf("expected a 64-char hex digest, got %q", m.logicalRequestID("run-1")) + } +} + +// TestLogicalRequestIDSeparatesFields is the reason the encoding is canonical: +// a plain concatenation would give these metas the same digest. +func TestLogicalRequestIDSeparatesFields(t *testing.T) { + base := testMeta() + + shifted := base + shifted.Provider = "anthropi" + shifted.Model = "cclaude-sonnet-4-6" + + swapped := base + swapped.FilePath, swapped.TaskType = base.TaskType, base.FilePath + + cases := map[string]RequestMeta{ + "byte shifted between provider and model": shifted, + "file path and task type swapped": swapped, + } + want := base.logicalRequestID("run-1") + for name, m := range cases { + t.Run(name, func(t *testing.T) { + if got := m.logicalRequestID("run-1"); got == want { + t.Fatalf("distinct meta produced the same ID %s", got) + } + }) + } +} + +func TestLogicalRequestIDVariesWithRunIDAndRequestNo(t *testing.T) { + m := testMeta() + base := m.logicalRequestID("run-1") + + if m.logicalRequestID("run-2") == base { + t.Fatal("different run_id produced the same ID") + } + other := m + other.RequestNo = 2 + if other.logicalRequestID("run-1") == base { + t.Fatal("different request_no produced the same ID") + } + + // The separator in front of request_no matters too: without it, task_type + // "main_task1" with request_no 2 and task_type "main_task" with request_no 12 + // would hash the same bytes. + a := m + a.TaskType, a.RequestNo = "main_task1", 2 + b := m + b.TaskType, b.RequestNo = "main_task", 12 + if a.logicalRequestID("run-1") == b.logicalRequestID("run-1") { + t.Fatal("task_type and request_no are not separated") + } +} + +// The encoding is sha256 over version, run_id, provider, model, file_path, +// task_type and decimal request_no, each followed by a NUL — the trailing field +// included. The digest is pinned because that layout is not observable from any +// other assertion: with the current field set the ID stays collision-free with or +// without the last terminator, so dropping it would silently reintroduce the +// collision that appending a future field would cause. +// +// IDs only have to be stable within one run, so updating this constant is +// allowed. It just has to be a deliberate edit and not a silent side effect. +func TestLogicalRequestIDCanonicalEncoding(t *testing.T) { + const want = "14e212a5316c922ea2e0758da1a243255ac33f6360fd0d4e70af90ad1441516c" + if got := testMeta().logicalRequestID("run-1"); got != want { + t.Fatalf("canonical encoding changed:\n got %s\nwant %s", got, want) + } +} + +func TestRequestMetaDescribe(t *testing.T) { + got := testMeta().describe() + for _, want := range []string{"file=payment.go", "task=main_task", "request_no=1"} { + if !strings.Contains(got, want) { + t.Fatalf("describe() = %q, missing %q", got, want) + } + } +} + +func TestWithRequestMeta(t *testing.T) { + t.Run("round trip", func(t *testing.T) { + m := testMeta() + ctx := WithRequestMeta(context.Background(), m) + got, ok := RequestMetaFromContext(ctx) + if !ok || got != m { + t.Fatalf("got (%+v, %v), want (%+v, true)", got, ok, m) + } + }) + + t.Run("invalid meta is not attached", func(t *testing.T) { + m := testMeta() + m.Model = "" + ctx := WithRequestMeta(context.Background(), m) + if _, ok := RequestMetaFromContext(ctx); ok { + t.Fatal("invalid meta was attached to the context") + } + }) + + t.Run("bare context carries nothing", func(t *testing.T) { + if _, ok := RequestMetaFromContext(context.Background()); ok { + t.Fatal("empty context reported a meta") + } + }) + + // Both directions tolerate a nil context instead of panicking, so a call site + // that never set one up degrades to "no identity, no report" rather than + // failing the review. + t.Run("nil context", func(t *testing.T) { + //nolint:staticcheck // deliberately passing a nil context + if got := WithRequestMeta(nil, testMeta()); got != nil { + t.Fatalf("WithRequestMeta(nil, ...) = %v, want nil", got) + } + //nolint:staticcheck // deliberately passing a nil context + if m, ok := RequestMetaFromContext(nil); ok || m != (RequestMeta{}) { + t.Fatalf("got (%+v, %v), want (zero, false)", m, ok) + } + }) +} diff --git a/internal/llm/retry_observer.go b/internal/llm/retry_observer.go new file mode 100644 index 00000000..29d77bd2 --- /dev/null +++ b/internal/llm/retry_observer.go @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "net/http" + "strconv" + "time" +) + +// retryObserver is the middleware signature both SDKs use. option.Middleware in +// anthropic-sdk-go and openai-go are type *aliases* for this exact function type, +// not defined types, so one observer value satisfies both without conversion — +// which is why the three clients share a single implementation. +type retryObserver = func(*http.Request, func(*http.Request) (*http.Response, error)) (*http.Response, error) + +// newRetryObserver builds the middleware that feeds collector. +// +// It sits inside the SDK retry loop, so it runs once per real HTTP attempt and +// sees neither the backoff sleep (which happens around it) nor the response body +// (which the SDK reads after it returns). Both of those shape what the observer +// can honestly report: the measured gap between attempts covers the sleep, and +// anything that only surfaces while decoding the body is invisible here and is +// corrected at the client boundary instead. +// +// The observer never reads or closes the response body. The SDK closes it before +// retrying and reads it itself on the error path; consuming it here would break +// the SDK and put response content somewhere it must never go. +func newRetryObserver(collector *RetryCollector) retryObserver { + return func(req *http.Request, next func(*http.Request) (*http.Response, error)) (*http.Response, error) { + // Identity comes from the context the business layer attached before + // calling the SDK. The per-attempt context the SDK builds is derived from + // it, so the value is still visible here. Requests without identity — + // scan and llm test — go straight through: the collector would drop them + // anyway, and skipping the timestamps keeps those paths untouched. + meta, ok := RequestMetaFromContext(req.Context()) + if collector == nil || !ok { + return next(req) + } + + startedAt := time.Now() + res, err := next(req) + endedAt := time.Now() + + collector.RecordAttempt(meta, observeAttempt(res, err, endedAt), startedAt, endedAt) + return res, err + } +} + +// observeAttempt turns one transport result into an AttemptRecord. +// +// Classification runs whenever the status is non-2xx or the transport failed. +// The non-2xx half is not optional: RecordAttempt records an unclassified error +// status as a violation, because deriving it as a success would make the request +// vanish from the report while still being counted. +func observeAttempt(res *http.Response, err error, endedAt time.Time) AttemptRecord { + a := AttemptRecord{} + if res != nil { + a.StatusCode = res.StatusCode + a.RequestID = responseRequestID(res.Header) + a.RetryAfterMS = parseRetryAfterMS(res.Header, endedAt) + a.SDKRetryDirective = parseRetryDirective(res.Header) + } + if isErrorStatus(a.StatusCode) || err != nil { + a.ErrorClass, a.FailurePhase = classifyAttempt(attemptObservation{StatusCode: a.StatusCode, Err: err}) + } + return a +} + +// responseRequestID reads the provider's request identifier. Anthropic sends +// request-id and OpenAI sends x-request-id; the observer is shared by all three +// clients, so it reads both rather than being parameterized by provider. +func responseRequestID(h http.Header) string { + if v := h.Get("request-id"); v != "" { + return v + } + return h.Get("x-request-id") +} + +// parseRetryDirective reads x-should-retry, the header both SDKs consult ahead of +// the status code (and without excluding 2xx). Only the two values they act on +// are recorded; anything else is reported as absent rather than coerced, because +// the SDK ignores it too. +func parseRetryDirective(h http.Header) *bool { + switch h.Get("x-should-retry") { + case "true": + v := true + return &v + case "false": + v := false + return &v + } + return nil +} + +// parseRetryAfterMS reports the server's retry hint in milliseconds, mirroring +// the SDKs' parseRetryAfterHeader: Retry-After-Ms wins over Retry-After, each is +// first read as a number (milliseconds and seconds respectively), and only +// Retry-After falls back to an RFC1123 date. +// +// The date form is resolved against endedAt rather than a fresh time.Now so the +// value is a function of what this attempt observed. The two differ only by +// header-parsing time; the contract requires the same precedence as the SDK, not +// byte-identical numbers. +// +// A hint in the past yields zero, matching the SDK's max(0, delay): a negative +// wait is not a fact about the server, it just means the deadline has passed. +func parseRetryAfterMS(h http.Header, endedAt time.Time) int64 { + for _, hdr := range []struct { + name string + unit time.Duration + asRFC bool + }{ + {name: "Retry-After-Ms", unit: time.Millisecond}, + {name: "Retry-After", unit: time.Second, asRFC: true}, + } { + v := h.Get(hdr.name) + if v == "" { + continue + } + if n, err := strconv.ParseFloat(v, 64); err == nil { + return nonNegativeMillis(time.Duration(n * float64(hdr.unit))) + } + if hdr.asRFC { + if t, err := time.Parse(time.RFC1123, v); err == nil { + return nonNegativeMillis(t.Sub(endedAt)) + } + } + } + return 0 +} diff --git a/internal/llm/retry_observer_test.go b/internal/llm/retry_observer_test.go new file mode 100644 index 00000000..8c102fcc --- /dev/null +++ b/internal/llm/retry_observer_test.go @@ -0,0 +1,691 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" + "time" +) + +// --- helpers --- + +const ( + anthropicOKBody = `{ + "id":"msg_test","type":"message","role":"assistant","model":"claude-test", + "content":[{"type":"text","text":"ok"}],"stop_reason":"end_turn", + "usage":{"input_tokens":1,"output_tokens":1} + }` + openAIOKBody = `{ + "id":"chatcmpl-test","object":"chat.completion","model":"gpt-test", + "choices":[{"index":0,"message":{"role":"assistant","content":"ok"},"finish_reason":"stop"}], + "usage":{"prompt_tokens":1,"completion_tokens":1,"total_tokens":2} + }` + responsesOKBody = `{ + "id":"resp_test","object":"response","model":"gpt-test","status":"completed", + "output":[{"type":"message","role":"assistant","content":[{"type":"output_text","text":"ok"}]}], + "usage":{"input_tokens":1,"output_tokens":1,"total_tokens":2} + }` +) + +// Every test below reaches Freeze without finalizing anything itself: the +// client boundary in CompletionsWithCtx does it. That is deliberate — Freeze +// refuses to build a report while any logical request is unfinalized, so a +// client that lost its boundary defer turns these Freeze assertions red instead +// of silently reporting nothing. + +// attemptsFor returns a copy of the attempts recorded for m. +// +// It takes the collector's lock rather than reading entries directly: a test +// that leaves a request in flight would otherwise race, and that failure would +// show up as a flake in an unrelated test rather than here. +func attemptsFor(t *testing.T, c *RetryCollector, m RequestMeta) []AttemptRecord { + t.Helper() + c.mu.Lock() + defer c.mu.Unlock() + e := c.entries[m] + if e == nil { + return nil + } + return append([]AttemptRecord(nil), e.attempts...) +} + +func ping(ctx context.Context, client LLMClient) (*ChatResponse, error) { + return client.CompletionsWithCtx(ctx, ChatRequest{ + Messages: []Message{{Role: "user", Content: "ping"}}, + MaxTokens: 64, + }) +} + +// metaCtx attaches identity the way the review path will in P4. +func metaCtx(m RequestMeta) context.Context { + return WithRequestMeta(context.Background(), m) +} + +func anthropicClient(url string, c *RetryCollector, extra map[string]string) *AnthropicClient { + return NewAnthropicClient(ClientConfig{ + URL: url + "/v1/messages", + APIKey: "test-key", + Model: "claude-test", + AuthHeader: "x-api-key", + ExtraHeaders: extra, + retryCollector: c, + }) +} + +// --- attempt-level observation --- + +// The canonical retry: one 429 with a server hint, then success. Everything the +// observer is responsible for is visible in this one sequence. +func TestObserverRecordsRateLimitedThenSuccess(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("request-id", fmt.Sprintf("req_%d", requests.Load()+1)) + if requests.Add(1) == 1 { + w.Header().Set("Retry-After-Ms", "40") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"type":"error","error":{"type":"rate_limit_error","message":"slow down"}}`) + return + } + _, _ = fmt.Fprint(w, anthropicOKBody) + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + if _, err := ping(metaCtx(m), anthropicClient(server.URL, c, nil)); err != nil { + t.Fatalf("CompletionsWithCtx: %v", err) + } + + got := attemptsFor(t, c, m) + if len(got) != 2 { + t.Fatalf("got %d attempts, want 2", len(got)) + } + + first, second := got[0], got[1] + if first.Number != 1 || first.Outcome != AttemptError { + t.Errorf("attempt 1 = number %d outcome %s, want 1/error", first.Number, first.Outcome) + } + if first.ErrorClass != ErrorClassRateLimited || first.FailurePhase != FailurePhaseHTTP { + t.Errorf("attempt 1 classified as %s/%s, want rate_limited/http", first.ErrorClass, first.FailurePhase) + } + if first.StatusCode != http.StatusTooManyRequests { + t.Errorf("attempt 1 status_code = %d, want 429", first.StatusCode) + } + if first.RequestID != "req_1" { + t.Errorf("attempt 1 request_id = %q, want req_1", first.RequestID) + } + if first.RetryAfterMS != 40 { + t.Errorf("attempt 1 retry_after_ms = %d, want 40", first.RetryAfterMS) + } + if first.ObservedBackoffMS != 0 { + t.Errorf("attempt 1 observed_backoff_ms = %d, want 0 (no predecessor)", first.ObservedBackoffMS) + } + + if second.Number != 2 || second.Outcome != AttemptSuccess { + t.Errorf("attempt 2 = number %d outcome %s, want 2/success", second.Number, second.Outcome) + } + if second.ErrorClass != "" || second.FailurePhase != "" { + t.Errorf("attempt 2 carries error fields: %+v", second) + } + if second.RequestID != "req_2" { + t.Errorf("attempt 2 request_id = %q, want req_2", second.RequestID) + } + // The SDK honors Retry-After-Ms verbatim, and the sleep happens around the + // middleware, so the measured gap must cover it. Asserted with slack because + // it is a real elapsed time, not a computed one. + if second.ObservedBackoffMS < 30 { + t.Errorf("attempt 2 observed_backoff_ms = %d, want >= 30 (server asked for 40)", second.ObservedBackoffMS) + } + + rep, err := c.Freeze("test-run-id") + if err != nil { + t.Fatalf("Freeze: %v", err) + } + if rep == nil { + t.Fatal("Freeze returned no report, want one") + } + if rep.TotalRequests != 1 || rep.RetriedRequests != 1 || rep.TotalRetries != 1 { + t.Errorf("aggregates = total %d retried %d retries %d, want 1/1/1", + rep.TotalRequests, rep.RetriedRequests, rep.TotalRetries) + } + if rep.RecoveredRequests != 1 || rep.FailedRequests != 0 { + t.Errorf("recovered %d failed %d, want 1/0", rep.RecoveredRequests, rep.FailedRequests) + } + if len(rep.Requests) != 1 || rep.Requests[0].Outcome != OutcomeRecovered { + t.Fatalf("requests = %+v, want one recovered", rep.Requests) + } +} + +// Retries exhaust at WithMaxRetries(5), so a permanently overloaded provider +// produces six attempts under one logical request, numbered without a gap. +func TestObserverRecordsExhaustedRetries(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Retry-After-Ms", "0") // keep the test fast; 0 is a valid hint + w.WriteHeader(529) + _, _ = fmt.Fprint(w, `{"type":"error","error":{"type":"overloaded_error","message":"overloaded"}}`) + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + _, err := ping(metaCtx(m), anthropicClient(server.URL, c, nil)) + if err == nil { + t.Fatal("CompletionsWithCtx succeeded, want an error") + } + + got := attemptsFor(t, c, m) + if len(got) != 6 { + t.Fatalf("got %d attempts, want 6 (1 + WithMaxRetries(5))", len(got)) + } + for i, a := range got { + if a.Number != i+1 { + t.Errorf("attempt %d numbered %d", i+1, a.Number) + } + if a.Outcome != AttemptError || a.ErrorClass != ErrorClassOverloaded || a.StatusCode != 529 { + t.Errorf("attempt %d = %+v, want overloaded 529 error", i+1, a) + } + } + + rep, freezeErr := c.Freeze("test-run-id") + if freezeErr != nil { + t.Fatalf("Freeze: %v", freezeErr) + } + if rep.TotalRetries != 5 || rep.FailedRequests != 1 || rep.RecoveredRequests != 0 { + t.Errorf("aggregates = retries %d failed %d recovered %d, want 5/1/0", + rep.TotalRetries, rep.FailedRequests, rep.RecoveredRequests) + } + if rep.Requests[0].Outcome != OutcomeFailed { + t.Errorf("outcome = %s, want failed", rep.Requests[0].Outcome) + } +} + +// Statuses the SDK does not retry produce exactly one attempt. 402 is the case +// that pins the coarse provider bucket: it is told apart by status_code rather +// than by an enum that duplicates HTTP. +func TestObserverClassifiesTerminalStatuses(t *testing.T) { + cases := []struct { + status int + want ErrorClass + }{ + {http.StatusUnauthorized, ErrorClassAuthentication}, + {http.StatusForbidden, ErrorClassAuthentication}, + {http.StatusPaymentRequired, ErrorClassProvider}, + } + for _, tc := range cases { + t.Run(fmt.Sprint(tc.status), func(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tc.status) + _, _ = fmt.Fprint(w, `{"type":"error","error":{"type":"invalid_request_error","message":"nope"}}`) + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + _, err := ping(metaCtx(m), anthropicClient(server.URL, c, nil)) + if err == nil { + t.Fatal("CompletionsWithCtx succeeded, want an error") + } + if n := requests.Load(); n != 1 { + t.Errorf("server saw %d requests, want 1 (no retry)", n) + } + + got := attemptsFor(t, c, m) + if len(got) != 1 { + t.Fatalf("got %d attempts, want 1", len(got)) + } + if got[0].ErrorClass != tc.want || got[0].FailurePhase != FailurePhaseHTTP { + t.Errorf("classified as %s/%s, want %s/http", got[0].ErrorClass, got[0].FailurePhase, tc.want) + } + if got[0].StatusCode != tc.status { + t.Errorf("status_code = %d, want %d", got[0].StatusCode, tc.status) + } + }) + } +} + +// Both SDKs consult x-should-retry before the status code and do not exclude +// 2xx, so a server can make them retry a successful response. The extra attempt +// is real and is reported, but nothing failed: the outcome is succeeded, and the +// summary legitimately shows a retry with zero recovered and zero failed. +func TestObserverRecordsRetryDirectiveOnSuccess(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if requests.Add(1) == 1 { + w.Header().Set("x-should-retry", "true") + w.Header().Set("Retry-After-Ms", "0") + } + _, _ = fmt.Fprint(w, anthropicOKBody) + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + if _, err := ping(metaCtx(m), anthropicClient(server.URL, c, nil)); err != nil { + t.Fatalf("CompletionsWithCtx: %v", err) + } + + got := attemptsFor(t, c, m) + if len(got) != 2 { + t.Fatalf("got %d attempts, want 2", len(got)) + } + if got[0].Outcome != AttemptSuccess || got[1].Outcome != AttemptSuccess { + t.Fatalf("attempts = %+v, want both success", got) + } + if got[0].SDKRetryDirective == nil || !*got[0].SDKRetryDirective { + t.Errorf("attempt 1 sdk_retry_directive = %v, want true", got[0].SDKRetryDirective) + } + if got[1].SDKRetryDirective != nil { + t.Errorf("attempt 2 sdk_retry_directive = %v, want absent", *got[1].SDKRetryDirective) + } + + rep, err := c.Freeze("test-run-id") + if err != nil { + t.Fatalf("Freeze: %v", err) + } + if rep.RetriedRequests != 1 || rep.TotalRetries != 1 { + t.Errorf("retried %d retries %d, want 1/1", rep.RetriedRequests, rep.TotalRetries) + } + if rep.RecoveredRequests != 0 || rep.FailedRequests != 0 { + t.Errorf("recovered %d failed %d, want 0/0", rep.RecoveredRequests, rep.FailedRequests) + } + if rep.Requests[0].Outcome != OutcomeSucceeded { + t.Errorf("outcome = %s, want succeeded", rep.Requests[0].Outcome) + } +} + +// A transport failure gives the observer no response at all: no status, no +// diagnostics, and a class derived from the Go error alone. +func TestObserverRecordsTransportFailure(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + url := server.URL + server.Close() // nothing is listening, so every attempt fails to connect + + c := NewRetryCollector() + m := testMeta() + // A refused connection sends no Retry-After, so the SDK falls back to its own + // exponential backoff and five retries take ~15s. The deadline cuts the run + // short during the first sleep; the attempts already recorded are the point. + ctx, cancel := context.WithTimeout(metaCtx(m), 300*time.Millisecond) + defer cancel() + if _, err := ping(ctx, anthropicClient(url, c, nil)); err == nil { + t.Fatal("CompletionsWithCtx succeeded, want a connection error") + } + + got := attemptsFor(t, c, m) + if len(got) == 0 { + t.Fatal("no attempts recorded") + } + for i, a := range got { + if a.StatusCode != 0 { + t.Errorf("attempt %d status_code = %d, want 0 (no response)", i+1, a.StatusCode) + } + if a.ErrorClass != ErrorClassNetwork || a.FailurePhase != FailurePhaseTransport { + t.Errorf("attempt %d = %s/%s, want network/transport", i+1, a.ErrorClass, a.FailurePhase) + } + } +} + +// X-Stainless-Retry-Count is an SDK implementation detail. Overriding it through +// ExtraHeaders makes the SDK stop maintaining it (it only refreshes the header +// when it still reads "0"), so every attempt carries the same bogus value — +// which must change nothing about numbering, collection or Freeze. +func TestObserverIgnoresOverriddenRetryCountHeader(t *testing.T) { + var seen []string + var mu sync.Mutex + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + seen = append(seen, r.Header.Get("X-Stainless-Retry-Count")) + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + if requests.Add(1) == 1 { + w.Header().Set("Retry-After-Ms", "0") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"type":"error","error":{"type":"rate_limit_error","message":"slow down"}}`) + return + } + _, _ = fmt.Fprint(w, anthropicOKBody) + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + extra := map[string]string{"X-Stainless-Retry-Count": "7"} + if _, err := ping(metaCtx(m), anthropicClient(server.URL, c, extra)); err != nil { + t.Fatalf("CompletionsWithCtx: %v", err) + } + + mu.Lock() + defer mu.Unlock() + for i, v := range seen { + if v != "7" { + t.Fatalf("request %d sent retry-count %q, want the overridden 7", i+1, v) + } + } + + got := attemptsFor(t, c, m) + if len(got) != 2 || got[0].Number != 1 || got[1].Number != 2 { + t.Fatalf("attempts = %+v, want two numbered 1 and 2", got) + } + if _, err := c.Freeze("test-run-id"); err != nil { + t.Fatalf("Freeze: %v", err) + } +} + +// Requests without identity — scan and llm test — are dropped whole. The +// assertion needs a second, identified request in the same collector: with only +// unidentified traffic there are no entries, Freeze returns (nil, nil), and +// total_requests cannot be observed at all. +func TestObserverDropsRequestsWithoutIdentity(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusPaymentRequired) + _, _ = fmt.Fprint(w, `{"type":"error","error":{"type":"invalid_request_error","message":"nope"}}`) + })) + defer server.Close() + + c := NewRetryCollector() + client := anthropicClient(server.URL, c, nil) + + // No RequestMeta on the context: this is what scan looks like. + if _, err := ping(context.Background(), client); err == nil { + t.Fatal("CompletionsWithCtx succeeded, want an error") + } + c.mu.Lock() + entries := len(c.entries) + c.mu.Unlock() + if entries != 0 { + t.Fatalf("collector holds %d entries after an unidentified request, want 0", entries) + } + + m := testMeta() + _, err := ping(metaCtx(m), client) + if err == nil { + t.Fatal("CompletionsWithCtx succeeded, want an error") + } + + rep, freezeErr := c.Freeze("test-run-id") + if freezeErr != nil { + t.Fatalf("Freeze: %v", freezeErr) + } + if rep.TotalRequests != 1 || len(rep.Requests) != 1 { + t.Errorf("total_requests %d, listed %d, want 1/1 — the unidentified request must not appear", + rep.TotalRequests, len(rep.Requests)) + } +} + +// --- the other two mount points --- + +// The observer is mounted on all three clients, so the OpenAI Chat Completions +// and Responses constructors get the same 429-then-success check. The bodies +// differ; the observed sequence must not. +func TestObserverMountedOnOpenAIClients(t *testing.T) { + cases := []struct { + name string + okBody string + errBody string + build func(url string, c *RetryCollector) LLMClient + }{ + { + name: "chat_completions", + okBody: openAIOKBody, + errBody: `{"error":{"message":"slow down","type":"rate_limit_error"}}`, + build: func(url string, c *RetryCollector) LLMClient { + return NewOpenAIClient(ClientConfig{ + URL: url + "/v1", APIKey: "test-key", Model: "gpt-test", retryCollector: c, + }) + }, + }, + { + name: "responses", + okBody: responsesOKBody, + errBody: `{"error":{"message":"slow down","type":"rate_limit_error"}}`, + build: func(url string, c *RetryCollector) LLMClient { + return NewOpenAIResponsesClient(ClientConfig{ + URL: url + "/v1", APIKey: "test-key", Model: "gpt-test", retryCollector: c, + }) + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if requests.Add(1) == 1 { + // OpenAI reports its identifier under a different header + // than Anthropic; the shared observer reads both. + w.Header().Set("x-request-id", "req_openai") + w.Header().Set("Retry-After-Ms", "0") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, tc.errBody) + return + } + _, _ = fmt.Fprint(w, tc.okBody) + })) + defer server.Close() + + c := NewRetryCollector() + m := testMeta() + if _, err := ping(metaCtx(m), tc.build(server.URL, c)); err != nil { + t.Fatalf("CompletionsWithCtx: %v", err) + } + + got := attemptsFor(t, c, m) + if len(got) != 2 { + t.Fatalf("got %d attempts, want 2", len(got)) + } + if got[0].ErrorClass != ErrorClassRateLimited || got[0].StatusCode != http.StatusTooManyRequests { + t.Errorf("attempt 1 = %+v, want rate_limited 429", got[0]) + } + if got[0].RequestID != "req_openai" { + t.Errorf("attempt 1 request_id = %q, want req_openai", got[0].RequestID) + } + if got[1].Outcome != AttemptSuccess { + t.Errorf("attempt 2 = %+v, want success", got[1]) + } + }) + } +} + +// A nil collector must leave the request path exactly as it was: no middleware, +// no observation, and no change in behavior. +func TestNilCollectorMountsNoObserver(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, anthropicOKBody) + })) + defer server.Close() + + client := anthropicClient(server.URL, nil, nil) + if _, err := ping(metaCtx(testMeta()), client); err != nil { + t.Fatalf("CompletionsWithCtx: %v", err) + } +} + +// --- concurrency --- + +// With --concurrency > 1 several files share one collector. Distinct metas must +// stay in distinct entries, and the frozen report must be stable. Run under +// -race, this is also the data-race assertion. +func TestObserverConcurrentRequests(t *testing.T) { + var requests sync.Map + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + key := r.Header.Get("X-Test-Request") + n, _ := requests.LoadOrStore(key, new(atomic.Int32)) + w.Header().Set("Content-Type", "application/json") + if n.(*atomic.Int32).Add(1) == 1 { + w.Header().Set("Retry-After-Ms", "0") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = fmt.Fprint(w, `{"type":"error","error":{"type":"rate_limit_error","message":"slow down"}}`) + return + } + _, _ = fmt.Fprint(w, anthropicOKBody) + })) + defer server.Close() + + c := NewRetryCollector() + const n = 8 + metas := make([]RequestMeta, n) + var wg sync.WaitGroup + for i := 0; i < n; i++ { + m := testMeta() + m.FilePath = fmt.Sprintf("file_%d.go", i) + metas[i] = m + client := anthropicClient(server.URL, c, map[string]string{"X-Test-Request": m.FilePath}) + wg.Add(1) + go func() { + defer wg.Done() + if _, err := ping(metaCtx(m), client); err != nil { + t.Errorf("%s: %v", m.FilePath, err) + } + }() + } + wg.Wait() + + for _, m := range metas { + if got := attemptsFor(t, c, m); len(got) != 2 { + t.Errorf("%s recorded %d attempts, want 2", m.FilePath, len(got)) + } + } + + rep, err := c.Freeze("test-run-id") + if err != nil { + t.Fatalf("Freeze: %v", err) + } + if rep.TotalRequests != n || rep.RecoveredRequests != n || rep.TotalRetries != n { + t.Errorf("aggregates = total %d recovered %d retries %d, want %d each", + rep.TotalRequests, rep.RecoveredRequests, rep.TotalRetries, n) + } + for i := 1; i < len(rep.Requests); i++ { + if rep.Requests[i-1].LogicalRequestID >= rep.Requests[i].LogicalRequestID { + t.Fatalf("requests not sorted by logical_request_id at %d", i) + } + } +} + +// --- header parsing units --- + +// Priority and units follow the SDKs' parseRetryAfterHeader exactly: Retry-After-Ms +// first and in milliseconds, then Retry-After as seconds, then Retry-After as an +// RFC1123 date. A hint that has already passed is not a negative wait. +func TestParseRetryAfterMS(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + cases := []struct { + name string + headers map[string]string + want int64 + }{ + {name: "absent", headers: nil, want: 0}, + {name: "milliseconds", headers: map[string]string{"Retry-After-Ms": "1500"}, want: 1500}, + {name: "seconds", headers: map[string]string{"Retry-After": "3"}, want: 3000}, + {name: "fractional seconds", headers: map[string]string{"Retry-After": "0.5"}, want: 500}, + { + name: "milliseconds win over seconds", + headers: map[string]string{"Retry-After-Ms": "250", "Retry-After": "9"}, + want: 250, + }, + { + name: "unparsable milliseconds fall through to seconds", + headers: map[string]string{"Retry-After-Ms": "soon", "Retry-After": "2"}, + want: 2000, + }, + { + name: "rfc1123 date", + headers: map[string]string{"Retry-After": now.Add(4 * time.Second).Format(time.RFC1123)}, + want: 4000, + }, + { + name: "past rfc1123 date floors at zero", + headers: map[string]string{"Retry-After": now.Add(-time.Hour).Format(time.RFC1123)}, + want: 0, + }, + {name: "unparsable", headers: map[string]string{"Retry-After": "tomorrow"}, want: 0}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := http.Header{} + for k, v := range tc.headers { + h.Set(k, v) + } + if got := parseRetryAfterMS(h, now); got != tc.want { + t.Errorf("parseRetryAfterMS = %d, want %d", got, tc.want) + } + }) + } +} + +// Only the two values the SDKs act on are recorded. Anything else is absent +// rather than coerced to false, because the SDK ignores it too and a false would +// claim the server said something it did not. +func TestParseRetryDirective(t *testing.T) { + cases := []struct { + value string + want *bool + }{ + {value: "", want: nil}, + {value: "true", want: boolPtr(true)}, + {value: "false", want: boolPtr(false)}, + {value: "yes", want: nil}, + } + for _, tc := range cases { + t.Run("x-should-retry="+tc.value, func(t *testing.T) { + h := http.Header{} + if tc.value != "" { + h.Set("x-should-retry", tc.value) + } + got := parseRetryDirective(h) + switch { + case tc.want == nil && got != nil: + t.Errorf("got %v, want absent", *got) + case tc.want != nil && got == nil: + t.Errorf("got absent, want %v", *tc.want) + case tc.want != nil && *got != *tc.want: + t.Errorf("got %v, want %v", *got, *tc.want) + } + }) + } +} + +func TestResponseRequestID(t *testing.T) { + cases := []struct { + name string + headers map[string]string + want string + }{ + {name: "anthropic", headers: map[string]string{"request-id": "req_a"}, want: "req_a"}, + {name: "openai", headers: map[string]string{"x-request-id": "req_o"}, want: "req_o"}, + { + name: "anthropic wins when both present", + headers: map[string]string{"request-id": "req_a", "x-request-id": "req_o"}, + want: "req_a", + }, + {name: "absent", headers: nil, want: ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := http.Header{} + for k, v := range tc.headers { + h.Set(k, v) + } + if got := responseRequestID(h); got != tc.want { + t.Errorf("responseRequestID = %q, want %q", got, tc.want) + } + }) + } +} + +func boolPtr(v bool) *bool { return &v } diff --git a/internal/llm/retry_report.go b/internal/llm/retry_report.go new file mode 100644 index 00000000..79732f7f --- /dev/null +++ b/internal/llm/retry_report.go @@ -0,0 +1,666 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "errors" + "fmt" + "io" + "sort" + "strings" + "sync" + "time" +) + +// RetryReportSchemaVersion is the contract version of the emitted report. +const RetryReportSchemaVersion = "ocr.llm-retry-report/v1" + +// ErrorClass is the attempt-level error classification. It is derived only from +// the HTTP status and the Go error type — never from error message text — and it +// is not interchangeable with session.FailureClass (file level) or +// session.RunFailureClass (run level). +type ErrorClass string + +const ( + ErrorClassRateLimited ErrorClass = "rate_limited" + ErrorClassOverloaded ErrorClass = "overloaded" + ErrorClassAuthentication ErrorClass = "authentication" + ErrorClassTimeout ErrorClass = "timeout" + ErrorClassNetwork ErrorClass = "network" + ErrorClassProvider ErrorClass = "provider" + ErrorClassCancelled ErrorClass = "cancelled" + ErrorClassUnknown ErrorClass = "unknown" +) + +// valid reports whether c is one of the fixed attempt error classes. +func (c ErrorClass) valid() bool { + switch c { + case ErrorClassRateLimited, ErrorClassOverloaded, ErrorClassAuthentication, + ErrorClassTimeout, ErrorClassNetwork, ErrorClassProvider, + ErrorClassCancelled, ErrorClassUnknown: + return true + } + return false +} + +// FailurePhase records where in the request lifecycle the failure surfaced. +type FailurePhase string + +const ( + FailurePhaseTransport FailurePhase = "transport" + FailurePhaseHTTP FailurePhase = "http" + FailurePhaseResponseDecode FailurePhase = "response_decode" + FailurePhaseStream FailurePhase = "stream" + FailurePhaseResponseStatus FailurePhase = "response_status" + FailurePhaseContext FailurePhase = "context" +) + +// valid reports whether p is one of the fixed failure phases. +func (p FailurePhase) valid() bool { + switch p { + case FailurePhaseTransport, FailurePhaseHTTP, FailurePhaseResponseDecode, + FailurePhaseStream, FailurePhaseResponseStatus, FailurePhaseContext: + return true + } + return false +} + +// Outcome is the request-level result of a logical request. It is decided once, +// at Finalize, from the whole attempt sequence plus the logical request's own +// return value and the parent context state — never inferred from the last +// attempt. +type Outcome string + +const ( + // OutcomeSucceeded means the logical request succeeded with no error + // attempt. It still reaches the report when the server made the SDK retry a + // successful response via x-should-retry, which is why recovered_requests + // stays honest: nothing was recovered from. + OutcomeSucceeded Outcome = "succeeded" + OutcomeRecovered Outcome = "recovered" + OutcomeFailed Outcome = "failed" + OutcomeCancelled Outcome = "cancelled" +) + +// AttemptOutcome is the per-attempt result. +type AttemptOutcome string + +const ( + AttemptSuccess AttemptOutcome = "success" + AttemptError AttemptOutcome = "error" +) + +// AttemptRecord is one observed real HTTP attempt. +// +// Number, Outcome, DurationToHeadersMS and ObservedBackoffMS are assigned by the +// collector; values passed in are ignored. The two durations are derived from the +// timestamps RecordAttempt receives, because ObservedBackoffMS spans two attempts +// and only the collector holds per-request state. Diagnostic fields carry +// observed values only — no request or response bodies, no prompts, no URLs, no +// raw SDK error strings. +type AttemptRecord struct { + Number int `json:"attempt"` + Outcome AttemptOutcome `json:"outcome"` + ErrorClass ErrorClass `json:"error_class,omitempty"` + FailurePhase FailurePhase `json:"failure_phase,omitempty"` + + StatusCode int `json:"status_code,omitempty"` + RequestID string `json:"request_id,omitempty"` + // RetryAfterMS is the server hint, normalized to milliseconds. + RetryAfterMS int64 `json:"retry_after_ms,omitempty"` + // ObservedBackoffMS is the measured gap between the end of the previous + // attempt and the start of this one. It is not the SDK's planned wait: the + // SDK's jitter is not readable from outside. + ObservedBackoffMS int64 `json:"observed_backoff_ms,omitempty"` + // DurationToHeadersMS covers request send to response headers only, not body + // read, decode or streaming. Total logical request time stays on + // session.TaskRecord.Duration. + DurationToHeadersMS int64 `json:"duration_to_headers_ms,omitempty"` + // SDKRetryDirective is the x-should-retry response header. A pointer so an + // absent header is distinguishable from an explicit false. + SDKRetryDirective *bool `json:"sdk_retry_directive,omitempty"` +} + +// RequestReport is one logical request and its attempts. +type RequestReport struct { + LogicalRequestID string `json:"logical_request_id"` + // Provider is required but may be empty: an empty string stably denotes an + // unnamed endpoint, so it is not omitempty. This is a deliberate difference + // from the display-only jsonLLMIdentity.Provider, which is omitempty. + Provider string `json:"provider"` + Model string `json:"model"` + FilePath string `json:"file_path"` + TaskType string `json:"task_type"` + RequestNo int `json:"request_no"` + Outcome Outcome `json:"outcome"` + Attempts []AttemptRecord `json:"attempts"` +} + +// RetryReport is the frozen, immutable result both the terminal summary and the +// JSON output read from. +type RetryReport struct { + SchemaVersion string `json:"schema_version"` + // TotalRequests counts RequestMeta that produced at least one real HTTP + // attempt. A logical request that failed before entering the observer has no + // observed fact to report and is excluded. + TotalRequests int `json:"total_requests"` + RetriedRequests int `json:"retried_requests"` + TotalRetries int `json:"total_retries"` + RecoveredRequests int `json:"recovered_requests"` + FailedRequests int `json:"failed_requests"` + CancelledRequests int `json:"cancelled_requests"` + Requests []RequestReport `json:"requests"` +} + +// attemptObservation is the provider-agnostic classifier input. +type attemptObservation struct { + // StatusCode is 0 when no HTTP response was received. + StatusCode int + Err error +} + +// isErrorStatus reports whether an observed HTTP status proves the attempt +// failed on its own. Status 0 means no response was received, so it proves +// nothing. +// +// This is the single definition of that boundary, shared by classifyAttempt and +// the collector's consistency guard. Duplicating it would let the classifier and +// the guard disagree about, say, a 3xx, and the guard would then reject attempts +// the classifier considers fine. +func isErrorStatus(code int) bool { + return code > 0 && (code < 200 || code >= 300) +} + +// classifyAttempt maps an observation to an ErrorClass and FailurePhase. +// +// A non-2xx status is the strongest available fact, so it decides the class +// before the error is consulted. A 2xx carries no error information, so it falls +// through to the error-based branch; that is also the branch the client-boundary +// correction feeds when a 200 turns out to be truncated. +func classifyAttempt(obs attemptObservation) (ErrorClass, FailurePhase) { + if isErrorStatus(obs.StatusCode) { + switch obs.StatusCode { + case 429: + return ErrorClassRateLimited, FailurePhaseHTTP + case 529: + return ErrorClassOverloaded, FailurePhaseHTTP + case 401, 403: + return ErrorClassAuthentication, FailurePhaseHTTP + case 408, 504: + return ErrorClassTimeout, FailurePhaseHTTP + default: + // Coarse bucket on purpose: 402/404/409/413 and transient 5xx all + // land here and are told apart by status_code, rather than growing + // an enum that duplicates HTTP. + return ErrorClassProvider, FailurePhaseHTTP + } + } + + switch { + case errors.Is(obs.Err, context.Canceled): + return ErrorClassCancelled, FailurePhaseContext + case errors.Is(obs.Err, context.DeadlineExceeded): + return ErrorClassTimeout, FailurePhaseContext + case errors.Is(obs.Err, io.ErrUnexpectedEOF): + return ErrorClassNetwork, FailurePhaseResponseDecode + case obs.Err != nil: + return ErrorClassNetwork, FailurePhaseTransport + } + + // No status and no error is a caller bug: a successful attempt should never + // be classified. Report it rather than guessing. + if obs.StatusCode > 0 { + return ErrorClassUnknown, FailurePhaseHTTP + } + return ErrorClassUnknown, FailurePhaseTransport +} + +// requestEntry is the collector's mutable per-request state. +type requestEntry struct { + attempts []AttemptRecord + outcome Outcome + finalized bool + // violation records the first detected ordering bug (double Finalize, or + // mutation after Finalize). It surfaces as a Freeze error so the invariant + // "Finalize runs exactly once per logical request" is machine-checked + // instead of only asserted in prose. + violation string + // lastAttemptEnd is when the previous attempt of this logical request + // finished, and is the only state ObservedBackoffMS needs. It lives here + // rather than in the observer because one observer instance serves every + // concurrent request: keyed per-request state is exactly what the collector + // already is. + // + // It stays zero until an attempt is actually appended, so a dropped attempt + // never becomes the baseline for the next gap. + lastAttemptEnd time.Time +} + +func (e *requestEntry) hasErrorAttempt() bool { + for _, a := range e.attempts { + if a.Outcome == AttemptError { + return true + } + } + return false +} + +// RetryCollector aggregates observed attempts for one review run. +// +// It is created per run and owned by llmRuntime; there is no package-level +// state, so two runs in one process can never share data. All methods are safe +// for concurrent use because --concurrency > 1 has several files writing +// attempts into the same instance. +// +// There is no Register step: the first RecordAttempt creates the entry. That +// makes "TotalRequests counts metas with at least one attempt" a property of the +// data structure rather than a rule to remember, and it makes a zero-attempt +// logical request a no-op at Finalize. +type RetryCollector struct { + mu sync.Mutex + entries map[RequestMeta]*requestEntry +} + +// NewRetryCollector returns an empty collector. +func NewRetryCollector() *RetryCollector { + return &RetryCollector{entries: make(map[RequestMeta]*requestEntry)} +} + +// RecordAttempt appends one observed HTTP attempt for m. +// +// startedAt and endedAt bracket the real HTTP call: the observer takes them +// immediately before and after the SDK's transport call returns response +// headers. They are passed in rather than read from a clock here so the +// collector stays a pure aggregator and the derived durations are fully +// deterministic in tests; no clock abstraction is needed. +// +// An invalid meta is dropped: without identity there is nothing to aggregate +// against, which is also how scan and llm test requests stay out of the report. +// Number, Outcome and both durations on a are derived here, so the observer can +// neither desynchronize the numbering from the real call order nor invent a +// backoff it cannot measure. +// +// An attempt arriving after Finalize is recorded as a violation and dropped +// rather than appended, the same way ReviseLastAttempt behaves: the outcome was +// already decided without knowledge of this attempt, so mutating the sequence +// afterwards could only produce a record that contradicts it. +func (c *RetryCollector) RecordAttempt(m RequestMeta, a AttemptRecord, startedAt, endedAt time.Time) { + if c == nil || !m.valid() { + return + } + c.mu.Lock() + defer c.mu.Unlock() + + e := c.entries[m] + if e == nil { + e = &requestEntry{} + c.entries[m] = e + } + if e.finalized { + if e.violation == "" { + e.violation = "attempt recorded after Finalize" + } + return + } + + if a.ErrorClass != "" || a.FailurePhase != "" { + a.Outcome = AttemptError + } else { + a.Outcome = AttemptSuccess + } + if a.Outcome == AttemptSuccess { + a.ErrorClass = "" + a.FailurePhase = "" + } + + // Consistency guard. Outcome is derived from the classification alone, so an + // observer that reports a non-2xx status without classifying it (a caller + // that forgot classifyAttempt) would have the attempt derived as a success + // against the strongest fact it had. + // + // This is caught here because nothing downstream can catch it: a lone + // unclassified error attempt makes Finalize decide succeeded, which makes the + // listing rule in Freeze skip the request entirely, so a failed request would + // vanish from requests while still counted in total_requests. validateReport + // only walks listed requests and would never see it. + // + // Recorded as a violation rather than repaired: the collector has the status + // but not the error, so it cannot derive the class, and guessing one would put + // a fabricated classification in the report. The attempt is still appended, so + // the state stays inspectable; Freeze refuses to publish either way. + if a.Outcome == AttemptSuccess && isErrorStatus(a.StatusCode) && e.violation == "" { + e.violation = "non-2xx attempt recorded without a classification" + } + + // Derived from the observed timestamps, never from what the caller put in a. + // + // The gap is skipped on a zero baseline rather than on "this is attempt 1". + // The two differ on the OpenAI Chat Completions EOF recovery, which makes a + // second SDK call under the same logical request: that call's first attempt + // does have a predecessor, and its gap measures the client-side re-call + // interval rather than an SDK backoff. That is still a real measured + // interval, which is all the field claims to be. + a.DurationToHeadersMS = nonNegativeMillis(endedAt.Sub(startedAt)) + a.ObservedBackoffMS = 0 + if !e.lastAttemptEnd.IsZero() { + a.ObservedBackoffMS = nonNegativeMillis(startedAt.Sub(e.lastAttemptEnd)) + } + + a.Number = len(e.attempts) + 1 + e.attempts = append(e.attempts, a) + e.lastAttemptEnd = endedAt +} + +// nonNegativeMillis converts d to milliseconds, flooring at zero. +// +// Timestamps taken from time.Now carry a monotonic reading, so a real attempt +// can never measure negative. Hand-built time.Time values have no monotonic +// reading and can, so the floor exists to keep a nonsensical negative out of the +// report rather than to paper over a real inversion. +func nonNegativeMillis(d time.Duration) int64 { + if d <= 0 { + return 0 + } + return d.Milliseconds() +} + +// ReviseLastAttempt rewrites the last attempt of m as an error. +// +// It is the client-boundary correction for what the observer cannot see: an +// error surfacing only after HTTP 200 (truncated body, mid-stream failure, a +// non-success Responses object status). +// +// The precondition is enforced here rather than at the call site: the revision +// applies only while the last attempt is still recorded as a success. An attempt +// already classified from its status code (500, 402) is never rewritten to +// unknown/response_decode just because its error body failed to parse — the +// status code is the stronger fact, and a corrupt body only costs diagnostic +// richness. +func (c *RetryCollector) ReviseLastAttempt(m RequestMeta, class ErrorClass, phase FailurePhase) { + if c == nil || !m.valid() || !class.valid() || !phase.valid() { + return + } + c.mu.Lock() + defer c.mu.Unlock() + + e := c.entries[m] + if e == nil || len(e.attempts) == 0 { + return + } + if e.finalized { + if e.violation == "" { + e.violation = "attempt revised after Finalize" + } + return + } + last := &e.attempts[len(e.attempts)-1] + if last.Outcome != AttemptSuccess { + return + } + last.Outcome = AttemptError + last.ErrorClass = class + last.FailurePhase = phase +} + +// Finalize decides the request-level outcome for m exactly once. +// +// The three inputs are the recorded attempts, the error the logical request +// returned to the business layer, and whether the parent context was cancelled. +// parentCancelled is a bool rather than a context so the decision stays a pure +// function: reading the parent context is the client boundary's job, and only +// the boundary can tell the parent context from the SDK's per-attempt timeout +// context. +// +// Evaluation order matters and mirrors the design's ordered table: +// +// 1. no attempts -> no record at all (nothing to report on) +// 2. parent cancelled -> cancelled, even if the last attempt is an HTTP error +// 3. request returned an error -> failed +// 4. success with an error attempt -> recovered +// 5. success with no error attempt -> succeeded +// +// Rule 2 outranking rule 3 is the whole reason this is not inferred from the +// last attempt: cancelling during backoff produces no new attempt, so the +// sequence still ends in an error while the request outcome is cancelled. +// +// A logical request must be finalized once. The EOF recovery in OpenAI Chat +// Completions makes two SDK calls under one logical request and must finalize +// only at the outermost boundary; a second call is recorded as a violation and +// surfaces at Freeze. +func (c *RetryCollector) Finalize(m RequestMeta, reqErr error, parentCancelled bool) { + if c == nil || !m.valid() { + return + } + c.mu.Lock() + defer c.mu.Unlock() + + e := c.entries[m] + if e == nil { + // Rule 1: the request never reached the observer. + return + } + if e.finalized { + if e.violation == "" { + e.violation = "Finalize called more than once" + } + return + } + e.finalized = true + + switch { + case parentCancelled: + e.outcome = OutcomeCancelled + case reqErr != nil: + e.outcome = OutcomeFailed + case e.hasErrorAttempt(): + e.outcome = OutcomeRecovered + default: + e.outcome = OutcomeSucceeded + } +} + +// Freeze builds the immutable report. It must be called once, after the run is +// done, and runID is the review session ID. +// +// The three return shapes are distinct and callers must handle all of them: +// +// (nil, nil) — nothing worth reporting: no retry and no request error. +// (nil, err) — an internal construction error. Do not publish a report. +// (rep, nil) — publish rep. +// +// runID is only needed here, which is why the collector does not need it at +// construction time and therefore does not depend on the session existing when +// the client is built. +func (c *RetryCollector) Freeze(runID string) (*RetryReport, error) { + if c == nil { + return nil, nil + } + if runID == "" || strings.ContainsRune(runID, 0) { + return nil, fmt.Errorf("retry report: invalid run_id") + } + + c.mu.Lock() + defer c.mu.Unlock() + + rep := &RetryReport{ + SchemaVersion: RetryReportSchemaVersion, + TotalRequests: len(c.entries), + } + + // Walk in logical_request_id order instead of map order. Both outputs depend + // on it: the listed requests need the order anyway, and a construction error + // has to name the same entry on every run, or a collector holding two broken + // entries would report a different one each time and no test could pin it. + type entryRef struct { + id string + meta RequestMeta + e *requestEntry + } + refs := make([]entryRef, 0, len(c.entries)) + for meta, e := range c.entries { + refs = append(refs, entryRef{id: meta.logicalRequestID(runID), meta: meta, e: e}) + } + sort.Slice(refs, func(i, j int) bool { return refs[i].id < refs[j].id }) + + for _, ref := range refs { + meta, e := ref.meta, ref.e + if e.violation != "" { + return nil, fmt.Errorf("retry report: %s (%s)", e.violation, meta.describe()) + } + if !e.finalized { + // Every logical request that produced an attempt must be finalized, + // which forces the client boundary to finalize on every exit path + // (including cancellation) rather than only on the happy path. + return nil, fmt.Errorf("retry report: logical request not finalized (%s)", meta.describe()) + } + if len(e.attempts) == 0 { + return nil, fmt.Errorf("retry report: entry with no attempt (%s)", meta.describe()) + } + + rep.TotalRetries += len(e.attempts) - 1 + if len(e.attempts) > 1 { + rep.RetriedRequests++ + } + switch e.outcome { + case OutcomeRecovered: + rep.RecoveredRequests++ + case OutcomeFailed: + rep.FailedRequests++ + case OutcomeCancelled: + rep.CancelledRequests++ + } + + // Listing rule: anything that retried, anything that saw an error, and + // anything whose outcome is not succeeded. The last clause is what keeps + // the aggregates verifiable from the listed requests alone — a request + // cancelled after a single clean attempt has no error attempt and no + // retry, yet it is counted in CancelledRequests, so it must be listed. + if len(e.attempts) == 1 && !e.hasErrorAttempt() && e.outcome == OutcomeSucceeded { + continue + } + rep.Requests = append(rep.Requests, RequestReport{ + LogicalRequestID: ref.id, + Provider: meta.Provider, + Model: meta.Model, + FilePath: meta.FilePath, + TaskType: meta.TaskType, + RequestNo: meta.RequestNo, + Outcome: e.outcome, + Attempts: append([]AttemptRecord(nil), e.attempts...), + }) + } + + if len(rep.Requests) == 0 { + return nil, nil + } + // No sort here: the walk above is already in logical_request_id order, which + // is what makes the output stable under --concurrency > 1. + if err := validateReport(rep); err != nil { + return nil, err + } + return rep, nil +} + +// validateReport enforces the report invariants. A violation returns an error +// and suppresses the report rather than publishing self-contradictory numbers. +// +// Every aggregate except TotalRequests is recomputed from the listed requests +// and compared with the value accumulated over all entries. That cross-check is +// the invariant: it holds only because the listing rule guarantees any request +// contributing to a count is listed. TotalRequests is the one aggregate that +// legitimately exceeds the listed set. +func validateReport(rep *RetryReport) error { + if rep.SchemaVersion != RetryReportSchemaVersion { + return fmt.Errorf("retry report: unexpected schema version %q", rep.SchemaVersion) + } + if rep.TotalRequests < len(rep.Requests) { + return fmt.Errorf("retry report: total_requests %d below listed %d", + rep.TotalRequests, len(rep.Requests)) + } + + seen := make(map[string]struct{}, len(rep.Requests)) + var retries, retried, recovered, failed, cancelled int + + for _, r := range rep.Requests { + if _, dup := seen[r.LogicalRequestID]; dup { + return fmt.Errorf("retry report: duplicate logical_request_id") + } + seen[r.LogicalRequestID] = struct{}{} + + if len(r.Attempts) == 0 { + return fmt.Errorf("retry report: request with no attempt") + } + hasError := false + for i, a := range r.Attempts { + if a.Number != i+1 { + return fmt.Errorf("retry report: attempt numbering not contiguous from 1") + } + switch a.Outcome { + case AttemptError: + hasError = true + if !a.ErrorClass.valid() || !a.FailurePhase.valid() { + return fmt.Errorf("retry report: error attempt without valid classification") + } + case AttemptSuccess: + if a.ErrorClass != "" || a.FailurePhase != "" { + return fmt.Errorf("retry report: success attempt carries error fields") + } + default: + return fmt.Errorf("retry report: unknown attempt outcome %q", a.Outcome) + } + } + + switch r.Outcome { + case OutcomeRecovered: + if !hasError { + return fmt.Errorf("retry report: recovered request without error attempt") + } + recovered++ + case OutcomeSucceeded: + if hasError { + return fmt.Errorf("retry report: succeeded request with error attempt") + } + // A listed succeeded request must have retried. That is not an + // independent rule: it holds only because Freeze's listing rule skips + // a succeeded request whose single attempt is clean. The redundancy is + // the point — this check fires if the listing rule ever drifts from + // the invariant it implements, which no other check would notice. + if len(r.Attempts) < 2 { + return fmt.Errorf("retry report: succeeded request listed with a single attempt") + } + case OutcomeFailed: + failed++ + case OutcomeCancelled: + cancelled++ + default: + return fmt.Errorf("retry report: unknown request outcome %q", r.Outcome) + } + + retries += len(r.Attempts) - 1 + if len(r.Attempts) > 1 { + retried++ + } + if r.Model == "" || r.FilePath == "" || r.TaskType == "" || r.RequestNo <= 0 { + return fmt.Errorf("retry report: incomplete request identity") + } + } + + if retries != rep.TotalRetries { + return fmt.Errorf("retry report: total_retries %d != %d", rep.TotalRetries, retries) + } + if retried != rep.RetriedRequests { + return fmt.Errorf("retry report: retried_requests %d != %d", rep.RetriedRequests, retried) + } + if recovered != rep.RecoveredRequests { + return fmt.Errorf("retry report: recovered_requests %d != %d", rep.RecoveredRequests, recovered) + } + if failed != rep.FailedRequests { + return fmt.Errorf("retry report: failed_requests %d != %d", rep.FailedRequests, failed) + } + if cancelled != rep.CancelledRequests { + return fmt.Errorf("retry report: cancelled_requests %d != %d", rep.CancelledRequests, cancelled) + } + return nil +} diff --git a/internal/llm/retry_report_test.go b/internal/llm/retry_report_test.go new file mode 100644 index 00000000..baf3d862 --- /dev/null +++ b/internal/llm/retry_report_test.go @@ -0,0 +1,905 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "reflect" + "sort" + "strings" + "sync" + "testing" + "time" +) + +// recordUntimed records an attempt with zero timestamps. +// +// These tests are about numbering, outcome and aggregation, where the derived +// durations are noise: zero timestamps make both of them 0 and leave +// lastAttemptEnd zero, so no attempt here ever gets an observed backoff. The +// timing derivation is covered on its own in TestRecordAttemptDerivesTimings. +func recordUntimed(c *RetryCollector, m RequestMeta, a AttemptRecord) { + c.RecordAttempt(m, a, time.Time{}, time.Time{}) +} + +func errAttempt(class ErrorClass, phase FailurePhase, status int) AttemptRecord { + return AttemptRecord{ErrorClass: class, FailurePhase: phase, StatusCode: status} +} + +func okAttempt() AttemptRecord { + return AttemptRecord{StatusCode: 200} +} + +// The enum sets are fixed by the report contract: 8 error classes and 6 failure +// phases. Enumerating them pins both the membership and the size, so adding a +// class or a phase has to come with a deliberate edit here rather than silently +// widening what the report can emit. +func TestErrorClassAndFailurePhaseSets(t *testing.T) { + classes := []ErrorClass{ + ErrorClassRateLimited, ErrorClassOverloaded, ErrorClassAuthentication, + ErrorClassTimeout, ErrorClassNetwork, ErrorClassProvider, + ErrorClassCancelled, ErrorClassUnknown, + } + if len(classes) != 8 { + t.Fatalf("the contract fixes 8 error classes, enumerated %d", len(classes)) + } + for _, c := range classes { + if !c.valid() { + t.Fatalf("error class %q rejected", c) + } + } + for _, c := range []ErrorClass{"", "rate-limited", "Overloaded", "throttled"} { + if c.valid() { + t.Fatalf("error class %q accepted", c) + } + } + + phases := []FailurePhase{ + FailurePhaseTransport, FailurePhaseHTTP, FailurePhaseResponseDecode, + FailurePhaseStream, FailurePhaseResponseStatus, FailurePhaseContext, + } + if len(phases) != 6 { + t.Fatalf("the contract fixes 6 failure phases, enumerated %d", len(phases)) + } + for _, p := range phases { + if !p.valid() { + t.Fatalf("failure phase %q rejected", p) + } + } + for _, p := range []FailurePhase{"", "http_request", "HTTP", "decode"} { + if p.valid() { + t.Fatalf("failure phase %q accepted", p) + } + } +} + +func TestClassifyAttempt(t *testing.T) { + cases := []struct { + name string + obs attemptObservation + wantClass ErrorClass + wantPhase FailurePhase + }{ + {"429", attemptObservation{StatusCode: 429}, ErrorClassRateLimited, FailurePhaseHTTP}, + {"529", attemptObservation{StatusCode: 529}, ErrorClassOverloaded, FailurePhaseHTTP}, + {"401", attemptObservation{StatusCode: 401}, ErrorClassAuthentication, FailurePhaseHTTP}, + {"403", attemptObservation{StatusCode: 403}, ErrorClassAuthentication, FailurePhaseHTTP}, + {"408", attemptObservation{StatusCode: 408}, ErrorClassTimeout, FailurePhaseHTTP}, + {"504", attemptObservation{StatusCode: 504}, ErrorClassTimeout, FailurePhaseHTTP}, + {"409", attemptObservation{StatusCode: 409}, ErrorClassProvider, FailurePhaseHTTP}, + {"402", attemptObservation{StatusCode: 402}, ErrorClassProvider, FailurePhaseHTTP}, + {"413", attemptObservation{StatusCode: 413}, ErrorClassProvider, FailurePhaseHTTP}, + {"500", attemptObservation{StatusCode: 500}, ErrorClassProvider, FailurePhaseHTTP}, + {"cancelled", attemptObservation{Err: context.Canceled}, ErrorClassCancelled, FailurePhaseContext}, + {"deadline", attemptObservation{Err: context.DeadlineExceeded}, ErrorClassTimeout, FailurePhaseContext}, + {"unexpected EOF", attemptObservation{Err: io.ErrUnexpectedEOF}, ErrorClassNetwork, FailurePhaseResponseDecode}, + {"wrapped EOF", attemptObservation{Err: fmt.Errorf("read body: %w", io.ErrUnexpectedEOF)}, ErrorClassNetwork, FailurePhaseResponseDecode}, + {"transport", attemptObservation{Err: errors.New("dial tcp: connection refused")}, ErrorClassNetwork, FailurePhaseTransport}, + {"no fact at all", attemptObservation{}, ErrorClassUnknown, FailurePhaseTransport}, + // Classifying a clean 2xx is a caller bug: there is no failure to + // describe. It reports unknown rather than guessing, and keeps the phase + // it does know — a response did arrive. + {"clean 2xx has nothing to classify", attemptObservation{StatusCode: 200}, ErrorClassUnknown, FailurePhaseHTTP}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + class, phase := classifyAttempt(tc.obs) + if class != tc.wantClass || phase != tc.wantPhase { + t.Fatalf("got (%s, %s), want (%s, %s)", class, phase, tc.wantClass, tc.wantPhase) + } + }) + } +} + +// A 2xx carries no error information, so the error decides. This is the path the +// client-boundary correction feeds when a 200 turns out to be truncated. +func TestClassifyAttemptTwoHundredFallsThroughToError(t *testing.T) { + class, phase := classifyAttempt(attemptObservation{StatusCode: 200, Err: io.ErrUnexpectedEOF}) + if class != ErrorClassNetwork || phase != FailurePhaseResponseDecode { + t.Fatalf("got (%s, %s), want (network, response_decode)", class, phase) + } +} + +func TestFinalizeDecisionOrder(t *testing.T) { + cases := []struct { + name string + attempts []AttemptRecord + reqErr error + parentCancelled bool + want Outcome + }{ + { + name: "success after an error is recovered", + attempts: []AttemptRecord{errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429), okAttempt()}, + want: OutcomeRecovered, + }, + { + name: "success with no error attempt is succeeded", + attempts: []AttemptRecord{okAttempt(), okAttempt()}, + want: OutcomeSucceeded, + }, + { + name: "returned error is failed", + attempts: []AttemptRecord{errAttempt(ErrorClassOverloaded, FailurePhaseHTTP, 529)}, + reqErr: errors.New("exhausted"), + want: OutcomeFailed, + }, + { + // Rule 2 outranks rule 3. Cancelling during backoff produces no new + // attempt, so the sequence still ends in an error while the request + // outcome is cancelled. Inferring from the last attempt would say + // failed. + name: "cancellation outranks a returned error", + attempts: []AttemptRecord{errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429)}, + reqErr: context.Canceled, + parentCancelled: true, + want: OutcomeCancelled, + }, + { + name: "cancellation after a clean attempt", + attempts: []AttemptRecord{okAttempt()}, + parentCancelled: true, + want: OutcomeCancelled, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + for _, a := range tc.attempts { + recordUntimed(c, m, a) + } + c.Finalize(m, tc.reqErr, tc.parentCancelled) + if got := c.entries[m].outcome; got != tc.want { + t.Fatalf("outcome = %s, want %s", got, tc.want) + } + }) + } +} + +// Rule 1: a logical request that never reached the observer produces no record +// at all, so it cannot inflate total_requests or appear in requests. +func TestFinalizeZeroAttemptProducesNoRecord(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + c.Finalize(m, errors.New("build request params"), false) + + if len(c.entries) != 0 { + t.Fatalf("expected no entry, got %d", len(c.entries)) + } + rep, err := c.Freeze("run-1") + if err != nil { + t.Fatalf("Freeze error: %v", err) + } + if rep != nil { + t.Fatalf("expected no report, got %+v", rep) + } +} + +func TestRecordAttemptNumbersAndDerivesOutcome(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + + // A caller-supplied number and outcome are ignored: numbering follows the + // real call order, never the SDK's retry-count header. + recordUntimed(c, m, AttemptRecord{Number: 99, Outcome: AttemptError, StatusCode: 200}) + recordUntimed(c, m, errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429)) + + got := c.entries[m].attempts + if len(got) != 2 { + t.Fatalf("got %d attempts, want 2", len(got)) + } + if got[0].Number != 1 || got[0].Outcome != AttemptSuccess { + t.Fatalf("attempt 1 = %+v, want number 1 and success", got[0]) + } + if got[0].ErrorClass != "" || got[0].FailurePhase != "" { + t.Fatalf("success attempt kept error fields: %+v", got[0]) + } + if got[1].Number != 2 || got[1].Outcome != AttemptError { + t.Fatalf("attempt 2 = %+v, want number 2 and error", got[1]) + } +} + +// Both durations are derived from the observed timestamps, so a caller cannot +// report a backoff it never measured. The first attempt has no predecessor and +// therefore no gap; the second measures the real interval, which spans the SDK's +// backoff sleep because that happens outside the middleware. +func TestRecordAttemptDerivesTimings(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + + // Supplied durations are ignored the same way Number and Outcome are. + first := errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429) + first.DurationToHeadersMS = 9999 + first.ObservedBackoffMS = 9999 + c.RecordAttempt(m, first, base, base.Add(120*time.Millisecond)) + c.RecordAttempt(m, okAttempt(), base.Add(1120*time.Millisecond), base.Add(1200*time.Millisecond)) + + got := c.entries[m].attempts + if got[0].DurationToHeadersMS != 120 { + t.Errorf("attempt 1 duration_to_headers_ms = %d, want 120", got[0].DurationToHeadersMS) + } + if got[0].ObservedBackoffMS != 0 { + t.Errorf("attempt 1 observed_backoff_ms = %d, want 0 (no predecessor)", got[0].ObservedBackoffMS) + } + if got[1].DurationToHeadersMS != 80 { + t.Errorf("attempt 2 duration_to_headers_ms = %d, want 80", got[1].DurationToHeadersMS) + } + // 1120 - 120: from the end of attempt 1 to the start of attempt 2. + if got[1].ObservedBackoffMS != 1000 { + t.Errorf("attempt 2 observed_backoff_ms = %d, want 1000", got[1].ObservedBackoffMS) + } +} + +// Timestamps from time.Now carry a monotonic reading and cannot invert, so this +// only guards hand-built times. It still has to hold: a negative duration in the +// report would be nonsense, and floored-at-zero is the honest answer when the +// clock says the attempt ended before it started. +func TestRecordAttemptFloorsInvertedTimestamps(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + + c.RecordAttempt(m, okAttempt(), base.Add(50*time.Millisecond), base) + // Starts before its predecessor ended, so the gap is negative too. + c.RecordAttempt(m, okAttempt(), base.Add(-50*time.Millisecond), base.Add(-100*time.Millisecond)) + + for i, a := range c.entries[m].attempts { + if a.DurationToHeadersMS != 0 || a.ObservedBackoffMS != 0 { + t.Errorf("attempt %d = duration %d, backoff %d, want both 0", + i+1, a.DurationToHeadersMS, a.ObservedBackoffMS) + } + } +} + +// An observer that reports a non-2xx status without classifying it contradicts +// the strongest fact it had. The failure mode this closes is silence, not noise: +// derived as a success, the attempt makes Finalize decide succeeded, the listing +// rule skips the request, and a failed request disappears from requests while +// total_requests still counts it. validateReport only walks listed requests and +// cannot see it. +func TestRecordAttemptRejectsUnclassifiedErrorStatus(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + recordUntimed(c, m, AttemptRecord{StatusCode: 500}) + c.Finalize(m, nil, false) + + // The attempt is kept so the state stays inspectable... + if got := c.entries[m].attempts; len(got) != 1 { + t.Fatalf("got %d attempts, want the attempt to be kept", len(got)) + } + // ...but the report is refused instead of silently omitting the request. + rep, err := c.Freeze("run-1") + if err == nil { + t.Fatalf("expected a construction error, got report %+v", rep) + } + if rep != nil { + t.Fatalf("a failed Freeze must not return a report: %+v", rep) + } + if !strings.Contains(err.Error(), "without a classification") { + t.Fatalf("unexpected error: %v", err) + } +} + +// The guard keys off the same boundary as classifyAttempt, so a 2xx never trips +// it: an unclassified 200 is the normal success path, and an unclassified 200 +// with an extra attempt is the x-should-retry case. +func TestRecordAttemptAcceptsUnclassifiedSuccessStatus(t *testing.T) { + for _, status := range []int{200, 201, 299, 0} { + c := NewRetryCollector() + m := testMeta() + recordUntimed(c, m, AttemptRecord{StatusCode: status}) + if got := c.entries[m].violation; got != "" { + t.Fatalf("status %d flagged a violation: %q", status, got) + } + } +} + +func TestRecordAttemptDropsRequestsWithoutIdentity(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + m.TaskType = "" // e.g. a scan or llm test request: no RequestMeta in context + recordUntimed(c, m, errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429)) + + if len(c.entries) != 0 { + t.Fatalf("expected the attempt to be dropped, got %d entries", len(c.entries)) + } +} + +// Every method tolerates a nil collector. This is the design premise that the +// reporting path can never fail a review: a call site that has no collector — or +// a future one that forgets to wire it — degrades to no report instead of +// panicking mid-review. +func TestNilCollectorIsInert(t *testing.T) { + var c *RetryCollector + m := testMeta() + + recordUntimed(c, m, errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429)) + c.ReviseLastAttempt(m, ErrorClassNetwork, FailurePhaseResponseDecode) + c.Finalize(m, errors.New("boom"), true) + + rep, err := c.Freeze("run-1") + if rep != nil || err != nil { + t.Fatalf("Freeze on a nil collector = (%+v, %v), want (nil, nil)", rep, err) + } +} + +// Invalid input is dropped rather than recorded. An unclassifiable correction is +// the dangerous case: writing it through would put an empty or bogus class on an +// attempt, and validateReport would then reject the whole report at Freeze — +// turning a caller mistake into a suppressed report. +func TestCollectorRejectsInvalidInput(t *testing.T) { + m := testMeta() + invalid := testMeta() + invalid.TaskType = "" + + t.Run("revision with an unknown class or phase", func(t *testing.T) { + for _, tc := range []struct { + name string + class ErrorClass + phase FailurePhase + }{ + {"empty class", "", FailurePhaseResponseDecode}, + {"empty phase", ErrorClassNetwork, ""}, + {"bogus class", ErrorClass("flaky"), FailurePhaseResponseDecode}, + {"bogus phase", ErrorClassNetwork, FailurePhase("decoding")}, + } { + t.Run(tc.name, func(t *testing.T) { + c := NewRetryCollector() + recordUntimed(c, m, okAttempt()) + c.ReviseLastAttempt(m, tc.class, tc.phase) + if got := c.entries[m].attempts[0]; got.Outcome != AttemptSuccess { + t.Fatalf("attempt was revised with invalid input: %+v", got) + } + }) + } + }) + + t.Run("revision without identity", func(t *testing.T) { + c := NewRetryCollector() + recordUntimed(c, m, okAttempt()) + c.ReviseLastAttempt(invalid, ErrorClassNetwork, FailurePhaseResponseDecode) + if got := c.entries[m].attempts[0]; got.Outcome != AttemptSuccess { + t.Fatalf("an unidentified revision reached another entry: %+v", got) + } + }) + + // A correction can arrive before any attempt was observed (the client + // boundary sees a decode error on a request that never entered the observer). + // There is nothing to revise and nothing to invent. + t.Run("revision with no attempt to revise", func(t *testing.T) { + c := NewRetryCollector() + c.ReviseLastAttempt(m, ErrorClassNetwork, FailurePhaseResponseDecode) + if len(c.entries) != 0 { + t.Fatalf("revision created an entry: %+v", c.entries) + } + }) + + t.Run("finalize without identity", func(t *testing.T) { + c := NewRetryCollector() + recordUntimed(c, m, okAttempt()) + c.Finalize(invalid, nil, false) + if c.entries[m].finalized { + t.Fatal("an unidentified Finalize finalized another entry") + } + }) +} + +func TestReviseLastAttempt(t *testing.T) { + t.Run("rewrites a success attempt", func(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + recordUntimed(c, m, okAttempt()) + c.ReviseLastAttempt(m, ErrorClassNetwork, FailurePhaseResponseDecode) + + last := c.entries[m].attempts[0] + if last.Outcome != AttemptError || last.ErrorClass != ErrorClassNetwork || + last.FailurePhase != FailurePhaseResponseDecode { + t.Fatalf("attempt not corrected: %+v", last) + } + if last.StatusCode != 200 { + t.Fatalf("correction dropped the observed status code: %+v", last) + } + }) + + // The precondition. A 500 already classified from its status code must not be + // rewritten to unknown/response_decode just because its error body failed to + // parse: the status code is the stronger fact. + t.Run("never overwrites a status-code classification", func(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + recordUntimed(c, m, errAttempt(ErrorClassProvider, FailurePhaseHTTP, 500)) + c.ReviseLastAttempt(m, ErrorClassUnknown, FailurePhaseResponseDecode) + + last := c.entries[m].attempts[0] + if last.ErrorClass != ErrorClassProvider || last.FailurePhase != FailurePhaseHTTP { + t.Fatalf("status-code classification was overwritten: %+v", last) + } + }) + + t.Run("stream and response status phases", func(t *testing.T) { + for _, phase := range []FailurePhase{FailurePhaseStream, FailurePhaseResponseStatus} { + c := NewRetryCollector() + m := testMeta() + recordUntimed(c, m, okAttempt()) + c.ReviseLastAttempt(m, ErrorClassProvider, phase) + if got := c.entries[m].attempts[0].FailurePhase; got != phase { + t.Fatalf("phase = %s, want %s", got, phase) + } + } + }) +} + +func TestFreezeReturnsNothingWhenNoRetryHappened(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + recordUntimed(c, m, okAttempt()) + c.Finalize(m, nil, false) + + rep, err := c.Freeze("run-1") + if err != nil { + t.Fatalf("Freeze error: %v", err) + } + if rep != nil { + t.Fatalf("expected no report for a first-try success, got %+v", rep) + } +} + +func TestFreezeAggregatesAndSorts(t *testing.T) { + c := NewRetryCollector() + + recovered := testMeta() + recovered.RequestNo = 2 + recordUntimed(c, recovered, errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429)) + recordUntimed(c, recovered, okAttempt()) + c.Finalize(recovered, nil, false) + + failed := testMeta() + failed.FilePath = "config.go" + recordUntimed(c, failed, errAttempt(ErrorClassProvider, FailurePhaseHTTP, 402)) + c.Finalize(failed, errors.New("payment required"), false) + + // A first-try success stays out of requests but still counts in + // total_requests. + quiet := testMeta() + quiet.FilePath = "quiet.go" + recordUntimed(c, quiet, okAttempt()) + c.Finalize(quiet, nil, false) + + rep, err := c.Freeze("run-1") + if err != nil { + t.Fatalf("Freeze error: %v", err) + } + if rep == nil { + t.Fatal("expected a report") + } + if rep.SchemaVersion != RetryReportSchemaVersion { + t.Fatalf("schema version = %q", rep.SchemaVersion) + } + if rep.TotalRequests != 3 || rep.RetriedRequests != 1 || rep.TotalRetries != 1 || + rep.RecoveredRequests != 1 || rep.FailedRequests != 1 { + t.Fatalf("aggregates wrong: %+v", *rep) + } + if len(rep.Requests) != 2 { + t.Fatalf("listed %d requests, want 2", len(rep.Requests)) + } + ids := []string{rep.Requests[0].LogicalRequestID, rep.Requests[1].LogicalRequestID} + if !sort.StringsAreSorted(ids) { + t.Fatalf("requests are not sorted by logical_request_id: %v", ids) + } +} + +// A request cancelled after a single clean attempt has no error attempt and no +// retry, yet it counts in cancelled_requests. It must still be listed, or the +// aggregates would not be verifiable from the report alone. +func TestFreezeListsCancelledRequestWithoutErrorAttempt(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + recordUntimed(c, m, okAttempt()) + c.Finalize(m, nil, true) + + rep, err := c.Freeze("run-1") + if err != nil { + t.Fatalf("Freeze error: %v", err) + } + if rep == nil || len(rep.Requests) != 1 { + t.Fatalf("cancelled request was not listed: %+v", rep) + } + if rep.Requests[0].Outcome != OutcomeCancelled || rep.CancelledRequests != 1 || rep.FailedRequests != 0 { + t.Fatalf("unexpected report: %+v", *rep) + } +} + +// The x-should-retry case: a retry with no error at all. "1 retry, 0 recovered, +// 0 failed" is the correct answer, not a counting bug. +func TestFreezeSucceededRequestWithExtraAttempt(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + directive := true + recordUntimed(c, m, AttemptRecord{StatusCode: 200, SDKRetryDirective: &directive}) + recordUntimed(c, m, okAttempt()) + c.Finalize(m, nil, false) + + rep, err := c.Freeze("run-1") + if err != nil { + t.Fatalf("Freeze error: %v", err) + } + if rep == nil || len(rep.Requests) != 1 { + t.Fatalf("expected the extra attempt to be reported: %+v", rep) + } + r := rep.Requests[0] + if r.Outcome != OutcomeSucceeded { + t.Fatalf("outcome = %s, want succeeded", r.Outcome) + } + if rep.RetriedRequests != 1 || rep.TotalRetries != 1 { + t.Fatalf("retry counts wrong: %+v", *rep) + } + if rep.RecoveredRequests != 0 || rep.FailedRequests != 0 { + t.Fatalf("nothing was recovered or failed: %+v", *rep) + } +} + +func TestFreezeRejectsOrderingViolations(t *testing.T) { + cases := []struct { + name string + mut func(*RetryCollector, RequestMeta) + want string + }{ + { + name: "double finalize", + mut: func(c *RetryCollector, m RequestMeta) { + c.Finalize(m, nil, false) + c.Finalize(m, nil, false) + }, + want: "Finalize called more than once", + }, + { + name: "attempt after finalize", + mut: func(c *RetryCollector, m RequestMeta) { + c.Finalize(m, nil, false) + recordUntimed(c, m, okAttempt()) + }, + want: "attempt recorded after Finalize", + }, + { + name: "revision after finalize", + mut: func(c *RetryCollector, m RequestMeta) { + c.Finalize(m, nil, false) + c.ReviseLastAttempt(m, ErrorClassNetwork, FailurePhaseResponseDecode) + }, + want: "attempt revised after Finalize", + }, + { + name: "never finalized", + mut: func(*RetryCollector, RequestMeta) {}, + want: "not finalized", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + recordUntimed(c, m, errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429)) + recordUntimed(c, m, okAttempt()) + tc.mut(c, m) + + rep, err := c.Freeze("run-1") + if err == nil { + t.Fatalf("expected a construction error, got report %+v", rep) + } + if rep != nil { + t.Fatalf("a failed Freeze must not return a report: %+v", rep) + } + if !strings.Contains(err.Error(), tc.want) { + t.Fatalf("error %q does not mention %q", err, tc.want) + } + }) + } +} + +// A construction error has to name the offending request: with --concurrency > 1 +// a run holds hundreds of logical requests, and the error reaches the user as +// part of runErr with no other context. +func TestFreezeErrorIdentifiesTheRequest(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + m.FilePath = "internal/pay/charge.go" + m.TaskType = "review_filter" + m.RequestNo = 7 + recordUntimed(c, m, okAttempt()) // recorded but never finalized + + _, err := c.Freeze("run-1") + if err == nil { + t.Fatal("expected a construction error") + } + for _, want := range []string{"not finalized", "file=internal/pay/charge.go", "task=review_filter", "request_no=7"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("error %q does not mention %q", err, want) + } + } +} + +// Freeze walks entries in logical_request_id order rather than map order, so a +// collector holding several broken entries always blames the same one. Under map +// order this error would rotate between runs and could not be pinned by a test +// or matched against a bug report. +func TestFreezeErrorIsDeterministic(t *testing.T) { + build := func() *RetryCollector { + c := NewRetryCollector() + for i := 1; i <= 8; i++ { + m := testMeta() + m.FilePath = fmt.Sprintf("file%02d.go", i) + m.RequestNo = i + recordUntimed(c, m, okAttempt()) // none of them finalized + } + return c + } + + first, err := build().Freeze("run-1") + if err == nil { + t.Fatalf("expected a construction error, got %+v", first) + } + for i := 0; i < 20; i++ { + _, again := build().Freeze("run-1") + if again == nil || again.Error() != err.Error() { + t.Fatalf("error is not deterministic: %v vs %v", err, again) + } + } +} + +func TestFreezeRejectsInvalidRunID(t *testing.T) { + for _, runID := range []string{"", "run\x001"} { + c := NewRetryCollector() + m := testMeta() + recordUntimed(c, m, errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429)) + recordUntimed(c, m, okAttempt()) + c.Finalize(m, nil, false) + + rep, err := c.Freeze(runID) + if err == nil || rep != nil { + t.Fatalf("run_id %q: got (%+v, %v), want (nil, error)", runID, rep, err) + } + } +} + +// Not reachable through the API: RecordAttempt is the only thing that creates an +// entry and it always appends. The entry is built directly because this guard is +// the last line of defense for "total_requests only counts metas with an +// attempt" — if some later layer registers a meta up front, the report must +// refuse rather than emit a request with an empty attempts array. +func TestFreezeRefusesEntryWithNoAttempt(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + c.entries[m] = &requestEntry{finalized: true, outcome: OutcomeSucceeded} + + rep, err := c.Freeze("run-1") + if err == nil || rep != nil { + t.Fatalf("got (%+v, %v), want (nil, error)", rep, err) + } + if !strings.Contains(err.Error(), "no attempt") { + t.Fatalf("unexpected error: %v", err) + } +} + +// The point is not the tampering — Freeze's own accounting cannot produce broken +// numbering. It is that validateReport is wired into Freeze rather than merely +// callable on its own, so a broken invariant suppresses the report instead of +// publishing self-contradictory numbers. +func TestFreezeSuppressesReportWhenValidationFails(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + recordUntimed(c, m, errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429)) + recordUntimed(c, m, okAttempt()) + c.Finalize(m, nil, false) + c.entries[m].attempts[1].Number = 7 + + rep, err := c.Freeze("run-1") + if err == nil || rep != nil { + t.Fatalf("got (%+v, %v), want (nil, error)", rep, err) + } + if !strings.Contains(err.Error(), "numbering") { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestValidateReportCatchesInconsistency(t *testing.T) { + base := func() *RetryReport { + return &RetryReport{ + SchemaVersion: RetryReportSchemaVersion, + TotalRequests: 1, + RetriedRequests: 1, + TotalRetries: 1, + RecoveredRequests: 1, + Requests: []RequestReport{{ + LogicalRequestID: "a", + Model: "m", + FilePath: "f", + TaskType: "t", + RequestNo: 1, + Outcome: OutcomeRecovered, + Attempts: []AttemptRecord{ + {Number: 1, Outcome: AttemptError, ErrorClass: ErrorClassRateLimited, FailurePhase: FailurePhaseHTTP, StatusCode: 429}, + {Number: 2, Outcome: AttemptSuccess, StatusCode: 200}, + }, + }}, + } + } + if err := validateReport(base()); err != nil { + t.Fatalf("baseline report rejected: %v", err) + } + + cases := map[string]func(*RetryReport){ + "total requests below listed": func(r *RetryReport) { r.TotalRequests = 0 }, + "retry count mismatch": func(r *RetryReport) { r.TotalRetries = 5 }, + "retried count mismatch": func(r *RetryReport) { r.RetriedRequests = 5 }, + "recovered count mismatch": func(r *RetryReport) { r.RecoveredRequests = 0 }, + "failed count mismatch": func(r *RetryReport) { r.FailedRequests = 1 }, + "cancelled count mismatch": func(r *RetryReport) { r.CancelledRequests = 1 }, + "non contiguous numbering": func(r *RetryReport) { r.Requests[0].Attempts[1].Number = 3 }, + "recovered without error": func(r *RetryReport) { r.Requests[0].Attempts[0] = AttemptRecord{Number: 1, Outcome: AttemptSuccess} }, + "success attempt carries class": func(r *RetryReport) { r.Requests[0].Attempts[1].ErrorClass = ErrorClassUnknown }, + "error attempt unclassified": func(r *RetryReport) { r.Requests[0].Attempts[0].ErrorClass = "" }, + "error attempt unphased": func(r *RetryReport) { r.Requests[0].Attempts[0].FailurePhase = "" }, + "unknown attempt outcome": func(r *RetryReport) { r.Requests[0].Attempts[1].Outcome = AttemptOutcome("weird") }, + "unknown outcome": func(r *RetryReport) { r.Requests[0].Outcome = Outcome("weird") }, + "request with no attempt": func(r *RetryReport) { r.Requests[0].Attempts = nil }, + "unexpected schema version": func(r *RetryReport) { r.SchemaVersion = "ocr.llm-retry-report/v0" }, + "incomplete identity": func(r *RetryReport) { r.Requests[0].FilePath = "" }, + // The other half of the succeeded contract: the listing rule keeps a + // clean single-attempt success out of the report, and an error attempt + // makes the outcome recovered, so a succeeded request carrying one is a + // contradiction from either direction. + "succeeded with an error attempt": func(r *RetryReport) { r.Requests[0].Outcome = OutcomeSucceeded }, + "duplicate id": func(r *RetryReport) { + r.Requests = append(r.Requests, r.Requests[0]) + r.TotalRequests = 2 + r.TotalRetries = 2 + r.RetriedRequests = 2 + r.RecoveredRequests = 2 + }, + "succeeded with a single attempt": func(r *RetryReport) { + r.Requests[0].Outcome = OutcomeSucceeded + r.Requests[0].Attempts = []AttemptRecord{{Number: 1, Outcome: AttemptSuccess}} + r.TotalRetries = 0 + r.RetriedRequests = 0 + r.RecoveredRequests = 0 + }, + } + for name, mut := range cases { + t.Run(name, func(t *testing.T) { + r := base() + mut(r) + if err := validateReport(r); err == nil { + t.Fatal("expected an invariant violation") + } + }) + } +} + +// provider is required but may be empty: an empty string stably denotes an +// unnamed endpoint and must not be omitted. +func TestProviderIsEmittedEvenWhenEmpty(t *testing.T) { + c := NewRetryCollector() + m := testMeta() + m.Provider = "" + recordUntimed(c, m, errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429)) + recordUntimed(c, m, okAttempt()) + c.Finalize(m, nil, false) + + rep, err := c.Freeze("run-1") + if err != nil || rep == nil { + t.Fatalf("Freeze = (%+v, %v)", rep, err) + } + blob, err := json.Marshal(rep.Requests[0]) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if !strings.Contains(string(blob), `"provider":""`) { + t.Fatalf("provider was omitted: %s", blob) + } +} + +// The security guarantee is structural rather than filter-based: the report has +// no free-text field, so there is nothing to redact. This test pins the exact +// set of plain-string fields, so adding one (an error message, a URL, a prompt) +// fails here and has to be argued for explicitly. Enum types are excluded +// automatically because their reflect.Type is not plain string. +func TestRetryReportHasNoUnexpectedTextFields(t *testing.T) { + want := []string{ + "RetryReport.Requests[].Attempts[].RequestID", + "RetryReport.Requests[].FilePath", + "RetryReport.Requests[].LogicalRequestID", + "RetryReport.Requests[].Model", + "RetryReport.Requests[].Provider", + "RetryReport.Requests[].TaskType", + "RetryReport.SchemaVersion", + } + + var got []string + var walk func(t reflect.Type, path string) + walk = func(rt reflect.Type, path string) { + switch rt.Kind() { + case reflect.Slice, reflect.Ptr: + walk(rt.Elem(), path+"[]") + case reflect.Struct: + for i := 0; i < rt.NumField(); i++ { + f := rt.Field(i) + walk(f.Type, path+"."+f.Name) + } + case reflect.String: + if rt == reflect.TypeOf("") { + got = append(got, path) + } + } + } + walk(reflect.TypeOf(RetryReport{}), "RetryReport") + sort.Strings(got) + if !reflect.DeepEqual(got, want) { + t.Fatalf("plain string fields changed:\n got %v\nwant %v", got, want) + } +} + +func TestRetryCollectorConcurrentUse(t *testing.T) { + c := NewRetryCollector() + const requests = 32 + + var wg sync.WaitGroup + for i := 1; i <= requests; i++ { + wg.Add(1) + go func(n int) { + defer wg.Done() + m := testMeta() + m.FilePath = fmt.Sprintf("file%02d.go", n) + m.RequestNo = n + recordUntimed(c, m, errAttempt(ErrorClassRateLimited, FailurePhaseHTTP, 429)) + recordUntimed(c, m, okAttempt()) + c.Finalize(m, nil, false) + }(i) + } + wg.Wait() + + rep, err := c.Freeze("run-1") + if err != nil { + t.Fatalf("Freeze error: %v", err) + } + if rep == nil { + t.Fatal("expected a report") + } + if rep.TotalRequests != requests || rep.RecoveredRequests != requests || + rep.TotalRetries != requests || len(rep.Requests) != requests { + t.Fatalf("aggregates wrong under concurrency: %+v", *rep) + } + ids := make([]string, len(rep.Requests)) + for i, r := range rep.Requests { + ids[i] = r.LogicalRequestID + } + if !sort.StringsAreSorted(ids) { + t.Fatal("report ordering is not stable") + } +} diff --git a/internal/llmloop/compression.go b/internal/llmloop/compression.go index 493ab306..eee23c93 100644 --- a/internal/llmloop/compression.go +++ b/internal/llmloop/compression.go @@ -227,16 +227,24 @@ func (r *Runner) runCompression(ctx context.Context, msgs []llm.Message, filePat compressionMsgs = append(compressionMsgs, llm.NewTextMessage(m.Role, content)) } + // The task record is created before the request, not after it, because the + // retry report keys request identity on RequestNo and that number only + // exists once the record does. The visible consequence is that the + // llm_request line reaches the session JSONL before the response: a run + // killed mid-request now leaves an llm_request with no response, which + // resume ignores (applyResumeLine has no case for it). + fs := r.deps.Session.GetOrCreateFileSession(filePath) + rec := fs.AppendTaskRecord(session.MemoryCompressionTask, compressionMsgs) + startTime := time.Now() - resp, err := r.deps.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{ + reqCtx := r.requestCtx(ctx, filePath, session.MemoryCompressionTask, rec.RequestNo) + resp, err := r.deps.LLMClient.CompletionsWithCtx(reqCtx, llm.ChatRequest{ Model: r.deps.Model, Messages: compressionMsgs, MaxTokens: r.deps.Template.CompletionTokenLimit(), }) duration := time.Since(startTime) - fs := r.deps.Session.GetOrCreateFileSession(filePath) - rec := fs.AppendTaskRecord(session.MemoryCompressionTask, compressionMsgs) if err != nil { rec.SetError(err, duration) // Return msgs unchanged: truncating to frozenEnd would discard all @@ -289,7 +297,11 @@ func (r *Runner) triggerAsyncCompression(ctx context.Context, st *compressionSta st.pendingJob = job st.mu.Unlock() + // Registered before the goroutine starts so WaitBackground can never miss + // a job that was launched but has not run yet. + r.bg.Add(1) go func() { + defer r.bg.Done() defer cancel() rebuilt, err := r.runCompression(asyncCtx, msgSnapshot, filePath) diff --git a/internal/llmloop/loop.go b/internal/llmloop/loop.go index cf99fd1f..9849aecf 100644 --- a/internal/llmloop/loop.go +++ b/internal/llmloop/loop.go @@ -39,6 +39,33 @@ type Deps struct { // in scan mode — scan adapters return a synthetic Diff whose // NewFileContent is the whole file and Diff is empty). DiffLookup func(path string) *model.Diff + + // NewRequestMeta builds the retry-report identity for one logical LLM + // request. Non-nil only for review: the retry report describes ocr review, + // and this Runner is shared with scan (internal/scan.Agent calls RunPerFile), + // so main_task, memory compression and re-location all run under both modes. + // + // The gate has to be this field rather than a Provider string, because an + // empty provider is a legitimate value for an unnamed endpoint — it cannot + // double as "identity disabled". Leaving it nil (what scan does) keeps every + // request exactly as it was before request identity existed. + // + // requestNo must be the RequestNo of the session.TaskRecord already created + // for this request, so the report joins against the session JSONL. + NewRequestMeta func(filePath string, taskType session.TaskType, requestNo int) llm.RequestMeta +} + +// requestCtx returns ctx carrying the identity of one logical LLM request, or +// ctx unchanged when identity is disabled (scan) or the meta is unusable. +// +// Callers must invoke it after AppendTaskRecord and pass that record's +// RequestNo — the fixed order is AppendTaskRecord -> requestCtx -> +// CompletionsWithCtx -> SetResponse/SetError. +func (r *Runner) requestCtx(ctx context.Context, filePath string, taskType session.TaskType, requestNo int) context.Context { + if r.deps.NewRequestMeta == nil { + return ctx + } + return llm.WithRequestMeta(ctx, r.deps.NewRequestMeta(filePath, taskType, requestNo)) } // Runner is a per-session (across files) executor of the LLM tool-use @@ -55,6 +82,11 @@ type Runner struct { warnings []AgentWarning toolCallsMu sync.Mutex toolCalls map[string]int64 + // bg tracks every background goroutine that can still issue an LLM + // request after RunPerFile returned. WaitBackground joins them so a + // retry-report Freeze at the run boundary cannot observe an + // un-finalized request. See WaitBackground. + bg sync.WaitGroup } // NewRunner returns a Runner bound to the given dependencies. @@ -62,6 +94,27 @@ func NewRunner(deps Deps) *Runner { return &Runner{deps: deps} } +// WaitBackground blocks until every background job started by this Runner has +// returned. Background memory compression is the only such job, and +// cancelPendingCompression cancels it without waiting — its goroutine can +// therefore still be inside an LLM request after RunPerFile returned. Callers +// that freeze a retry report at the run boundary must join here first: +// RetryCollector.Freeze rejects any request that has not been finalized and +// discards the whole report, which would otherwise be an intermittent race. +// +// Every pending job has already been cancelled by the time the last +// RunPerFile returns (cancelPendingCompression runs as a deferred call on +// every exit, and triggerAsyncCompression refuses to start a second job while +// one is pending), so this normally returns quickly — but the wait length +// ultimately depends on the LLM client honouring context cancellation, and no +// additional deadline is imposed here: the job already carries its own +// timeout. +// Scan does not currently call this because it freezes no retry report; its +// analogous session-finalization race is outside this change. +func (r *Runner) WaitBackground() { + r.bg.Wait() +} + // TotalInputTokens returns the accumulated input/prompt tokens from all LLM calls. func (r *Runner) TotalInputTokens() int64 { return atomic.LoadInt64(&r.totalInputTokens) } @@ -202,8 +255,12 @@ func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath rec := fs.AppendTaskRecord(session.MainTask, append([]llm.Message(nil), messages...)) startTime := time.Now() + // Scoped to this round: ctx itself must stay identity-free so each + // iteration's meta replaces the previous one instead of nesting. + reqCtx := r.requestCtx(ctx, newPath, session.MainTask, rec.RequestNo) + _, llmSpan := telemetry.StartLLMSpan(ctx, r.deps.Model) - resp, err := r.deps.LLMClient.CompletionsWithCtx(ctx, llm.ChatRequest{ + resp, err := r.deps.LLMClient.CompletionsWithCtx(reqCtx, llm.ChatRequest{ Model: r.deps.Model, Messages: messages, Tools: r.deps.MainToolDefs, @@ -420,11 +477,23 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T } if d != nil { if !diff.ResolveComment(cm, d) && r.deps.Template.ReLocationTask != nil { + // rlStart stays ahead of prompt construction, which is + // where it sat when ReLocateComment built the messages + // itself — moving it would silently change what + // TaskRecord.Duration measures. rlStart := time.Now() - _, resp, msgs := diff.ReLocateComment(rctx, cm, d, r.deps.LLMClient, r.deps.Template.ReLocationTask, r.deps.Model, r.deps.Template.CompletionTokenLimit()) - if msgs != nil { + msgs := diff.BuildReLocationMessages(cm, d, r.deps.Template.ReLocationTask) + if len(msgs) > 0 { fs := r.deps.Session.GetOrCreateFileSession(cm.Path) rlRec := fs.AppendTaskRecord(session.ReLocationTask, msgs) + // FilePath is cm.Path so it cannot drift from the file + // session opened above — that join is what the report + // needs. It equals newPath whenever newPath is set, + // because the path arg is overridden with it further + // up, but reading it from the comment keeps the two + // aligned without depending on that. + reqCtx := r.requestCtx(rctx, cm.Path, session.ReLocationTask, rlRec.RequestNo) + _, resp := diff.ReLocateComment(reqCtx, cm, d, r.deps.LLMClient, msgs, r.deps.Model, r.deps.Template.CompletionTokenLimit()) if resp != nil { rlRec.SetResponse(resp, time.Since(rlStart)) if resp.Usage != nil { diff --git a/internal/llmloop/retry_background_test.go b/internal/llmloop/retry_background_test.go new file mode 100644 index 00000000..a33becc3 --- /dev/null +++ b/internal/llmloop/retry_background_test.go @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llmloop + +import ( + "context" + "strings" + "sync" + "testing" + "time" + + "github.com/alibaba/open-code-review/internal/llm" + "github.com/alibaba/open-code-review/internal/session" +) + +// blockingCompressionClient stands in for the real client boundary of a +// background memory-compression request: it records the request's attempts on a +// shared collector, blocks until the test releases it, and only then finalizes +// the logical request — the same order internal/llm's CompletionsWithCtx uses +// (observer per attempt, Finalize in the boundary defer). Blocking is what makes +// the un-finalized window observable; a real background job's window is however +// long the request takes. +type blockingCompressionClient struct { + collector *llm.RetryCollector + started chan struct{} + release chan struct{} + once sync.Once +} + +func newBlockingCompressionClient(c *llm.RetryCollector) *blockingCompressionClient { + return &blockingCompressionClient{ + collector: c, + started: make(chan struct{}), + release: make(chan struct{}), + } +} + +func (c *blockingCompressionClient) CompletionsWithCtx(ctx context.Context, _ llm.ChatRequest) (*llm.ChatResponse, error) { + meta, ok := llm.RequestMetaFromContext(ctx) + if !ok { + return emptyResponse(), nil + } + base := time.Date(2026, 8, 7, 12, 0, 0, 0, time.UTC) + // One retried request, so the frozen report has something to list rather + // than being the (nil, nil) "nothing worth reporting" shape. + c.collector.RecordAttempt(meta, llm.AttemptRecord{ + ErrorClass: llm.ErrorClassRateLimited, FailurePhase: llm.FailurePhaseHTTP, StatusCode: 429, + }, base, base.Add(10*time.Millisecond)) + c.collector.RecordAttempt(meta, llm.AttemptRecord{}, base.Add(time.Second), base.Add(time.Second+10*time.Millisecond)) + + c.once.Do(func() { close(c.started) }) + <-c.release + + c.collector.Finalize(meta, nil, false) + summary := "compressed summary" + return &llm.ChatResponse{Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &summary}}}}, nil +} + +// #368 P5 决策 1: the run boundary joins background memory compression before +// the report is frozen. Without that join, Freeze can see a request that +// recorded attempts but was never finalized, which is an invariant violation and +// drops the whole run's report — so the barrier is what makes the report +// publishable at all, not just more complete. +func TestWaitBackground_JoinsCompressionBeforeFreeze(t *testing.T) { + collector := llm.NewRetryCollector() + client := newBlockingCompressionClient(collector) + r, msgs := newCompressionRunner(t, client, metaFactory("openai", "fake")) + + st := &compressionState{} + r.triggerAsyncCompression(context.Background(), st, msgs, "test.go") + + // The job is mid-request: its attempts are recorded, its outcome is not. + <-client.started + if rep, err := collector.Freeze("run-uuid"); err == nil { + t.Fatalf("an un-finalized background request must fail Freeze, got report %+v", rep) + } else if !strings.Contains(err.Error(), "not finalized") { + t.Errorf("Freeze error = %v, want it to name the un-finalized request", err) + } + + close(client.release) + r.WaitBackground() + + rep, err := collector.Freeze("run-uuid") + if err != nil { + t.Fatalf("Freeze after WaitBackground: %v", err) + } + if rep == nil { + t.Fatal("expected a report for the retried compression request") + } + if len(rep.Requests) != 1 { + t.Fatalf("report lists %d requests, want the compression one: %+v", len(rep.Requests), rep.Requests) + } + got := rep.Requests[0] + if got.TaskType != string(session.MemoryCompressionTask) { + t.Errorf("task_type = %q, want %q", got.TaskType, session.MemoryCompressionTask) + } + if got.Outcome != llm.OutcomeRecovered { + t.Errorf("outcome = %q, want recovered", got.Outcome) + } + if rep.RetriedRequests != 1 || rep.TotalRetries != 1 || rep.RecoveredRequests != 1 { + t.Errorf("aggregates = %+v, want one retried/recovered request", rep) + } +} + +// WaitBackground must also be safe when no job ever started and when it is +// called twice, because the run boundary calls it unconditionally on every exit. +func TestWaitBackground_NoJobIsANoOp(t *testing.T) { + collector := llm.NewRetryCollector() + r, _ := newCompressionRunner(t, newBlockingCompressionClient(collector), metaFactory("openai", "fake")) + + r.WaitBackground() + r.WaitBackground() + + rep, err := collector.Freeze("run-uuid") + if err != nil { + t.Fatalf("Freeze: %v", err) + } + if rep != nil { + t.Errorf("a run that made no request must report nothing, got %+v", rep) + } +} diff --git a/internal/llmloop/retry_identity_test.go b/internal/llmloop/retry_identity_test.go new file mode 100644 index 00000000..5c673eca --- /dev/null +++ b/internal/llmloop/retry_identity_test.go @@ -0,0 +1,356 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llmloop + +import ( + "context" + "strings" + "sync" + "testing" + + "github.com/alibaba/open-code-review/internal/config/template" + "github.com/alibaba/open-code-review/internal/llm" + "github.com/alibaba/open-code-review/internal/model" + "github.com/alibaba/open-code-review/internal/session" + "github.com/alibaba/open-code-review/internal/tool" +) + +// capturedRequest is one observed CompletionsWithCtx call and the request +// identity its context carried, if any. +type capturedRequest struct { + meta llm.RequestMeta + ok bool +} + +// metaCaptureClient records the RequestMeta of every request it receives. It is +// the only way to check identity from outside package llm: the meta travels in +// the context, so the client is where it becomes observable. +type metaCaptureClient struct { + mu sync.Mutex + captured []capturedRequest + // respond is called with the zero-based call index so a test can drive a + // multi-round tool loop. + respond func(n int) *llm.ChatResponse +} + +func (c *metaCaptureClient) CompletionsWithCtx(ctx context.Context, _ llm.ChatRequest) (*llm.ChatResponse, error) { + meta, ok := llm.RequestMetaFromContext(ctx) + c.mu.Lock() + n := len(c.captured) + c.captured = append(c.captured, capturedRequest{meta: meta, ok: ok}) + c.mu.Unlock() + if c.respond == nil { + return emptyResponse(), nil + } + return c.respond(n), nil +} + +func (c *metaCaptureClient) requests() []capturedRequest { + c.mu.Lock() + defer c.mu.Unlock() + return append([]capturedRequest(nil), c.captured...) +} + +func emptyResponse() *llm.ChatResponse { + content := "" + return &llm.ChatResponse{ + Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &content}}}, + Model: "fake", + } +} + +// metaFactory builds the same closure internal/agent injects, so these tests +// exercise the review wiring rather than a test-only shape. +func metaFactory(provider, modelName string) func(string, session.TaskType, int) llm.RequestMeta { + return func(filePath string, taskType session.TaskType, requestNo int) llm.RequestMeta { + return llm.RequestMeta{ + Provider: provider, + Model: modelName, + FilePath: filePath, + TaskType: string(taskType), + RequestNo: requestNo, + } + } +} + +func wantMeta(t *testing.T, got capturedRequest, want llm.RequestMeta) { + t.Helper() + if !got.ok { + t.Fatalf("request carried no identity, want %+v", want) + } + if got.meta != want { + t.Errorf("meta = %+v, want %+v", got.meta, want) + } +} + +// TestRunPerFile_MainTaskIdentity checks that every main_task round carries the +// identity of the TaskRecord created for it, including the per-round RequestNo. +// An empty provider is covered as its own case: it is the real value for an +// unnamed endpoint, so it must still produce identity rather than be read as +// "no meta". +func TestRunPerFile_MainTaskIdentity(t *testing.T) { + for _, provider := range []string{"openai", ""} { + name := provider + if name == "" { + name = "empty-provider" + } + t.Run(name, func(t *testing.T) { + client := &metaCaptureClient{} + client.respond = func(n int) *llm.ChatResponse { + if n == 0 { + return fileReadToolCallResponse("call_1", `{"path":"main.go"}`) + } + return taskDoneResponse() + } + + deps := newTestDeps(client) + deps.NewRequestMeta = metaFactory(provider, deps.Model) + runner := NewRunner(deps) + + if _, _, err := runner.RunPerFile( + context.Background(), + []llm.Message{llm.NewTextMessage("user", "review this file")}, + "main.go", + ); err != nil { + t.Fatalf("RunPerFile: %v", err) + } + + reqs := client.requests() + if len(reqs) != 2 { + t.Fatalf("got %d requests, want 2", len(reqs)) + } + for i, got := range reqs { + wantMeta(t, got, llm.RequestMeta{ + Provider: provider, + Model: "fake", + FilePath: "main.go", + TaskType: string(session.MainTask), + RequestNo: i + 1, + }) + } + + // The report joins on these fields, so they must match the records + // the session actually wrote, not just each other. + fs := deps.Session.GetOrCreateFileSession("main.go") + if n := len(fs.TaskRecords[session.MainTask]); n != 2 { + t.Fatalf("session holds %d main_task records, want 2", n) + } + for i, rec := range fs.TaskRecords[session.MainTask] { + if reqs[i].meta.RequestNo != rec.RequestNo { + t.Errorf("request %d: meta RequestNo = %d, record = %d", i, reqs[i].meta.RequestNo, rec.RequestNo) + } + } + }) + } +} + +// TestRunPerFile_NoIdentityWhenFactoryNil is the scan guarantee: the Runner is +// shared, and with NewRequestMeta left nil no request may carry identity. +func TestRunPerFile_NoIdentityWhenFactoryNil(t *testing.T) { + client := &metaCaptureClient{respond: func(int) *llm.ChatResponse { return taskDoneResponse() }} + runner := NewRunner(newTestDeps(client)) // NewRequestMeta unset, as scan leaves it + + if _, _, err := runner.RunPerFile( + context.Background(), + []llm.Message{llm.NewTextMessage("user", "review this file")}, + "main.go", + ); err != nil { + t.Fatalf("RunPerFile: %v", err) + } + + reqs := client.requests() + if len(reqs) != 1 { + t.Fatalf("got %d requests, want 1", len(reqs)) + } + if reqs[0].ok { + t.Errorf("scan request carried identity %+v, want none", reqs[0].meta) + } +} + +// newCompressionRunner returns a Runner whose template forces runCompression to +// issue a request, plus the message slice to feed it. +func newCompressionRunner(t *testing.T, client llm.LLMClient, factory func(string, session.TaskType, int) llm.RequestMeta) (*Runner, []llm.Message) { + t.Helper() + sess := session.New(t.TempDir(), "main", "fake", session.SessionOptions{ReviewMode: "diff"}) + r := NewRunner(Deps{ + LLMClient: client, + Model: "fake", + Template: template.Template{ + MemoryCompressionTask: template.LlmConversation{ + Messages: []template.ChatMessage{{Role: "user", Content: "Summarize: {{context}}"}}, + }, + MaxTokens: 50, + }, + CommentCollector: tool.NewCommentCollector(), + Session: sess, + NewRequestMeta: factory, + }) + + msgs := []llm.Message{ + llm.NewTextMessage("system", "sys"), + llm.NewTextMessage("user", "prompt"), + } + for i := 0; i < 10; i++ { + msgs = append(msgs, llm.NewTextMessage("assistant", strings.Repeat("word ", 100))) + msgs = append(msgs, llm.NewTextMessage("tool", strings.Repeat("data ", 50))) + } + return r, msgs +} + +// TestRunCompression_Identity covers both compression paths at once: the +// synchronous one is this call, and the async one reaches the same function +// through triggerAsyncCompression, so identity is stamped for both. +func TestRunCompression_Identity(t *testing.T) { + summary := "compressed summary" + client := &metaCaptureClient{respond: func(int) *llm.ChatResponse { + return &llm.ChatResponse{Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &summary}}}} + }} + r, msgs := newCompressionRunner(t, client, metaFactory("openai", "fake")) + + if _, err := r.runCompression(context.Background(), msgs, "test.go"); err != nil { + t.Fatalf("runCompression: %v", err) + } + + reqs := client.requests() + if len(reqs) != 1 { + t.Fatalf("got %d requests, want 1", len(reqs)) + } + wantMeta(t, reqs[0], llm.RequestMeta{ + Provider: "openai", + Model: "fake", + FilePath: "test.go", + TaskType: string(session.MemoryCompressionTask), + RequestNo: 1, + }) +} + +func TestRunCompression_NoIdentityWhenFactoryNil(t *testing.T) { + summary := "compressed summary" + client := &metaCaptureClient{respond: func(int) *llm.ChatResponse { + return &llm.ChatResponse{Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &summary}}}} + }} + r, msgs := newCompressionRunner(t, client, nil) + + if _, err := r.runCompression(context.Background(), msgs, "test.go"); err != nil { + t.Fatalf("runCompression: %v", err) + } + + reqs := client.requests() + if len(reqs) != 1 { + t.Fatalf("got %d requests, want 1", len(reqs)) + } + if reqs[0].ok { + t.Errorf("scan compression carried identity %+v, want none", reqs[0].meta) + } +} + +// TestReLocation_Identity pins the field that is easy to get wrong: FilePath is +// the comment's path, which is the file session the re-location record was +// written to. It normally equals the tool loop's newPath, because executeToolCall +// overrides the path argument with it — so the test leaves newPath empty, the one +// case where the argument survives, to show which of the two identity follows. +func TestReLocation_Identity(t *testing.T) { + collector := tool.NewCommentCollector() + reg := tool.NewRegistry() + reg.Register(&tool.CodeCommentProvider{Collector: collector}) + reg.Freeze() + + sess := session.New(t.TempDir(), "main", "fake", session.SessionOptions{ReviewMode: "diff"}) + // No fenced code block in the reply: re-location fails to improve the match, + // which keeps the assertion on identity rather than on resolution. + reply := "cannot find it" + client := &metaCaptureClient{respond: func(int) *llm.ChatResponse { + return &llm.ChatResponse{Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &reply}}}} + }} + + r := NewRunner(Deps{ + LLMClient: client, + Model: "fake", + Template: template.Template{ + MaxTokens: 10000, + ReLocationTask: &template.LlmConversation{ + Messages: []template.ChatMessage{{Role: "user", Content: "relocate {suggestion_content} in {diff} near {existing_code}"}}, + }, + }, + Tools: reg, + CommentCollector: collector, + Session: sess, + DiffLookup: func(path string) *model.Diff { + return &model.Diff{NewPath: path, NewFileContent: "line one\nline two\n"} + }, + NewRequestMeta: metaFactory("openai", "fake"), + }) + + // newPath is empty so the path override does not fire and the comment keeps + // the target the tool call named, other.go. + cp := r.executeToolCall(context.Background(), "", llm.ToolCall{ + Function: llm.FunctionCall{ + Name: tool.CodeComment.Name(), + Arguments: `{"path":"other.go","comments":[{"content":"issue","existing_code":"no such code"}]}`, + }, + }, nil, "") + if cp.Data != tool.CommentSucceed { + t.Fatalf("cp.Data = %q, want CommentSucceed", cp.Data) + } + + reqs := client.requests() + if len(reqs) != 1 { + t.Fatalf("got %d requests, want 1", len(reqs)) + } + wantMeta(t, reqs[0], llm.RequestMeta{ + Provider: "openai", + Model: "fake", + FilePath: "other.go", + TaskType: string(session.ReLocationTask), + RequestNo: 1, + }) + if n := len(sess.GetOrCreateFileSession("other.go").TaskRecords[session.ReLocationTask]); n != 1 { + t.Errorf("other.go holds %d re-location records, want 1", n) + } +} + +// TestReLocation_NoRequestWithoutTemplate covers the branch that skips both the +// session record and the request: with no prompt there is nothing to send, so +// no orphan re-location record may appear. +func TestReLocation_NoRequestWithoutTemplate(t *testing.T) { + collector := tool.NewCommentCollector() + reg := tool.NewRegistry() + reg.Register(&tool.CodeCommentProvider{Collector: collector}) + reg.Freeze() + + sess := session.New(t.TempDir(), "main", "fake", session.SessionOptions{ReviewMode: "diff"}) + client := &metaCaptureClient{} + + r := NewRunner(Deps{ + LLMClient: client, + Model: "fake", + Template: template.Template{ + MaxTokens: 10000, + // Present but empty: BuildReLocationMessages returns nil, which the + // caller must treat exactly like a nil task. + ReLocationTask: &template.LlmConversation{}, + }, + Tools: reg, + CommentCollector: collector, + Session: sess, + DiffLookup: func(path string) *model.Diff { + return &model.Diff{NewPath: path, NewFileContent: "line one\nline two\n"} + }, + NewRequestMeta: metaFactory("openai", "fake"), + }) + + r.executeToolCall(context.Background(), "", llm.ToolCall{ + Function: llm.FunctionCall{ + Name: tool.CodeComment.Name(), + Arguments: `{"path":"other.go","comments":[{"content":"issue","existing_code":"no such code"}]}`, + }, + }, nil, "") + + if reqs := client.requests(); len(reqs) != 0 { + t.Fatalf("got %d requests, want 0", len(reqs)) + } + if n := len(sess.GetOrCreateFileSession("other.go").TaskRecords[session.ReLocationTask]); n != 0 { + t.Errorf("other.go holds %d re-location records, want 0", n) + } +} diff --git a/internal/scan/agent.go b/internal/scan/agent.go index 7fba079b..aed33088 100644 --- a/internal/scan/agent.go +++ b/internal/scan/agent.go @@ -146,6 +146,9 @@ func NewAgent(args Args) *Agent { // line-number resolver (resolveFromFileContent) can match against // the full file content of the scanned file. DiffLookup: a.lookupDiff, + // NewRequestMeta is deliberately left nil. The retry report describes + // ocr review; scan shares this Runner, and a nil factory is what keeps + // scan's requests out of the report. See llmloop.Deps.NewRequestMeta. }) return a } diff --git a/internal/scan/retry_identity_test.go b/internal/scan/retry_identity_test.go new file mode 100644 index 00000000..b6525471 --- /dev/null +++ b/internal/scan/retry_identity_test.go @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package scan + +import ( + "context" + "sync" + "testing" + + "github.com/alibaba/open-code-review/internal/config/template" + "github.com/alibaba/open-code-review/internal/llm" + "github.com/alibaba/open-code-review/internal/model" + "github.com/alibaba/open-code-review/internal/session" + "github.com/alibaba/open-code-review/internal/tool" +) + +// identityProbeClient records, for every request it receives, whether the +// context carried a retry-report RequestMeta. +type identityProbeClient struct { + mu sync.Mutex + withMeta []llm.RequestMeta + callCount int + reply string +} + +func (c *identityProbeClient) CompletionsWithCtx(ctx context.Context, _ llm.ChatRequest) (*llm.ChatResponse, error) { + c.mu.Lock() + c.callCount++ + if meta, ok := llm.RequestMetaFromContext(ctx); ok { + c.withMeta = append(c.withMeta, meta) + } + c.mu.Unlock() + + reply := c.reply + return &llm.ChatResponse{ + Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &reply}}}, + Usage: &llm.UsageInfo{PromptTokens: 1, CompletionTokens: 1}, + }, nil +} + +func (c *identityProbeClient) assertClean(t *testing.T, wantCalls int) { + t.Helper() + c.mu.Lock() + defer c.mu.Unlock() + if c.callCount != wantCalls { + t.Fatalf("got %d requests, want %d", c.callCount, wantCalls) + } + if len(c.withMeta) != 0 { + t.Errorf("scan requests carried identity %+v, want none", c.withMeta) + } +} + +// newProbeAgent builds a scan Agent whose only non-default piece is the probe +// client, so the assertions describe NewAgent's real wiring. +func newProbeAgent(t *testing.T, tpl template.ScanTemplate, client *identityProbeClient, collector *tool.CommentCollector) *Agent { + t.Helper() + if collector == nil { + collector = tool.NewCommentCollector() + } + a := NewAgent(Args{ + Template: tpl, + LLMClient: client, + Model: "test", + CommentCollector: collector, + Tools: tool.NewRegistry(), + Session: session.New(t.TempDir(), "main", "test", session.SessionOptions{ + ReviewMode: session.ReviewModeFullScan, + }), + }) + a.currentDate = "2026-08-07 10:00" + return a +} + +// TestScanRequestsCarryNoIdentity is the gate that keeps scan out of the retry +// report. It covers scan's three own request types; the three it reaches through +// the shared llmloop Runner are covered there by the nil-factory cases, which +// this file's sibling in internal/llmloop asserts. +func TestScanRequestsCarryNoIdentity(t *testing.T) { + t.Run("plan", func(t *testing.T) { + tpl := makeTemplateWithFullScan() + tpl.PlanTask = &template.LlmConversation{ + Messages: []template.ChatMessage{{Role: "user", Content: "plan {{current_file_path}} {{file_content}}"}}, + } + client := &identityProbeClient{reply: `{"summary":"s","checkpoints":[]}`} + a := newProbeAgent(t, tpl, client, nil) + + a.maybeRunPlan(context.Background(), model.ScanItem{Path: "h.go", Content: "package h\n"}, "rule") + client.assertClean(t, 1) + }) + + t.Run("project summary", func(t *testing.T) { + tpl := makeTemplateWithFullScan() + tpl.ProjectSummaryTask = &template.LlmConversation{ + Messages: []template.ChatMessage{{Role: "user", Content: "summarize {{all_comments}}"}}, + } + client := &identityProbeClient{reply: "overall summary"} + a := newProbeAgent(t, tpl, client, nil) + + a.maybeRunProjectSummary(context.Background(), []model.LlmComment{ + {Path: "a.go", Content: "missing error check"}, + {Path: "b.go", Content: "no input validation"}, + }) + client.assertClean(t, 1) + }) + + t.Run("dedup", func(t *testing.T) { + tpl := makeTemplateWithFullScan() + tpl.DedupTask = &template.LlmConversation{ + Messages: []template.ChatMessage{{Role: "user", Content: "dedup {{batch_comments}}"}}, + } + collector := tool.NewCommentCollector() + collector.Add(model.LlmComment{Path: "a.go", Content: "dup 1"}) + collector.Add(model.LlmComment{Path: "a.go", Content: "dup 2"}) + collector.Add(model.LlmComment{Path: "b.go", Content: "unique"}) + + client := &identityProbeClient{reply: `{"groups":[{"members":["c-0","c-1"],"merged_content":"combined"},{"members":["c-2"]}]}`} + a := newProbeAgent(t, tpl, client, collector) + + a.maybeRunDedup(context.Background(), 0, 0) + client.assertClean(t, 1) + }) + + t.Run("main task via shared runner", func(t *testing.T) { + client := &identityProbeClient{reply: "no findings"} + a := newProbeAgent(t, makeTemplateWithFullScan(), client, nil) + + // executeSubtask drives llmloop.RunPerFile, the code path scan shares + // with review — so this is the assertion that NewAgent leaves + // Deps.NewRequestMeta nil. + if _, _, err := a.executeSubtask(context.Background(), model.ScanItem{ + Path: "h.go", + Content: "package h\n", + }); err != nil { + t.Fatalf("executeSubtask: %v", err) + } + if client.callCount == 0 { + t.Fatal("expected at least one main_task request") + } + client.mu.Lock() + defer client.mu.Unlock() + if len(client.withMeta) != 0 { + t.Errorf("scan main_task carried identity %+v, want none", client.withMeta) + } + }) +} diff --git a/internal/session/resume_orphan_request_test.go b/internal/session/resume_orphan_request_test.go new file mode 100644 index 00000000..38451d23 --- /dev/null +++ b/internal/session/resume_orphan_request_test.go @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package session + +import ( + "os" + "path/filepath" + "testing" +) + +// orphanLLMRequestLine is the shape WriteLLMRequest emits, with no llm_response +// after it. Since the task record is now created before the HTTP call rather +// than after it, a run killed mid-request leaves exactly this in the JSONL. +const orphanLLMRequestLine = `{"uuid":"u2","parentUuid":"u1","type":"llm_request","sessionId":"orphan-session",` + + `"timestamp":"2026-08-07T02:00:00Z","filePath":"b.go","taskType":"main_task","request_no":1,` + + `"messages":[{"role":"user","content":"review b.go"}]}` + +// TestLoadResumeState_IgnoresOrphanLLMRequest is the regression guard for that +// reordering: resume must treat a request with no response as absent, neither +// erroring out nor counting b.go as completed. applyResumeLine has no case for +// llm_request, and this test is what keeps it that way. +func TestLoadResumeState_IgnoresOrphanLLMRequest(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + repoDir := "/test/orphan" + sessionID := "orphan-session" + path, err := SessionFilePath(repoDir, sessionID) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + t.Fatal(err) + } + + // a.go finished before the kill; b.go only got as far as its request. + content := `{"type":"session_start","sessionId":"orphan-session","cwd":"/test/orphan","reviewMode":"range"}` + "\n" + + `{"type":"review_item_done","filePath":"a.go","fingerprint":"fp-a","comments":[{"content":"comment-a"}]}` + "\n" + + orphanLLMRequestLine + "\n" + if err := os.WriteFile(path, []byte(content), 0600); err != nil { + t.Fatal(err) + } + + state, err := LoadResumeState(repoDir, sessionID) + if err != nil { + t.Fatalf("LoadResumeState: %v", err) + } + if state.CompletedCount() != 1 { + t.Errorf("CompletedCount = %d, want 1 (only a.go)", state.CompletedCount()) + } + if _, ok := state.Item("fp-a"); !ok { + t.Error("fp-a should have survived the orphan request line") + } + for fp := range state.Items { + if state.Items[fp].FilePath == "b.go" { + t.Errorf("b.go was resumed as completed from an orphan llm_request: %+v", state.Items[fp]) + } + } +} + +// TestApplyResumeLine_OrphanLLMRequestIsNoOp is the same guarantee at the unit +// level: the line must change nothing at all, not merely avoid an error. +func TestApplyResumeLine_OrphanLLMRequestIsNoOp(t *testing.T) { + s := &ResumeState{Items: make(map[string]ResumeItem)} + if err := s.applyResumeLine([]byte(orphanLLMRequestLine)); err != nil { + t.Fatalf("applyResumeLine: %v", err) + } + if len(s.Items) != 0 { + t.Errorf("Items = %+v, want empty", s.Items) + } + if s.SessionID != "" || s.ReviewMode != "" { + t.Errorf("llm_request mutated state: SessionID=%q ReviewMode=%q", s.SessionID, s.ReviewMode) + } +}