diff --git a/CHANGELOG.md b/CHANGELOG.md index b820339..b3464f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,31 @@ and the project adheres to [Semantic Versioning 2.0.0](https://semver.org/spec/v ## [Unreleased] +### Changed +- **Repair-oriented structured-output recovery (ADR-0031).** When a provider + returns output that fails findings-JSON parsing, the pipeline no longer retries + with the byte-identical request. **Phase 0** — `ParseFindings` now unwraps a + lone markdown code-fence pair (` ```json … ``` `) before parsing, so a provider + that fenced otherwise-valid JSON (common for prompt-only / OpenAI-compatible / + Ollama models) parses on the **first** attempt with **zero** retries; non-fenced + input is byte-identical, so the cache key is unchanged and cached results that + happen to be fenced now render as findings on replay. **Phase 1+2** — the parse + error is classified (empty / prose / truncated-JSON / schema violation) and the + single retry sends a **failure-mode-specific repair prompt** instead of the + identical one: a hard "JSON only, no prose" reset for prose / schema-ignored + output, or a "complete the JSON" nudge (with the partial output embedded and a + raised `max_tokens` ceiling) for a truncated attempt. The retry stays a fresh + single-shot request; the terminal `markdown-fallback` degrade state and summed + token usage are unchanged. CLI plain-text providers are unaffected. + +### Added +- **Recovery observability (ADR-0031).** New additive optional + `meta.retry_count` / `meta.degrade_reason` fields in the `--json` output + (`omitempty`, so a clean review is byte-for-byte the same and schema stays `1`) + plus two `--verbose` footer lines, making retries and degrades visible for + `make eval` model comparison. Both are live-call-only (a cache replay reports + neither). + ## [1.12.0] - 2026-06-21 ### Added diff --git a/internal/cli/remote_pr.go b/internal/cli/remote_pr.go index 0fdc950..bf9b16d 100644 --- a/internal/cli/remote_pr.go +++ b/internal/cli/remote_pr.go @@ -412,6 +412,8 @@ func runRemotePRLocal(cmd *cobra.Command, prID string, f remotePRFlags, runner r content string usage provider.Usage format string + retries int + degrade string ) if plainText { resp, callErr := prov.Review(ctx, req) @@ -421,8 +423,7 @@ func runRemotePRLocal(cmd *cobra.Command, prID string, f remotePRFlags, runner r } content, usage, format = resp.Content, resp.Usage, cache.FormatPlainText } else { - var callErr error - content, usage, format, callErr = tryStructuredReview(ctx, prov, req, func() { + outcome, callErr := tryStructuredReview(ctx, prov, req, func() { prog.Soft() prog.Start(cat.T("progress.retrying")) }) @@ -430,6 +431,8 @@ func runRemotePRLocal(cmd *cobra.Command, prID string, f remotePRFlags, runner r prog.Fail(callErr) return fmt.Errorf("provider %s: %w", prov.Name(), callErr) } + content, usage, format = outcome.Content, outcome.Usage, outcome.Format + retries, degrade = outcome.Retries, outcome.DegradeReason } prog.Finish() prog.Clear() @@ -444,17 +447,19 @@ func runRemotePRLocal(cmd *cobra.Command, prID string, f remotePRFlags, runner r } meta := render.Meta{ - Provider: prov.Name(), - Model: model, - Lang: app.Lang.Code, - Usage: usage, - Cost: resolvePricing(app.Config, prov, model).Cost(usage), - Latency: latency, - Timestamp: time.Now().UTC(), - Files: parsed.FileCount(), - LinesAdded: parsed.AddedLines(), - LinesRemoved: parsed.DeletedLines(), - RulesLoaded: loaded.Source != rules.SourceDefault, + Provider: prov.Name(), + Model: model, + Lang: app.Lang.Code, + Usage: usage, + Cost: resolvePricing(app.Config, prov, model).Cost(usage), + Latency: latency, + Timestamp: time.Now().UTC(), + Files: parsed.FileCount(), + LinesAdded: parsed.AddedLines(), + LinesRemoved: parsed.DeletedLines(), + RulesLoaded: loaded.Source != rules.SourceDefault, + Retries: retries, + DegradeReason: degrade, } if !global.noCache && cacheStore != nil { @@ -580,19 +585,19 @@ func reviewOnePRDiff(ctx context.Context, runner remote.Runner, prID string, f r Lang: app.Lang.Code, } start := time.Now() - content, usage, format, err := tryStructuredReview(ctx, prov, req, func() {}) + outcome, err := tryStructuredReview(ctx, prov, req, func() {}) if err != nil { return prReviewResult{}, err } latency := time.Since(start) - if format != cache.FormatJSON { + if outcome.Format != cache.FormatJSON { return prReviewResult{}, errors.New(app.Catalog.T("remote.degraded")) } - findings, err := render.ParseFindings(content) + findings, err := render.ParseFindings(outcome.Content) if err != nil { return prReviewResult{}, errors.New(app.Catalog.T("remote.degraded")) } - return prReviewResult{findings: findings, anchors: anchors, usage: usage, latency: latency}, nil + return prReviewResult{findings: findings, anchors: anchors, usage: outcome.Usage, latency: latency}, nil } // submitPRReview posts the selected inline comments and the review-level diff --git a/internal/cli/review.go b/internal/cli/review.go index 8913657..ed78441 100644 --- a/internal/cli/review.go +++ b/internal/cli/review.go @@ -439,6 +439,8 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er content string usage provider.Usage format string + retries int + degrade string ) if plainText { // CLI-backed providers: single-shot call, no JSON parsing, no @@ -451,12 +453,12 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er } content, usage, format = resp.Content, resp.Usage, cache.FormatPlainText } else { - var callErr error - content, usage, format, callErr = tryStructuredReview(ctx, prov, req, func() { - // First attempt produced unparseable JSON; ADR-0014 §4 - // retry fires next. Mark the current "Thinking..." as - // Soft (neutral) and start a fresh "Retrying..." stage so - // the user sees we noticed the first attempt was iffy. + outcome, callErr := tryStructuredReview(ctx, prov, req, func() { + // First attempt produced unparseable JSON; the ADR-0031 + // repair-oriented retry fires next. Mark the current + // "Thinking..." as Soft (neutral) and start a fresh + // "Retrying..." stage so the user sees we noticed the first + // attempt was iffy. prog.Soft() prog.Start(app.Catalog.T("progress.retrying")) }) @@ -464,6 +466,8 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er prog.Fail(callErr) return fmt.Errorf("provider %s: %w", prov.Name(), callErr) } + content, usage, format = outcome.Content, outcome.Usage, outcome.Format + retries, degrade = outcome.Retries, outcome.DegradeReason } prog.Finish() // Cards / JSON / Markdown render takes over the screen below — @@ -497,19 +501,21 @@ func runReview(cmd *cobra.Command, scope reviewScopeFlags, diffArgs []string) er respModel := model meta := render.Meta{ - Provider: prov.Name(), - Model: respModel, - Lang: app.Lang.Code, - Usage: usage, - Cost: resolvePricing(app.Config, prov, respModel).Cost(usage), - Latency: latency, - Timestamp: time.Now().UTC(), - Files: parsed.FileCount(), - LinesAdded: parsed.AddedLines(), - LinesRemoved: parsed.DeletedLines(), - RulesLoaded: loaded.Source != rules.SourceDefault, - Baselined: baselined, - Suppressed: suppressed, + Provider: prov.Name(), + Model: respModel, + Lang: app.Lang.Code, + Usage: usage, + Cost: resolvePricing(app.Config, prov, respModel).Cost(usage), + Latency: latency, + Timestamp: time.Now().UTC(), + Files: parsed.FileCount(), + LinesAdded: parsed.AddedLines(), + LinesRemoved: parsed.DeletedLines(), + RulesLoaded: loaded.Source != rules.SourceDefault, + Baselined: baselined, + Suppressed: suppressed, + Retries: retries, + DegradeReason: degrade, } if !global.noCache && cacheStore != nil { @@ -805,53 +811,138 @@ func handleCopyFlag(cmd *cobra.Command, app *appContext, findings []render.Findi hint("clipboard.copied", len(findings), label) } -// tryStructuredReview runs Review and, on parse failure, retries once. -// Returns (content, totalUsage, format, err). format is FormatJSON when -// either the first or retry response parses cleanly; FormatMarkdownFallback -// when both attempts fail (the caller emits the user warning and stores -// the marker in cache so replays stay silent). +// structuredOutcome carries the result of a structured review plus the +// observability signals (ADR-0031): how many repair retries were issued and, +// on a graceful degrade, why. Retries is 0 on a first-attempt success and 1 +// when a repair retry was made (whether it recovered or degraded). +// DegradeReason is empty unless Format == cache.FormatMarkdownFallback. +type structuredOutcome struct { + Content string + Usage provider.Usage + Format string + Retries int + DegradeReason string +} + +// repairMaxTokens is the raised output ceiling used on the truncation-repair +// branch: a truncated first response is often max_tokens exhaustion (providers +// default to 4096 when req.MaxTokens is 0), so the "complete the JSON" retry +// gets more room. Not a cache-key input. +const repairMaxTokens = 8192 + +// tryStructuredReview runs Review and, on parse failure, retries once with a +// repair-oriented request (ADR-0031) instead of the byte-identical prompt: the +// retry classifies the parse failure and appends a failure-mode-specific +// recovery directive (and, for truncation, embeds the partial output and +// raises the token ceiling). Format is FormatJSON when either the first or the +// repair response parses cleanly; FormatMarkdownFallback when both attempts +// fail (the caller emits the user warning and stores the marker in cache so +// replays stay silent). // // Token usage is summed across both attempts so the verbose footer / cost // reflects what the user actually spent, even on a graceful degrade. // -// onRetry, if non-nil, fires after the first attempt parses-fails but -// before the retry call goes out. The progress UI uses it to flip the -// "Thinking..." stage to a soft (neutral) state and start a fresh -// "Retrying..." stage so the user sees what happened. +// onRetry, if non-nil, fires after the first attempt parse-fails but before +// the repair call goes out. The progress UI uses it to flip the "Thinking..." +// stage to a soft (neutral) state and start a fresh "Retrying..." stage so the +// user sees what happened. func tryStructuredReview( ctx context.Context, prov provider.Provider, req provider.Request, onRetry func(), -) (string, provider.Usage, string, error) { +) (structuredOutcome, error) { resp, err := prov.Review(ctx, req) if err != nil { - return "", provider.Usage{}, "", err + return structuredOutcome{}, err } - if _, parseErr := render.ParseFindings(resp.Content); parseErr == nil { - return resp.Content, resp.Usage, cache.FormatJSON, nil + _, parseErr := render.ParseFindings(resp.Content) + if parseErr == nil { + return structuredOutcome{Content: resp.Content, Usage: resp.Usage, Format: cache.FormatJSON}, nil } - // First attempt unparseable — ADR-0014 §4 retry-once. + // First attempt unparseable — ADR-0031 repair-oriented retry-once. if onRetry != nil { onRetry() } - resp2, err2 := prov.Review(ctx, req) + repairReq := repairRequest(req, classifyParseError(parseErr), resp.Content) + resp2, err2 := prov.Review(ctx, repairReq) if err2 != nil { - // Network/auth failure on retry: surface the first response with - // the fallback marker; the caller can still render via degrade. - return resp.Content, resp.Usage, cache.FormatMarkdownFallback, nil + // Network/auth failure on retry: surface the first response with the + // fallback marker; the caller can still render via degrade. + return structuredOutcome{ + Content: resp.Content, + Usage: resp.Usage, + Format: cache.FormatMarkdownFallback, + Retries: 1, + DegradeReason: "retry-error", + }, nil } totalUsage := provider.Usage{ InputTokens: resp.Usage.InputTokens + resp2.Usage.InputTokens, OutputTokens: resp.Usage.OutputTokens + resp2.Usage.OutputTokens, CachedInputTokens: resp.Usage.CachedInputTokens + resp2.Usage.CachedInputTokens, } - if _, parseErr := render.ParseFindings(resp2.Content); parseErr == nil { - return resp2.Content, totalUsage, cache.FormatJSON, nil + if _, parseErr2 := render.ParseFindings(resp2.Content); parseErr2 == nil { + return structuredOutcome{Content: resp2.Content, Usage: totalUsage, Format: cache.FormatJSON, Retries: 1}, nil + } else { + // Both attempts produced unparseable output — degrade with the first + // response cached as the canonical fallback content. + return structuredOutcome{ + Content: resp.Content, + Usage: totalUsage, + Format: cache.FormatMarkdownFallback, + Retries: 1, + DegradeReason: degradeReason(parseErr2), + }, nil + } +} + +// repairRequest builds the ADR-0031 repair retry: a fresh single-shot request +// (the Provider interface is single-shot and semver-locked) that appends a +// failure-mode-specific recovery directive to the system prompt instead of +// resending the identical prompt. For truncated/malformed JSON it also embeds +// the partial output in the user prompt and raises the output ceiling, since +// truncation is often max_tokens exhaustion. +func repairRequest(req provider.Request, kind render.ParseErrorKind, prevOutput string) provider.Request { + r := req + switch kind { + case render.ParseErrMalformedJSON: + r.SystemPrompt = req.SystemPrompt + "\n\n" + rules.RepairJSONComplete + r.UserPrompt = req.UserPrompt + "\n\n" + fmt.Sprintf(rules.RepairPrevOutputTemplate, prevOutput) + if r.MaxTokens <= 0 { + r.MaxTokens = repairMaxTokens + } + default: // ParseErrEmpty, ParseErrProse, ParseErrSchema → schema-ignored + r.SystemPrompt = req.SystemPrompt + "\n\n" + rules.RepairSchemaReset + } + return r +} + +// classifyParseError extracts the ParseErrorKind from a ParseFindings error so +// the repair prompt can branch by failure mode; a non-ParseError falls back to +// the schema-reset branch. +func classifyParseError(err error) render.ParseErrorKind { + var pe *render.ParseError + if errors.As(err, &pe) { + return pe.Kind + } + return render.ParseErrSchema +} + +// degradeReason maps a final (post-retry) parse failure to a stable snake-case +// reason surfaced as meta.degrade_reason (ADR-0031), kept machine-stable for +// `make eval` model comparison. +func degradeReason(err error) string { + switch classifyParseError(err) { + case render.ParseErrEmpty: + return "empty" + case render.ParseErrProse: + return "prose" + case render.ParseErrMalformedJSON: + return "malformed-json" + default: + return "schema" } - // Both attempts produced unparseable output — degrade with first - // response cached as the canonical fallback content. - return resp.Content, totalUsage, cache.FormatMarkdownFallback, nil } func fetchDiff(repo *git.DispatchRepo, scope reviewScopeFlags, diffArgs []string) (git.Diff, error) { diff --git a/internal/cli/structured_test.go b/internal/cli/structured_test.go index f0136a6..4e2b2a3 100644 --- a/internal/cli/structured_test.go +++ b/internal/cli/structured_test.go @@ -11,55 +11,108 @@ import ( "github.com/CommitBrief/commitbrief/internal/cache" "github.com/CommitBrief/commitbrief/internal/provider" "github.com/CommitBrief/commitbrief/internal/provider/mock" + "github.com/CommitBrief/commitbrief/internal/render" + "github.com/CommitBrief/commitbrief/internal/rules" ) +// TestCacheReplayFencedContentParses mirrors the cache-hit replay decision in +// runReview (review.go: `case cache.FormatJSON, "": ParseFindings(content)`). +// With Phase-0 salvage in ParseFindings (ADR-0031), a FormatJSON entry whose +// body was cached with a code fence renders as structured findings on replay, +// with no provider round-trip. +func TestCacheReplayFencedContentParses(t *testing.T) { + fenced := "```json\n" + mock.DefaultResponseContent + "\n```" + entry := cache.Entry{Result: cache.Result{Format: cache.FormatJSON, Content: fenced}} + + var findings []render.Finding + switch entry.Result.Format { + case cache.FormatJSON, "": + findings, _ = render.ParseFindings(entry.Result.Content) + } + if len(findings) == 0 { + t.Fatalf("cached fenced content should replay as structured findings; got %d", len(findings)) + } +} + func TestTryStructuredReviewHappyPath(t *testing.T) { m := mock.New() // Default response is already valid Findings JSON (DefaultResponseContent). - content, usage, format, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) + out, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } - if format != cache.FormatJSON { - t.Errorf("format = %q, want %q", format, cache.FormatJSON) + if out.Format != cache.FormatJSON { + t.Errorf("format = %q, want %q", out.Format, cache.FormatJSON) } - if !strings.Contains(content, `"findings"`) { - t.Errorf("content missing findings wrapper: %q", content) + if !strings.Contains(out.Content, `"findings"`) { + t.Errorf("content missing findings wrapper: %q", out.Content) } if m.ReviewCalls != 1 { t.Errorf("ReviewCalls = %d, want 1 (no retry on first-attempt success)", m.ReviewCalls) } - if usage.InputTokens == 0 { + if out.Retries != 0 { + t.Errorf("Retries = %d, want 0 on first-attempt success", out.Retries) + } + if out.DegradeReason != "" { + t.Errorf("DegradeReason = %q, want empty on success", out.DegradeReason) + } + if out.Usage.InputTokens == 0 { t.Error("usage should be populated") } } +func TestTryStructuredReviewFencedValidFirstAttempt(t *testing.T) { + // Phase-0 salvage (ADR-0031): valid findings JSON wrapped in a ```json + // fence must parse on the FIRST attempt — zero retries, no degrade. + m := mock.New() + m.ResponseContent = "```json\n" + mock.DefaultResponseContent + "\n```" + + out, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if m.ReviewCalls != 1 { + t.Errorf("ReviewCalls = %d, want 1 (fence salvage, no retry)", m.ReviewCalls) + } + if out.Format != cache.FormatJSON { + t.Errorf("format = %q, want %q", out.Format, cache.FormatJSON) + } + if out.Retries != 0 { + t.Errorf("Retries = %d, want 0", out.Retries) + } +} + func TestTryStructuredReviewRetriesOnce(t *testing.T) { - // First call returns invalid JSON; retry will see the same canned - // response (mock is stateless) — so both calls fail and we mark - // markdown-fallback. + // First call returns prose; retry sees the same canned response (mock is + // stateless) — both attempts fail and we mark markdown-fallback. m := mock.New() m.ResponseContent = "not actually JSON" - content, usage, format, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) + out, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } if m.ReviewCalls != 2 { t.Errorf("ReviewCalls = %d, want 2 (retry-once on parse failure)", m.ReviewCalls) } - if format != cache.FormatMarkdownFallback { - t.Errorf("format = %q, want %q", format, cache.FormatMarkdownFallback) + if out.Format != cache.FormatMarkdownFallback { + t.Errorf("format = %q, want %q", out.Format, cache.FormatMarkdownFallback) + } + if out.Content != "not actually JSON" { + t.Errorf("content = %q, want first-response text preserved", out.Content) } - if content != "not actually JSON" { - t.Errorf("content = %q, want first-response text preserved", content) + if out.Retries != 1 { + t.Errorf("Retries = %d, want 1", out.Retries) + } + if out.DegradeReason != "prose" { + t.Errorf("DegradeReason = %q, want %q (non-JSON commentary)", out.DegradeReason, "prose") } // Token usage should be summed across both attempts. wantInput := m.InputTokens * 2 wantOutput := m.OutputTokens * 2 - if usage.InputTokens != wantInput || usage.OutputTokens != wantOutput { + if out.Usage.InputTokens != wantInput || out.Usage.OutputTokens != wantOutput { t.Errorf("usage = %+v, want input=%d output=%d (summed across 2 calls)", - usage, wantInput, wantOutput) + out.Usage, wantInput, wantOutput) } } @@ -71,22 +124,87 @@ func TestTryStructuredReviewRetryRecovers(t *testing.T) { usage: provider.Usage{InputTokens: 50, OutputTokens: 10}, } - content, usage, format, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) + out, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } if m.calls != 2 { t.Errorf("calls = %d, want 2", m.calls) } - if format != cache.FormatJSON { - t.Errorf("format = %q, want %q on recovery", format, cache.FormatJSON) + if out.Format != cache.FormatJSON { + t.Errorf("format = %q, want %q on recovery", out.Format, cache.FormatJSON) + } + if out.Content != validJSON { + t.Errorf("content should be retry response; got %q", out.Content) + } + if out.Retries != 1 { + t.Errorf("Retries = %d, want 1 (recovered after one repair retry)", out.Retries) } - if content != validJSON { - t.Errorf("content should be retry response; got %q", content) + if out.DegradeReason != "" { + t.Errorf("DegradeReason = %q, want empty on recovery", out.DegradeReason) } // Tokens accumulate across both calls. - if usage.InputTokens != 100 || usage.OutputTokens != 20 { - t.Errorf("usage = %+v, want input=100 output=20 (summed)", usage) + if out.Usage.InputTokens != 100 || out.Usage.OutputTokens != 20 { + t.Errorf("usage = %+v, want input=100 output=20 (summed)", out.Usage) + } +} + +func TestTryStructuredReviewRepairProseReset(t *testing.T) { + // Prose first attempt → the repair retry must carry the hard schema-reset + // directive, NOT the "complete the JSON" one. + validJSON := `{"findings":[]}` + m := &switchingMock{ + responses: []string{"Sure! Here are the findings I noticed:", validJSON}, + usage: provider.Usage{InputTokens: 10, OutputTokens: 5}, + } + + out, err := tryStructuredReview(context.Background(), m, provider.Request{SystemPrompt: "BASE"}, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if out.Format != cache.FormatJSON { + t.Errorf("format = %q, want recovery to %q", out.Format, cache.FormatJSON) + } + if len(m.reqs) != 2 { + t.Fatalf("expected 2 requests captured, got %d", len(m.reqs)) + } + if !strings.Contains(m.reqs[1].SystemPrompt, rules.RepairSchemaReset) { + t.Errorf("repair request system prompt missing schema-reset directive:\n%q", m.reqs[1].SystemPrompt) + } + if strings.Contains(m.reqs[1].SystemPrompt, rules.RepairJSONComplete) { + t.Error("prose failure should NOT use the complete-the-JSON directive") + } +} + +func TestTryStructuredReviewRepairTruncatedJSON(t *testing.T) { + // A truncated JSON attempt → the repair retry must use the complete-the- + // JSON directive, embed the partial output, and raise the token ceiling. + truncated := `{"findings":[{"severity":"info","file":"a.go",` + validJSON := `{"findings":[]}` + m := &switchingMock{ + responses: []string{truncated, validJSON}, + usage: provider.Usage{InputTokens: 20, OutputTokens: 8}, + } + + out, err := tryStructuredReview(context.Background(), m, provider.Request{SystemPrompt: "BASE", UserPrompt: "DIFF"}, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if out.Format != cache.FormatJSON { + t.Errorf("format = %q, want recovery to %q", out.Format, cache.FormatJSON) + } + if len(m.reqs) != 2 { + t.Fatalf("expected 2 requests captured, got %d", len(m.reqs)) + } + repair := m.reqs[1] + if !strings.Contains(repair.SystemPrompt, rules.RepairJSONComplete) { + t.Errorf("repair request missing complete-the-JSON directive:\n%q", repair.SystemPrompt) + } + if !strings.Contains(repair.UserPrompt, truncated) { + t.Errorf("repair user prompt should embed the partial output; got:\n%q", repair.UserPrompt) + } + if repair.MaxTokens != repairMaxTokens { + t.Errorf("repair MaxTokens = %d, want %d (raised ceiling for truncation)", repair.MaxTokens, repairMaxTokens) } } @@ -96,7 +214,7 @@ func TestTryStructuredReviewBubblesFirstCallError(t *testing.T) { m := mock.New() m.ReviewErr = errors.New("provider down") - _, _, _, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) + _, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) if err == nil { t.Fatal("want error, got nil") } @@ -115,29 +233,37 @@ func TestTryStructuredReviewRetryNetworkErrorFallsBack(t *testing.T) { usage: provider.Usage{InputTokens: 30, OutputTokens: 5}, } - content, usage, format, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) + out, err := tryStructuredReview(context.Background(), m, provider.Request{}, nil) if err != nil { t.Fatalf("retry network error should not bubble; got %v", err) } - if format != cache.FormatMarkdownFallback { - t.Errorf("format = %q, want %q after retry network error", format, cache.FormatMarkdownFallback) + if out.Format != cache.FormatMarkdownFallback { + t.Errorf("format = %q, want %q after retry network error", out.Format, cache.FormatMarkdownFallback) + } + if out.Content != "first response" { + t.Errorf("content = %q, want first response preserved", out.Content) + } + if out.Retries != 1 { + t.Errorf("Retries = %d, want 1", out.Retries) } - if content != "first response" { - t.Errorf("content = %q, want first response preserved", content) + if out.DegradeReason != "retry-error" { + t.Errorf("DegradeReason = %q, want %q", out.DegradeReason, "retry-error") } // Only the first call's usage counts when retry network-failed. - if usage.InputTokens != 30 { - t.Errorf("usage.InputTokens = %d, want 30 (only first call counted)", usage.InputTokens) + if out.Usage.InputTokens != 30 { + t.Errorf("usage.InputTokens = %d, want 30 (only first call counted)", out.Usage.InputTokens) } } -// switchingMock returns a different canned response each call. Used to -// simulate transient malformation that recovers on retry. +// switchingMock returns a different canned response each call and records the +// requests it received, so tests can both simulate transient malformation that +// recovers on retry and assert on the repair prompt the retry carried. type switchingMock struct { responses []string errs []error usage provider.Usage calls int + reqs []provider.Request } func (s *switchingMock) Name() string { return "switching-mock" } @@ -147,7 +273,8 @@ func (s *switchingMock) EstimateTokens(t string) int { return (len(t) + func (s *switchingMock) Pricing(string) provider.Pricing { return provider.Pricing{} } func (s *switchingMock) TestConnection(context.Context) error { return nil } -func (s *switchingMock) Review(_ context.Context, _ provider.Request) (provider.Response, error) { +func (s *switchingMock) Review(_ context.Context, req provider.Request) (provider.Response, error) { + s.reqs = append(s.reqs, req) defer func() { s.calls++ }() idx := s.calls if idx >= len(s.responses) { diff --git a/internal/render/findings.go b/internal/render/findings.go index bd01523..018cac9 100644 --- a/internal/render/findings.go +++ b/internal/render/findings.go @@ -4,7 +4,6 @@ package render import ( "encoding/json" - "errors" "fmt" "io" "strings" @@ -96,34 +95,86 @@ type findingsEnvelope struct { Findings []Finding `json:"findings"` } +// ParseErrorKind classifies why ParseFindings rejected a payload so the +// retry/repair path (ADR-0031) can pick a failure-mode-specific recovery +// prompt instead of a blind re-roll of the identical request. +type ParseErrorKind int + +const ( + // ParseErrEmpty — the response was empty (or whitespace only) after + // trimming; there was no JSON at all. Recovered with a hard "JSON only" + // reset, same as prose / schema-ignored output. + ParseErrEmpty ParseErrorKind = iota + // ParseErrProse — json.Unmarshal failed and the payload does not even look + // like a JSON attempt (it starts with commentary, not `{`/`[`): the model + // ignored the schema and wrote prose. Recovered with a hard "JSON only, + // no prose" reset — asking it to "complete the JSON" would be nonsense. + ParseErrProse + // ParseErrMalformedJSON — json.Unmarshal failed but the payload looks like + // a genuine JSON attempt that is truncated or syntactically broken (often + // max_tokens exhaustion). Recovered by asking the model to complete/correct + // the JSON, with a raised token ceiling. + ParseErrMalformedJSON + // ParseErrSchema — the payload decoded as JSON but violated the findings + // contract (unknown severity or a missing required field). Recovered with + // a clean schema-conformant redraw. + ParseErrSchema +) + +// ParseError is the typed error ParseFindings returns. Its Error() text is +// kept byte-identical to the pre-ADR-0031 messages so existing substring +// assertions keep working; Kind adds machine-readable classification and +// Unwrap exposes the underlying json error for further inspection. +type ParseError struct { + Kind ParseErrorKind + msg string + err error +} + +func (e *ParseError) Error() string { return e.msg } +func (e *ParseError) Unwrap() error { return e.err } + // ParseFindings decodes the LLM-emitted JSON payload into a slice. The // returned slice may be empty (a clean review) but is non-nil on success. -// Errors trigger graceful degrade at the caller (ADR-0014 §4) — the -// pipeline never crashes on a malformed response. +// Errors are *ParseError (classifiable via errors.As) and trigger the +// repair-oriented retry, then graceful degrade, at the caller (ADR-0014 §4, +// ADR-0031) — the pipeline never crashes on a malformed response. func ParseFindings(content string) ([]Finding, error) { trimmed := strings.TrimSpace(content) if trimmed == "" { - return nil, errors.New("parse findings: empty content") + return nil, &ParseError{Kind: ParseErrEmpty, msg: "parse findings: empty content"} } + // Phase-0 salvage (ADR-0031): unwrap a lone ```json … ``` fence pair so a + // provider that fenced otherwise-valid findings JSON parses cleanly with + // no retry. Non-fenced input is returned unchanged (byte-identical path). + trimmed = stripCodeFence(trimmed) var env findingsEnvelope if err := json.Unmarshal([]byte(trimmed), &env); err != nil { - return nil, fmt.Errorf("parse findings: %w", err) + // Distinguish a broken JSON *attempt* (starts with `{`/`[` → truncated + // or syntactically malformed) from prose the model wrote instead of + // JSON. The two need different repair prompts (ADR-0031): complete-the- + // JSON vs a hard schema reset. + kind := ParseErrMalformedJSON + if !looksLikeJSON(trimmed) { + kind = ParseErrProse + } + return nil, &ParseError{Kind: kind, msg: fmt.Sprintf("parse findings: %v", err), err: err} } for i, f := range env.Findings { if !f.Severity.IsValid() { - return nil, fmt.Errorf("parse findings: finding %d: unknown severity %q", i, f.Severity) + return nil, &ParseError{Kind: ParseErrSchema, msg: fmt.Sprintf("parse findings: finding %d: unknown severity %q", i, f.Severity)} } if f.File == "" { - return nil, fmt.Errorf("parse findings: finding %d: missing file", i) + return nil, &ParseError{Kind: ParseErrSchema, msg: fmt.Sprintf("parse findings: finding %d: missing file", i)} } if f.Title == "" { - return nil, fmt.Errorf("parse findings: finding %d: missing title", i) + return nil, &ParseError{Kind: ParseErrSchema, msg: fmt.Sprintf("parse findings: finding %d: missing title", i)} } if f.Description == "" { - return nil, fmt.Errorf("parse findings: finding %d: missing description", i) + return nil, &ParseError{Kind: ParseErrSchema, msg: fmt.Sprintf("parse findings: finding %d: missing description", i)} } if f.Suggestion == "" { - return nil, fmt.Errorf("parse findings: finding %d: missing suggestion", i) + return nil, &ParseError{Kind: ParseErrSchema, msg: fmt.Sprintf("parse findings: finding %d: missing suggestion", i)} } } if env.Findings == nil { @@ -132,6 +183,57 @@ func ParseFindings(content string) ([]Finding, error) { return env.Findings, nil } +// stripCodeFence unwraps a single markdown code-fence pair (```json … ``` or +// ``` … ```) when — and only when — the content is exactly one fenced block: +// a leading fence line and a trailing fence line with the payload between +// them. This is the deterministic Phase-0 salvage (ADR-0031) for providers +// that wrap otherwise-valid findings JSON in fences. It is intentionally +// conservative: it never hunts for a {…} substring inside surrounding prose +// (that would blur failure-mode classification and risk accepting truncated +// fragments). Non-fenced input is returned unchanged so the happy path — and +// therefore the cache key — stays byte-identical. +func stripCodeFence(s string) string { + if !strings.HasPrefix(s, "```") { + return s + } + nl := strings.IndexByte(s, '\n') + if nl < 0 { + return s // a single line starting with ``` is not a fence pair + } + if !isFenceOpener(strings.TrimSpace(s[:nl])) { + return s + } + body := strings.TrimRight(s[nl+1:], " \t\r\n") + lastNL := strings.LastIndexByte(body, '\n') + if strings.TrimSpace(body[lastNL+1:]) != "```" { + return s // no matching closing fence — leave untouched + } + return strings.TrimSpace(body[:lastNL+1]) +} + +// looksLikeJSON reports whether s begins with a JSON object/array opener, +// used to tell a truncated/broken JSON attempt apart from prose the model +// wrote instead. s is assumed already trimmed and fence-stripped. +func looksLikeJSON(s string) bool { + return strings.HasPrefix(s, "{") || strings.HasPrefix(s, "[") +} + +// isFenceOpener reports whether line is a bare ``` optionally followed by a +// simple language tag (letters/digits only, e.g. ```json). Anything else — +// including a prose line that merely happens to start with ``` — is rejected +// so it is not mistaken for a fence. +func isFenceOpener(line string) bool { + tag := strings.TrimPrefix(line, "```") + for i := 0; i < len(tag); i++ { + c := tag[i] + if c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' { + continue + } + return false + } + return true +} + // SeverityGroup is the value type produced by GroupBySeverity for template // consumption. Items preserve the order they appeared in the source slice. type SeverityGroup struct { diff --git a/internal/render/findings_test.go b/internal/render/findings_test.go index 014343a..b441535 100644 --- a/internal/render/findings_test.go +++ b/internal/render/findings_test.go @@ -3,6 +3,7 @@ package render import ( + "errors" "strings" "testing" ) @@ -116,6 +117,81 @@ func TestParseFindings_Errors(t *testing.T) { } } +func TestParseFindings_FenceSalvage(t *testing.T) { + // Phase-0 salvage (ADR-0031): valid findings JSON wrapped in a lone code + // fence pair parses cleanly, across common fence spellings and surrounding + // whitespace. + valid := `{"findings":[{"severity":"info","file":"a.go","line":1,"title":"t","description":"d","suggestion":"s"}]}` + ok := map[string]string{ + "json fence": "```json\n" + valid + "\n```", + "bare fence": "```\n" + valid + "\n```", + "uppercase fence": "```JSON\n" + valid + "\n```", + "padded fence": "\n\n```json\n" + valid + "\n```\n\n", + } + for name, in := range ok { + t.Run(name, func(t *testing.T) { + got, err := ParseFindings(in) + if err != nil { + t.Fatalf("fenced valid JSON should parse: %v\ninput=%q", err, in) + } + if len(got) != 1 { + t.Errorf("len(findings) = %d, want 1", len(got)) + } + }) + } + + // Fenced but the inner payload is truncated → still an error, and it is + // classified as a malformed JSON attempt (not prose), because the unwrapped + // content starts with `{`. + _, err := ParseFindings("```json\n{\"findings\":\n```") + if err == nil { + t.Fatal("fenced-but-broken inner should error") + } + var pe *ParseError + if errors.As(err, &pe) && pe.Kind != ParseErrMalformedJSON { + t.Errorf("fenced broken inner Kind = %d, want ParseErrMalformedJSON (%d)", pe.Kind, ParseErrMalformedJSON) + } + + // Unfenced valid JSON is byte-identical behavior — still parses. + if _, err := ParseFindings(valid); err != nil { + t.Errorf("unfenced valid JSON should parse: %v", err) + } + + // A prose line that merely mentions ``` mid-text is not a fence pair and is + // left untouched (→ prose classification, below). + if _, err := ParseFindings("see the ```json block above"); err == nil { + t.Error("non-fenced prose should not be salvaged into a parse") + } +} + +func TestParseFindings_ErrorKinds(t *testing.T) { + cases := []struct { + name string + in string + want ParseErrorKind + }{ + {"empty", "", ParseErrEmpty}, + {"whitespace", " \n ", ParseErrEmpty}, + {"prose", "Here are the findings I noticed in your diff.", ParseErrProse}, + {"truncated json", `{"findings":[{"severity":"info","file":"a.go"`, ParseErrMalformedJSON}, + {"array attempt", `[{"severity":`, ParseErrMalformedJSON}, + {"unknown severity", `{"findings":[{"severity":"blocker","file":"a.go","line":1,"title":"t","description":"d","suggestion":"s"}]}`, ParseErrSchema}, + {"missing field", `{"findings":[{"severity":"high","file":"","line":1,"title":"t","description":"d","suggestion":"s"}]}`, ParseErrSchema}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := ParseFindings(tc.in) + var pe *ParseError + if !errors.As(err, &pe) { + t.Fatalf("error %v is not *ParseError", err) + } + if pe.Kind != tc.want { + t.Errorf("Kind = %d, want %d", pe.Kind, tc.want) + } + }) + } +} + func TestParseFindings_LineEndRoundTrip(t *testing.T) { // line_end is the schema-additive multi-line indicator (see // docs/json-schema.md). Parser must round-trip it as an integer diff --git a/internal/render/json.go b/internal/render/json.go index 8dae1ef..8b88d65 100644 --- a/internal/render/json.go +++ b/internal/render/json.go @@ -59,6 +59,15 @@ type jsonMeta struct { // never silent. Baselined int `json:"baselined,omitempty"` Suppressed int `json:"suppressed,omitempty"` + + // Recovery observability (ADR-0031). Additive optional fields, same + // omitempty discipline as baselined/suppressed: RetryCount is emitted only + // when a repair retry ran; DegradeReason only when the review degraded to + // markdown-fallback. A clean first-attempt review omits both, so schema + // stays 1 and the byte-for-byte v1 meta shape is unchanged. Both are + // live-call-only (a cache replay reports neither). + RetryCount int `json:"retry_count,omitempty"` + DegradeReason string `json:"degrade_reason,omitempty"` } type jsonUsage struct { @@ -87,15 +96,17 @@ func JSON(w io.Writer, p Payload) error { Content: content, Findings: findings, Meta: jsonMeta{ - Provider: p.Meta.Provider, - Model: p.Meta.Model, - Lang: p.Meta.Lang, - Cost: p.Meta.Cost, - LatencyMS: p.Meta.Latency.Milliseconds(), - Cached: p.Meta.Cached, - Timestamp: p.Meta.Timestamp, - Baselined: p.Meta.Baselined, - Suppressed: p.Meta.Suppressed, + Provider: p.Meta.Provider, + Model: p.Meta.Model, + Lang: p.Meta.Lang, + Cost: p.Meta.Cost, + LatencyMS: p.Meta.Latency.Milliseconds(), + Cached: p.Meta.Cached, + Timestamp: p.Meta.Timestamp, + Baselined: p.Meta.Baselined, + Suppressed: p.Meta.Suppressed, + RetryCount: p.Meta.Retries, + DegradeReason: p.Meta.DegradeReason, Usage: jsonUsage{ InputTokens: p.Meta.Usage.InputTokens, OutputTokens: p.Meta.Usage.OutputTokens, diff --git a/internal/render/render.go b/internal/render/render.go index 0413407..f03fa41 100644 --- a/internal/render/render.go +++ b/internal/render/render.go @@ -83,4 +83,13 @@ type Meta struct { // locked schema-v1 shape is unchanged when no signal control fired. Baselined int Suppressed int + + // Recovery observability (ADR-0031). Retries counts repair retries issued + // on this live review (0 or 1); DegradeReason is a stable snake-case tag + // ("malformed-json" / "schema" / "empty" / "retry-error") set only when the + // review degraded to markdown-fallback. Both are additive optional JSON + // meta fields (omitempty) plus verbose-footer lines; both are zero on a + // cache replay, which made no provider call. + Retries int + DegradeReason string } diff --git a/internal/render/render_test.go b/internal/render/render_test.go index 9ceac63..313b99a 100644 --- a/internal/render/render_test.go +++ b/internal/render/render_test.go @@ -215,6 +215,49 @@ func TestJSONSignalControlCountsPresentWhenSet(t *testing.T) { } } +func TestJSONRecoveryFieldsOmittedWhenZero(t *testing.T) { + var w bytes.Buffer + if err := JSON(&w, samplePayload()); err != nil { + t.Fatal(err) + } + var doc map[string]any + if err := json.Unmarshal(w.Bytes(), &doc); err != nil { + t.Fatal(err) + } + meta := doc["meta"].(map[string]any) + if _, ok := meta["retry_count"]; ok { + t.Error("retry_count must be omitted when zero (schema-v1 byte stability)") + } + if _, ok := meta["degrade_reason"]; ok { + t.Error("degrade_reason must be omitted when empty") + } +} + +func TestJSONRecoveryFieldsPresentWhenSet(t *testing.T) { + p := samplePayload() + p.Meta.Retries = 1 + p.Meta.DegradeReason = "malformed-json" + var w bytes.Buffer + if err := JSON(&w, p); err != nil { + t.Fatal(err) + } + var doc map[string]any + if err := json.Unmarshal(w.Bytes(), &doc); err != nil { + t.Fatal(err) + } + meta := doc["meta"].(map[string]any) + if meta["retry_count"] != float64(1) { + t.Errorf("retry_count = %v, want 1", meta["retry_count"]) + } + if meta["degrade_reason"] != "malformed-json" { + t.Errorf("degrade_reason = %v, want malformed-json", meta["degrade_reason"]) + } + // schema int must remain 1 — additive fields are not a version bump. + if doc["schema"] != float64(1) { + t.Errorf("schema = %v, want 1 (additive change must not bump)", doc["schema"]) + } +} + // TestJSONv1Golden is the drift guard for the v1 JSON schema. Any change to // JSON output bytes (field rename, type change, ordering, formatting) trips // this test. If the change is intentional and v1-compatible (additive only — @@ -340,6 +383,27 @@ func TestVerboseFooterOmitsEmptyFields(t *testing.T) { } } +func TestVerboseFooterRecoveryFields(t *testing.T) { + m := samplePayload().Meta + m.Retries = 1 + m.DegradeReason = "prose" + out := VerboseFooter(m) + if !strings.Contains(out, "Retries: 1") { + t.Errorf("footer missing Retries line; got:\n%s", out) + } + if !strings.Contains(out, "Degraded: prose") { + t.Errorf("footer missing Degraded line; got:\n%s", out) + } + // Omitted on a clean review (no retry, no degrade). + clean := VerboseFooter(samplePayload().Meta) + if strings.Contains(clean, "Retries:") { + t.Errorf("Retries line should be omitted when zero; got:\n%s", clean) + } + if strings.Contains(clean, "Degraded:") { + t.Errorf("Degraded line should be omitted when empty; got:\n%s", clean) + } +} + func TestExportedLineWrappers(t *testing.T) { // HeaderLine / StatusLine / FooterLine must produce the same content // the Cards renderer embeds, so non-card surfaces (remote pr) print diff --git a/internal/render/verbose.go b/internal/render/verbose.go index e38f0cb..3cc4205 100644 --- a/internal/render/verbose.go +++ b/internal/render/verbose.go @@ -45,6 +45,14 @@ func VerboseFooter(m Meta) string { if m.Latency > 0 { fmt.Fprintf(&sb, "Latency: %s\n", formatDuration(m.Latency)) } + // Recovery observability (ADR-0031): only shown when a repair retry ran or + // the review degraded, so a clean review's footer is unchanged. + if m.Retries > 0 { + fmt.Fprintf(&sb, "Retries: %d\n", m.Retries) + } + if m.DegradeReason != "" { + fmt.Fprintf(&sb, "Degraded: %s\n", m.DegradeReason) + } sb.WriteString(verboseRule) sb.WriteString("\n") return sb.String() diff --git a/internal/rules/prompt.go b/internal/rules/prompt.go index a02786b..b1d710b 100644 --- a/internal/rules/prompt.go +++ b/internal/rules/prompt.go @@ -88,6 +88,28 @@ Optional fields: Emit "findings": [] when the diff has no review-worthy issues.` +// Repair directives (ADR-0031). When a first structured attempt fails to +// parse, the retry appends one of these to the system prompt instead of +// resending the byte-identical request. They are English constants — like +// jsonContract — because they instruct the model *how* to shape its output; +// the separate language directive still governs the language findings are +// written in. Kept short so they don't dominate the (already large) system +// prompt on the second pass. + +// RepairSchemaReset is the recovery directive for prose / schema-ignored +// output (ParseErrEmpty, ParseErrSchema): a hard "JSON only" reset. +const RepairSchemaReset = `Your previous response did not parse as valid findings JSON. Output ONLY a single JSON object matching the schema above — no prose, no markdown code fences, no commentary before, after, or between objects. If there are no review-worthy issues, return {"findings": []}.` + +// RepairJSONComplete is the recovery directive for truncated / malformed JSON +// (ParseErrMalformedJSON): ask the model to finish and correct the document. +const RepairJSONComplete = `Your previous response was not valid JSON — it was truncated or malformed. Return the COMPLETE, corrected JSON object matching the schema above, in full. Output JSON only: no prose, no markdown code fences, no commentary.` + +// RepairPrevOutputTemplate embeds the previous (partial) output in the repair +// user prompt so the model can complete it. Single %s for the prior response. +// The Provider interface is single-shot (ADR-0014); "continue" must be a fresh +// request carrying the partial output, not a multi-turn continuation. +const RepairPrevOutputTemplate = "Your previous response, which failed to parse, was:\n```\n%s\n```\nReturn the complete corrected JSON object now." + // plainTextContract is the response-format block used by CLI-based // providers (claude-cli, gemini-cli, …) instead of jsonContract. The // agentic CLI tools don't expose native structured-output mechanisms