From 3e284a3e7aa3e9e12cf780a7f42f5fbfaa23142e Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sat, 27 Jun 2026 09:59:07 -0400 Subject: [PATCH 1/4] Reuse CR-settled thread summaries Closes #394 --- docs/architecture.md | 20 +- internal/cmd/respondcmd/respondcmd.go | 52 +++- internal/cmd/respondcmd/respondcmd_test.go | 31 ++- internal/pipeline/pipeline.go | 235 ++++++++++++++--- internal/pipeline/pipeline_test.go | 252 +++++++++++++++++++ internal/threadcontext/threadcontext.go | 92 +++++-- internal/threadcontext/threadcontext_test.go | 87 +++++++ 7 files changed, 699 insertions(+), 70 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 4275f47..4be54a8 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -82,18 +82,22 @@ data. The intended decomposition is: - `internal/threadcontext` normalizes `gitprovider.InlineThread`, detects codereview-authored finding threads, detects latest human replies, strips - shared markers, collapses resolved threads to the latest sanitized comment, - and produces file-scoped reviewer context. + shared markers, collapses provider-resolved threads to the latest sanitized + comment, recognizes CR-authored settled summary replies when provider + resolution is unavailable, and produces file-scoped reviewer context. - `internal/threadanalysis` accepts normalized thread context and returns reusable domain decisions: thread ID, decision, reply body, summary, resolve flag, and rationale. -Resolved inline threads should not be reprocessed as full conversations on -every review. Their durable context is the latest sanitized comment on the -resolved thread, with marker metadata retained when the comment contains a -codereview thread-summary marker. Reviewer prompts should receive compact -file-scoped summaries so agents avoid re-raising issues that have already been -discussed and resolved. +Settled inline threads should not be reprocessed as full conversations on every +review. Provider-resolved context is the latest sanitized comment on a thread +whose provider state is resolved. CR-settled context is the latest CR-authored +inline reply carrying a valid `codereview:thread-summary` marker when no newer +human reply follows it, even if the provider still reports the thread as +unresolved. Reviewer prompts should receive compact file-scoped summaries for +both sources so agents avoid re-raising issues that have already been discussed +and settled, while command output keeps provider resolution distinct from +CR-settled cache metadata. `cr review` and `cr respond` should share the same normalization, filtering, model resolution, LLM execution, and action-planning components. `cr respond` diff --git a/internal/cmd/respondcmd/respondcmd.go b/internal/cmd/respondcmd/respondcmd.go index e839cd8..2669b6a 100644 --- a/internal/cmd/respondcmd/respondcmd.go +++ b/internal/cmd/respondcmd/respondcmd.go @@ -153,10 +153,13 @@ type renderedResult struct { } type counts struct { - Considered int `json:"considered"` - Responded int `json:"responded"` - Resolved int `json:"resolved"` - Planned int `json:"planned"` + Considered int `json:"considered"` + Responded int `json:"responded"` + Resolved int `json:"resolved"` + ResolvePlanned int `json:"resolve_planned"` + ResolveFailed int `json:"resolve_failed"` + ProviderResolved int `json:"provider_resolved"` + Planned int `json:"planned"` } func newResult(result threadrespond.Result) renderedResult { @@ -169,11 +172,13 @@ func newResult(result threadrespond.Result) renderedResult { outboxExitCode = result.ExitCode } responded := countThreadReplies(result.Plan.Actions) - resolved := countThreadResolves(result.Plan.Actions) - if responded == 0 && resolved == 0 && len(result.PlannedActions) > 0 { + resolvePlanned := countThreadResolves(result.Plan.Actions) + if responded == 0 && resolvePlanned == 0 && len(result.PlannedActions) > 0 { responded = countPlannedThreadReplies(result.PlannedActions) - resolved = countPlannedThreadResolves(result.PlannedActions) + resolvePlanned = countPlannedThreadResolves(result.PlannedActions) } + providerResolved := countPostedThreadResolves(result.PlannedActions) + resolveFailed := countFailedThreadResolves(result.PlannedActions) return renderedResult{ Run: view.ReviewRun{ RunID: result.Run.RunID, @@ -186,10 +191,13 @@ func newResult(result threadrespond.Result) renderedResult { HeadSHA: result.Run.SHA, }, Counts: counts{ - Considered: len(result.EligibleThreads), - Responded: responded, - Resolved: resolved, - Planned: len(result.PlannedActions), + Considered: len(result.EligibleThreads), + Responded: responded, + Resolved: providerResolved, + ResolvePlanned: resolvePlanned, + ResolveFailed: resolveFailed, + ProviderResolved: providerResolved, + Planned: len(result.PlannedActions), }, Outbox: view.ReviewOutbox{ Outcome: outcome, @@ -221,7 +229,7 @@ func renderText(w io.Writer, rendered renderedResult) error { if _, err := fmt.Fprintf(w, "Outcome: %s\n", rendered.Run.Outcome); err != nil { return err } - if _, err := fmt.Fprintf(w, "Threads: considered %d, responded %d, resolved %d\n", rendered.Counts.Considered, rendered.Counts.Responded, rendered.Counts.Resolved); err != nil { + if _, err := fmt.Fprintf(w, "Threads: considered %d, responded %d, provider resolved %d (resolve planned %d, failed %d)\n", rendered.Counts.Considered, rendered.Counts.Responded, rendered.Counts.ProviderResolved, rendered.Counts.ResolvePlanned, rendered.Counts.ResolveFailed); err != nil { return err } if _, err := fmt.Fprintf(w, "Planned actions: %d\n", rendered.Counts.Planned); err != nil { @@ -283,6 +291,26 @@ func countPlannedThreadResolves(actions []ledger.PlannedAction) int { return count } +func countPostedThreadResolves(actions []ledger.PlannedAction) int { + var count int + for _, action := range actions { + if action.Kind == ledger.PlannedActionResolveThread && action.Status == ledger.PlannedActionPosted { + count++ + } + } + return count +} + +func countFailedThreadResolves(actions []ledger.PlannedAction) int { + var count int + for _, action := range actions { + if action.Kind == ledger.PlannedActionResolveThread && action.Status == ledger.PlannedActionFailedTerminal { + count++ + } + } + return count +} + func resultError(result threadrespond.Result) error { if strings.TrimSpace(result.Message) != "" { return errors.New(result.Message) diff --git a/internal/cmd/respondcmd/respondcmd_test.go b/internal/cmd/respondcmd/respondcmd_test.go index 7218c64..383edd7 100644 --- a/internal/cmd/respondcmd/respondcmd_test.go +++ b/internal/cmd/respondcmd/respondcmd_test.go @@ -55,7 +55,7 @@ func TestRespondDryRunCallsResponderAndRendersText(t *testing.T) { t.Fatal("runtime cleanup was not called") } text := out.String() - if !strings.Contains(text, "Threads: considered 1, responded 1, resolved 1") || !strings.Contains(text, "Planned actions: 2") { + if !strings.Contains(text, "Threads: considered 1, responded 1, provider resolved 0 (resolve planned 1, failed 0)") || !strings.Contains(text, "Planned actions: 2") { t.Fatalf("stdout = %q, want respond summary", text) } } @@ -82,7 +82,7 @@ func TestRespondRetryPostsFlagCallsResponder(t *testing.T) { if responder.requests[0].DryRun { t.Fatalf("respond retry request = %#v, want live retry", responder.requests[0]) } - if text := out.String(); !strings.Contains(text, "responded 1, resolved 1") || !strings.Contains(text, "Planned actions: 2") { + if text := out.String(); !strings.Contains(text, "responded 1, provider resolved 0 (resolve planned 1, failed 0)") || !strings.Contains(text, "Planned actions: 2") { t.Fatalf("stdout = %q, want retry counts from planned actions", text) } } @@ -128,7 +128,10 @@ func TestRespondJSONRendersStableShape(t *testing.T) { decoded.Run.HeadSHA != "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" || decoded.Counts.Considered != 1 || decoded.Counts.Responded != 1 || - decoded.Counts.Resolved != 1 || + decoded.Counts.Resolved != 0 || + decoded.Counts.ProviderResolved != 0 || + decoded.Counts.ResolvePlanned != 1 || + decoded.Counts.ResolveFailed != 0 || decoded.Counts.Planned != 2 || decoded.Outbox.Outcome != "comment" || decoded.Outbox.ExitCode != 0 || @@ -140,6 +143,28 @@ func TestRespondJSONRendersStableShape(t *testing.T) { } } +func TestRespondRendersFailedProviderResolveSeparately(t *testing.T) { + result := testThreadRespondResult(ledger.OutcomeComment) + result.PlannedActions[0].Status = ledger.PlannedActionPosted + result.PlannedActions[1].Status = ledger.PlannedActionFailedTerminal + result.Outbox = outbox.Result{Outcome: ledger.OutcomeComment, ExitCode: 1, Posted: 1, FailedTerminal: 1} + result.ExitCode = 1 + result.Message = "resolveReviewThread permission denied" + responder := &fakeResponder{result: result} + cmd, out := newTestCommand(t, testConfig(), fakeFactory(responder)) + + err := root.Execute(cmd, []string{"respond", "https://github.com/open-cli-collective/codereview-cli/pull/29"}) + if err == nil { + t.Fatal("Execute respond error = nil, want non-zero result error") + } + text := out.String() + if !strings.Contains(text, "provider resolved 0 (resolve planned 1, failed 1)") || + !strings.Contains(text, "Outbox: posted 1, pending 0, failed 1") || + !strings.Contains(text, "Message: resolveReviewThread permission denied") { + t.Fatalf("stdout = %q, want provider resolve failure called out", text) + } +} + func TestRespondRejectsRetryPostsWithDryRun(t *testing.T) { responder := &fakeResponder{result: testThreadRespondResult(ledger.OutcomeComment)} cmd, _ := newTestCommand(t, testConfig(), fakeFactory(responder)) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index bce7e9f..0964123 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -1024,6 +1024,7 @@ func prepareSelectionContext(ctx context.Context, opts Options, req selectionSet PinnedReview: reviewCtx.pinnedReview, ChangedFiles: parsed.Patches, Threads: threads, + ThreadContext: threadContext, Reviews: reviews, IssueComments: issueComments, Catalog: catalog, @@ -2612,9 +2613,14 @@ func reviewerPromptInputFromArtifacts(paths ArtifactPaths, pr gitprovider.PR, se func selectionThreadPrompts(threads []gitprovider.InlineThread, summary dossierDiscussionSummaryArtifact) []selectionThreadPrompt { summaryByAnchor := make(map[string]dossierInlineThreadSummaryArtifact, len(summary.InlineThreads)) + summaryByThreadID := make(map[string]dossierInlineThreadSummaryArtifact, len(summary.InlineThreads)) for _, thread := range summary.InlineThreads { - key := dossierInlineThreadAnchorKey(thread.Path, thread.Side, thread.Line, thread.AnchorKind) - summaryByAnchor[key] = thread + if strings.TrimSpace(thread.ThreadID) != "" { + summaryByThreadID[strings.TrimSpace(thread.ThreadID)] = thread + } else { + key := dossierInlineThreadAnchorKey(thread.Path, thread.Side, thread.Line, thread.AnchorKind) + summaryByAnchor[key] = thread + } } out := make([]selectionThreadPrompt, 0, len(threads)) for _, thread := range threads { @@ -2626,7 +2632,10 @@ func selectionThreadPrompts(threads []gitprovider.InlineThread, summary dossierD AnchorKind: string(thread.SubjectType), Resolved: thread.Resolved, } - if summarized, ok := summaryByAnchor[dossierInlineThreadAnchorKey(thread.Path, string(thread.Side), thread.Line, string(thread.SubjectType))]; ok { + if summarized, ok := summaryByThreadID[string(thread.ID)]; ok { + promptThread.Status = summarized.Status + promptThread.Summary = summarized.Summary + } else if summarized, ok := summaryByAnchor[dossierInlineThreadAnchorKey(thread.Path, string(thread.Side), thread.Line, string(thread.SubjectType))]; ok { promptThread.Status = summarized.Status promptThread.Summary = summarized.Summary } else if len(thread.Comments) > 0 { @@ -2639,9 +2648,14 @@ func selectionThreadPrompts(threads []gitprovider.InlineThread, summary dossierD func selectionThreadPromptsFromContext(threads []threadcontext.Thread, summary dossierDiscussionSummaryArtifact) []selectionThreadPrompt { summaryByAnchor := make(map[string]dossierInlineThreadSummaryArtifact, len(summary.InlineThreads)) + summaryByThreadID := make(map[string]dossierInlineThreadSummaryArtifact, len(summary.InlineThreads)) for _, thread := range summary.InlineThreads { - key := dossierInlineThreadAnchorKey(thread.Path, thread.Side, thread.Line, thread.AnchorKind) - summaryByAnchor[key] = thread + if strings.TrimSpace(thread.ThreadID) != "" { + summaryByThreadID[strings.TrimSpace(thread.ThreadID)] = thread + } else { + key := dossierInlineThreadAnchorKey(thread.Path, thread.Side, thread.Line, thread.AnchorKind) + summaryByAnchor[key] = thread + } } out := make([]selectionThreadPrompt, 0, len(threads)) for _, thread := range threads { @@ -2653,9 +2667,12 @@ func selectionThreadPromptsFromContext(threads []threadcontext.Thread, summary d AnchorKind: string(thread.Anchor.SubjectType), Resolved: thread.Resolved, } - if thread.ResolvedSummary != nil { + if settled, ok := thread.EffectiveSettledSummary(); ok { promptThread.Status = "settled" - promptThread.Summary = singleLineExcerpt(thread.ResolvedSummary.Body, dossierFinalExcerptRunes) + promptThread.Summary = singleLineExcerpt(settled.Body, dossierFinalExcerptRunes) + } else if summarized, ok := summaryByThreadID[string(thread.ID)]; ok { + promptThread.Status = summarized.Status + promptThread.Summary = summarized.Summary } else if summarized, ok := summaryByAnchor[dossierInlineThreadAnchorKey(thread.Anchor.Path, string(thread.Anchor.Side), thread.Anchor.Line, string(thread.Anchor.SubjectType))]; ok { promptThread.Status = summarized.Status promptThread.Summary = summarized.Summary @@ -2992,6 +3009,7 @@ type dossierInputs struct { PinnedReview bool ChangedFiles []FilePatch Threads []gitprovider.InlineThread + ThreadContext []threadcontext.Thread Reviews []gitprovider.Review IssueComments []gitprovider.IssueComment Catalog agents.Catalog @@ -3030,15 +3048,23 @@ type dossierThreadCommentArtifact struct { UpdatedAt time.Time `json:"updated_at,omitempty"` } +type dossierCachedThreadSummaryArtifact struct { + Source string `json:"source,omitempty"` + ThreadID string `json:"thread_id,omitempty"` + Body string `json:"body,omitempty"` + LastCommentID string `json:"last_comment_id,omitempty"` +} + type dossierInlineThreadArtifact struct { - ID string `json:"id"` - Path string `json:"path,omitempty"` - Side string `json:"side,omitempty"` - Line int `json:"line,omitempty"` - AnchorKind string `json:"anchor_kind,omitempty"` - Resolved bool `json:"resolved"` - CommitSHA string `json:"commit_sha,omitempty"` - Comments []dossierThreadCommentArtifact `json:"comments,omitempty"` + ID string `json:"id"` + Path string `json:"path,omitempty"` + Side string `json:"side,omitempty"` + Line int `json:"line,omitempty"` + AnchorKind string `json:"anchor_kind,omitempty"` + Resolved bool `json:"resolved"` + CommitSHA string `json:"commit_sha,omitempty"` + CachedSummary *dossierCachedThreadSummaryArtifact `json:"cached_summary,omitempty"` + Comments []dossierThreadCommentArtifact `json:"comments,omitempty"` } type dossierRepoContextArtifact struct { @@ -3088,6 +3114,7 @@ type dossierTopLevelCommentSummary struct { } type dossierInlineThreadSummaryArtifact struct { + ThreadID string `json:"thread_id,omitempty"` Path string `json:"path,omitempty"` Side string `json:"side,omitempty"` Line int `json:"line,omitempty"` @@ -3106,9 +3133,11 @@ type dossierDiscussionPromptInput struct { } type dossierDiscussionPromptData struct { - Input dossierDiscussionPromptInput - SourceFingerprint string - InlineThreadMap map[string]dossierInlineThreadPromptData + Input dossierDiscussionPromptInput + SourceFingerprint string + InlineThreadMap map[string]dossierInlineThreadPromptData + InlineThreadIDMap map[string]dossierInlineThreadPromptData + CachedInlineSummaries []dossierInlineThreadSummaryArtifact } type dossierInlineThreadPromptData struct { @@ -3123,6 +3152,7 @@ type dossierPromptTopLevelComment struct { } type dossierPromptInlineThread struct { + ThreadID string `json:"thread_id,omitempty"` Path string `json:"path,omitempty"` Side string `json:"side,omitempty"` Line int `json:"line,omitempty"` @@ -3218,6 +3248,8 @@ const ( dossierSummaryMaxInlineThreads = 20 dossierSummaryMaxThreadComments = 5 dossierSummaryExcerptRunes = 480 + dossierCachedSummaryProviderSource = "provider_resolved" + dossierCachedSummaryCRSource = "cr_settled" workbenchMetadataSchemaVersion = 1 workbenchCheckoutModeArtifactClone = "artifact-clone" ) @@ -3761,6 +3793,9 @@ func writeRawDossierArtifacts(paths ArtifactPaths, in dossierInputs) error { changedFiles := dossierChangedFiles(in.ChangedFiles) topLevelComments := dossierTopLevelComments(in.IssueComments, in.Reviews) inlineThreads := dossierInlineThreads(in.Threads) + if len(in.ThreadContext) > 0 { + inlineThreads = dossierInlineThreadsFromContext(in.ThreadContext) + } repoContext := dossierRepoContextArtifact{ RepoInfo: in.Catalog.Repo, Sources: append([]agents.SourceInfo(nil), in.Catalog.Sources...), @@ -3852,6 +3887,7 @@ func summarizeDiscussionArtifacts(ctx context.Context, opts Options, req dossier SourceFingerprint: promptData.SourceFingerprint, TopLevelOmitted: promptData.Input.TopLevelCommentsOmitted, InlineThreadsOmitted: promptData.Input.InlineThreadsOmitted, + InlineThreads: append([]dossierInlineThreadSummaryArtifact(nil), promptData.CachedInlineSummaries...), }, nil } runtimeConfig, err := resolveSelectionRuntimeConfig(req.Profile, req.SelectionModelOverride, req.SelectionEffortOverride) @@ -3897,6 +3933,7 @@ func summarizeDiscussionArtifacts(ctx context.Context, opts Options, req dossier } summary.TopLevelOmitted = promptData.Input.TopLevelCommentsOmitted summary.InlineThreadsOmitted = promptData.Input.InlineThreadsOmitted + summary.InlineThreads = mergeDossierInlineThreadSummaries(summary.InlineThreads, promptData.CachedInlineSummaries) return summary, nil } @@ -3992,7 +4029,7 @@ func buildDossierDiscussionSummaryPrompt(input dossierDiscussionPromptData) (str "Preserve file/line/side/anchor context for inline threads.", }, "top_level_comments_fields": []string{"kind", "author", "summary"}, - "inline_thread_fields": []string{"path", "line", "side", "anchor_kind", "resolved", "status", "summary"}, + "inline_thread_fields": []string{"thread_id", "path", "line", "side", "anchor_kind", "resolved", "status", "summary"}, "inline_thread_statuses": []string{"settled", "unresolved", "noted"}, }, "discussion": input.Input, @@ -4008,6 +4045,17 @@ func dossierDiscussionPromptInputFromDiscussion(discussion dossierDiscussionArti filteredTopLevel := reviewerFacingTopLevelComments(discussion.TopLevelComments) input := dossierDiscussionPromptInput{} inlineThreadMap := make(map[string]dossierInlineThreadPromptData, len(discussion.InlineThreads)) + inlineThreadIDMap := make(map[string]dossierInlineThreadPromptData, len(discussion.InlineThreads)) + cachedSummaries := make([]dossierInlineThreadSummaryArtifact, 0) + uncachedThreads := make([]dossierInlineThreadArtifact, 0, len(discussion.InlineThreads)) + for _, thread := range discussion.InlineThreads { + if cached, ok := cachedDossierInlineThreadSummary(thread); ok { + cachedSummaries = append(cachedSummaries, cached) + inlineThreadIDMap[thread.ID] = dossierInlineThreadPromptData{Thread: thread} + continue + } + uncachedThreads = append(uncachedThreads, thread) + } for _, comment := range capTopLevelCommentsForSummary(filteredTopLevel) { body := singleLineExcerpt(comment.Body, dossierSummaryExcerptRunes) if shouldExcludeDiscussionSummaryText(body) { @@ -4022,8 +4070,9 @@ func dossierDiscussionPromptInputFromDiscussion(discussion dossierDiscussionArti if len(filteredTopLevel) > dossierSummaryMaxTopLevel { input.TopLevelCommentsOmitted = len(filteredTopLevel) - dossierSummaryMaxTopLevel } - for _, thread := range capInlineThreadsForSummary(discussion.InlineThreads) { + for _, thread := range capInlineThreadsForSummary(uncachedThreads) { promptThread := dossierPromptInlineThread{ + ThreadID: thread.ID, Path: thread.Path, Side: thread.Side, Line: thread.Line, @@ -4044,13 +4093,19 @@ func dossierDiscussionPromptInputFromDiscussion(discussion dossierDiscussionArti promptThread.CommentsOmitted = len(thread.Comments) - dossierSummaryMaxThreadComments } input.InlineThreads = append(input.InlineThreads, promptThread) + if strings.TrimSpace(thread.ID) != "" { + inlineThreadIDMap[thread.ID] = dossierInlineThreadPromptData{ + Thread: thread, + CommentsOmitted: promptThread.CommentsOmitted, + } + } inlineThreadMap[dossierInlineThreadAnchorKey(thread.Path, thread.Side, thread.Line, thread.AnchorKind)] = dossierInlineThreadPromptData{ Thread: thread, CommentsOmitted: promptThread.CommentsOmitted, } } - if len(discussion.InlineThreads) > dossierSummaryMaxInlineThreads { - input.InlineThreadsOmitted = len(discussion.InlineThreads) - dossierSummaryMaxInlineThreads + if len(uncachedThreads) > dossierSummaryMaxInlineThreads { + input.InlineThreadsOmitted = len(uncachedThreads) - dossierSummaryMaxInlineThreads } // Intentionally fingerprint the full reviewer-facing discussion, not the // capped prompt projection, so omitted content changes still invalidate the @@ -4071,12 +4126,72 @@ func dossierDiscussionPromptInputFromDiscussion(discussion dossierDiscussionArti return dossierDiscussionPromptData{}, fmt.Errorf("pipeline: marshal dossier discussion source fingerprint: %w", err) } return dossierDiscussionPromptData{ - Input: input, - SourceFingerprint: sha256Hex(data), - InlineThreadMap: inlineThreadMap, + Input: input, + SourceFingerprint: sha256Hex(data), + InlineThreadMap: inlineThreadMap, + InlineThreadIDMap: inlineThreadIDMap, + CachedInlineSummaries: cachedSummaries, }, nil } +func cachedDossierInlineThreadSummary(thread dossierInlineThreadArtifact) (dossierInlineThreadSummaryArtifact, bool) { + if thread.CachedSummary == nil { + return dossierInlineThreadSummaryArtifact{}, false + } + body := strings.TrimSpace(thread.CachedSummary.Body) + if body == "" || shouldExcludeDiscussionSummaryText(body) { + return dossierInlineThreadSummaryArtifact{}, false + } + threadID := strings.TrimSpace(thread.CachedSummary.ThreadID) + if threadID == "" { + threadID = strings.TrimSpace(thread.ID) + } + return dossierInlineThreadSummaryArtifact{ + ThreadID: threadID, + Path: thread.Path, + Side: thread.Side, + Line: thread.Line, + AnchorKind: thread.AnchorKind, + Resolved: thread.Resolved, + Status: "settled", + Summary: body, + }, true +} + +func mergeDossierInlineThreadSummaries(generated, cached []dossierInlineThreadSummaryArtifact) []dossierInlineThreadSummaryArtifact { + if len(cached) == 0 { + return generated + } + out := append([]dossierInlineThreadSummaryArtifact(nil), generated...) + indexByThreadID := make(map[string]int, len(out)+len(cached)) + for i, summary := range out { + if strings.TrimSpace(summary.ThreadID) != "" { + indexByThreadID[strings.TrimSpace(summary.ThreadID)] = i + } + } + for _, summary := range cached { + threadID := strings.TrimSpace(summary.ThreadID) + if threadID != "" { + if i, ok := indexByThreadID[threadID]; ok { + out[i] = summary + continue + } + indexByThreadID[threadID] = len(out) + } + out = append(out, summary) + } + sort.SliceStable(out, func(i, j int) bool { + if out[i].Path == out[j].Path { + if out[i].Line == out[j].Line { + return out[i].ThreadID < out[j].ThreadID + } + return out[i].Line < out[j].Line + } + return out[i].Path < out[j].Path + }) + return out +} + func decodeDossierDiscussionSummary(data []byte, promptData dossierDiscussionPromptData) (dossierDiscussionSummaryArtifact, error) { var decoded dossierDiscussionSummaryArtifact if err := json.Unmarshal(data, &decoded); err != nil { @@ -4099,6 +4214,7 @@ func decodeDossierDiscussionSummary(data []byte, promptData dossierDiscussionPro } } for i := range decoded.InlineThreads { + decoded.InlineThreads[i].ThreadID = strings.TrimSpace(decoded.InlineThreads[i].ThreadID) decoded.InlineThreads[i].Path = strings.TrimSpace(decoded.InlineThreads[i].Path) decoded.InlineThreads[i].Side = strings.TrimSpace(decoded.InlineThreads[i].Side) decoded.InlineThreads[i].AnchorKind = strings.TrimSpace(decoded.InlineThreads[i].AnchorKind) @@ -4115,11 +4231,15 @@ func decodeDossierDiscussionSummary(data []byte, promptData dossierDiscussionPro default: return dossierDiscussionSummaryArtifact{}, fmt.Errorf("discussion summary inline_threads[%d].status = %q, want settled|unresolved|noted", i, decoded.InlineThreads[i].Status) } - anchorKey := dossierInlineThreadAnchorKey(decoded.InlineThreads[i].Path, decoded.InlineThreads[i].Side, decoded.InlineThreads[i].Line, decoded.InlineThreads[i].AnchorKind) - expected, ok := promptData.InlineThreadMap[anchorKey] + expected, ok := promptData.InlineThreadIDMap[decoded.InlineThreads[i].ThreadID] if !ok { - return dossierDiscussionSummaryArtifact{}, fmt.Errorf("discussion summary inline_threads[%d] anchor %q is not present in the source discussion", i, anchorKey) + anchorKey := dossierInlineThreadAnchorKey(decoded.InlineThreads[i].Path, decoded.InlineThreads[i].Side, decoded.InlineThreads[i].Line, decoded.InlineThreads[i].AnchorKind) + expected, ok = promptData.InlineThreadMap[anchorKey] + if !ok { + return dossierDiscussionSummaryArtifact{}, fmt.Errorf("discussion summary inline_threads[%d] anchor %q is not present in the source discussion", i, anchorKey) + } } + decoded.InlineThreads[i].ThreadID = expected.Thread.ID decoded.InlineThreads[i].Path = expected.Thread.Path decoded.InlineThreads[i].Side = expected.Thread.Side decoded.InlineThreads[i].Line = expected.Thread.Line @@ -4364,6 +4484,65 @@ func dossierInlineThreads(threads []gitprovider.InlineThread) []dossierInlineThr return out } +func dossierInlineThreadsFromContext(threads []threadcontext.Thread) []dossierInlineThreadArtifact { + out := make([]dossierInlineThreadArtifact, 0, len(threads)) + for _, thread := range threads { + comments := make([]dossierThreadCommentArtifact, 0, len(thread.Comments)) + for _, comment := range thread.Comments { + comments = append(comments, dossierThreadCommentArtifact{ + URL: comment.URL, + Author: comment.Author.Login, + Body: strings.TrimSpace(comment.Body), + CommitSHA: comment.Anchor.CommitSHA, + CreatedAt: comment.CreatedAt, + UpdatedAt: comment.UpdatedAt, + }) + } + sort.SliceStable(comments, func(i, j int) bool { + if comments[i].CreatedAt.Equal(comments[j].CreatedAt) { + if comments[i].URL == comments[j].URL { + return comments[i].Body < comments[j].Body + } + return comments[i].URL < comments[j].URL + } + return comments[i].CreatedAt.Before(comments[j].CreatedAt) + }) + artifact := dossierInlineThreadArtifact{ + ID: string(thread.ID), + Path: thread.Anchor.Path, + Side: string(thread.Anchor.Side), + Line: thread.Anchor.Line, + AnchorKind: string(thread.Anchor.SubjectType), + Resolved: thread.Resolved, + CommitSHA: thread.Anchor.CommitSHA, + Comments: comments, + } + if summary, ok := thread.EffectiveSettledSummary(); ok { + source := dossierCachedSummaryCRSource + if thread.ResolvedSummary != nil && summary == thread.ResolvedSummary { + source = dossierCachedSummaryProviderSource + } + artifact.CachedSummary = &dossierCachedThreadSummaryArtifact{ + Source: source, + ThreadID: string(summary.ThreadID), + Body: strings.TrimSpace(summary.Body), + LastCommentID: string(summary.LastCommentID), + } + } + out = append(out, artifact) + } + sort.SliceStable(out, func(i, j int) bool { + if out[i].Path == out[j].Path { + if out[i].Line == out[j].Line { + return out[i].ID < out[j].ID + } + return out[i].Line < out[j].Line + } + return out[i].Path < out[j].Path + }) + return out +} + func renderDossierPRIntent(pr dossierPRContextArtifact) string { var out strings.Builder out.WriteString("# PR Intent\n\n") diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 1c48caf..ec31dec 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -30,6 +30,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/runartifact" "github.com/open-cli-collective/codereview-cli/internal/stagemodel" "github.com/open-cli-collective/codereview-cli/internal/statepaths" + "github.com/open-cli-collective/codereview-cli/internal/threadcontext" ) func dryRunForTest(ctx context.Context, opts Options, req Request) (Result, error) { @@ -1573,6 +1574,230 @@ func TestPrepareDossierArtifactsSummaryPromptCarriesSplitDiscussionShape(t *test } } +func TestPrepareDossierArtifactsReusesCRSettledThreadSummaryWithoutLLM(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + layout := statepaths.NewLayout(t.TempDir(), t.TempDir()) + run := allocatePipelineRun(t, store, layout, "run-dossier-cr-settled", ledger.PostModeDryRun, fixedNow()) + provider, req := dryRunHarness(t) + artifacts := ArtifactPathsFromDir(run.ArtifactPath) + bot := req.PostingIdentity + human := gitprovider.Identity{Login: "human", ID: "human-id"} + threads := []gitprovider.InlineThread{ + crSettledReviewThread(t, "thread-1", "main.go", 2, bot, human, "Cached settled summary"), + } + threadContext, err := threadcontext.Normalize(threads, threadcontext.Options{PostingIdentity: bot}) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if err := writeRawDossierArtifacts(artifacts, dossierInputs{ + CurrentPR: provider.pr, + ReviewPR: provider.pr, + ChangedFiles: parseDiffPatchesForTest(t, provider.diff.Raw), + Threads: threads, + ThreadContext: threadContext, + Catalog: agents.Catalog{}, + CurrentBaseSHA: provider.pr.Base.SHA, + CurrentHeadSHA: provider.pr.Head.SHA, + }); err != nil { + t.Fatalf("writeRawDossierArtifacts: %v", err) + } + + adapter := &llm.FakeAdapter{NameValue: "fake-llm"} + if err := prepareDossierArtifacts(ctx, Options{ + Adapter: adapter, + Store: store, + Now: fixedNow, + NewSessionRowID: sequence("session"), + }, dossierPreparationRequest{ + RunID: run.RunID, + Profile: req.Profile, + Artifacts: artifacts, + }); err != nil { + t.Fatalf("prepareDossierArtifacts: %v", err) + } + if len(adapter.Requests()) != 0 { + t.Fatalf("adapter requests = %d, want cached dossier summary without LLM", len(adapter.Requests())) + } + var summary dossierDiscussionSummaryArtifact + if err := readJSONFile(filepath.Join(artifacts.DossierDir, "summary", "discussion.json"), &summary); err != nil { + t.Fatalf("read summary discussion: %v", err) + } + if len(summary.InlineThreads) != 1 { + t.Fatalf("summary inline threads = %#v, want one cached entry", summary.InlineThreads) + } + got := summary.InlineThreads[0] + if got.ThreadID != "thread-1" || got.Resolved || got.Status != "settled" || got.Summary != "Cached settled summary" { + t.Fatalf("cached summary = %#v, want thread-1 unresolved settled cached summary", got) + } + discussionPath := filepath.Join(artifacts.DossierDir, "final", "discussion.md") + assertFileContains(t, discussionPath, "main.go:2 [RIGHT] {line} Settled: Cached settled summary") + assertFileOmits(t, discussionPath, "Original finding") +} + +func TestPrepareDossierArtifactsKeepsSameAnchorCachedSummariesByThreadID(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + layout := statepaths.NewLayout(t.TempDir(), t.TempDir()) + run := allocatePipelineRun(t, store, layout, "run-dossier-cr-settled-same-anchor", ledger.PostModeDryRun, fixedNow()) + provider, req := dryRunHarness(t) + artifacts := ArtifactPathsFromDir(run.ArtifactPath) + bot := req.PostingIdentity + human := gitprovider.Identity{Login: "human", ID: "human-id"} + threads := []gitprovider.InlineThread{ + crSettledReviewThread(t, "thread-a", "main.go", 2, bot, human, "First cached summary"), + crSettledReviewThread(t, "thread-b", "main.go", 2, bot, human, "Second cached summary"), + } + threadContext, err := threadcontext.Normalize(threads, threadcontext.Options{PostingIdentity: bot}) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if err := writeRawDossierArtifacts(artifacts, dossierInputs{ + CurrentPR: provider.pr, + ReviewPR: provider.pr, + ChangedFiles: parseDiffPatchesForTest(t, provider.diff.Raw), + Threads: threads, + ThreadContext: threadContext, + Catalog: agents.Catalog{}, + CurrentBaseSHA: provider.pr.Base.SHA, + CurrentHeadSHA: provider.pr.Head.SHA, + }); err != nil { + t.Fatalf("writeRawDossierArtifacts: %v", err) + } + + if err := prepareDossierArtifacts(ctx, Options{ + Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}, + Store: store, + Now: fixedNow, + NewSessionRowID: sequence("session"), + }, dossierPreparationRequest{ + RunID: run.RunID, + Profile: req.Profile, + Artifacts: artifacts, + }); err != nil { + t.Fatalf("prepareDossierArtifacts: %v", err) + } + var summary dossierDiscussionSummaryArtifact + if err := readJSONFile(filepath.Join(artifacts.DossierDir, "summary", "discussion.json"), &summary); err != nil { + t.Fatalf("read summary discussion: %v", err) + } + if len(summary.InlineThreads) != 2 { + t.Fatalf("summary inline threads = %#v, want both same-anchor cached entries", summary.InlineThreads) + } + got := map[string]string{} + for _, thread := range summary.InlineThreads { + got[thread.ThreadID] = thread.Summary + } + want := map[string]string{"thread-a": "First cached summary", "thread-b": "Second cached summary"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("cached summaries = %#v, want %#v", got, want) + } +} + +func TestPrepareDossierArtifactsIncludesCachedSummaryBeyondInlineThreadCap(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + layout := statepaths.NewLayout(t.TempDir(), t.TempDir()) + run := allocatePipelineRun(t, store, layout, "run-dossier-cr-settled-beyond-cap", ledger.PostModeDryRun, fixedNow()) + provider, req := dryRunHarness(t) + artifacts := ArtifactPathsFromDir(run.ArtifactPath) + bot := req.PostingIdentity + human := gitprovider.Identity{Login: "human", ID: "human-id"} + threads := make([]gitprovider.InlineThread, 0, dossierSummaryMaxInlineThreads+1) + for i := 0; i < dossierSummaryMaxInlineThreads; i++ { + threads = append(threads, gitprovider.InlineThread{ + ID: gitprovider.ThreadID(fmt.Sprintf("thread-open-%02d", i)), + Path: "a.go", + Side: review.DiffSideRight, + Line: i + 1, + SubjectType: review.AnchorKindLine, + Comments: []gitprovider.ThreadComment{{ + Body: fmt.Sprintf("open thread %02d", i), + Author: human, + Path: "a.go", + Side: review.DiffSideRight, + Line: i + 1, + CreatedAt: fixedNow(), + UpdatedAt: fixedNow(), + }}, + }) + } + threads = append(threads, crSettledReviewThread(t, "thread-cached", "z.go", 99, bot, human, "Cached beyond cap")) + threadContext, err := threadcontext.Normalize(threads, threadcontext.Options{PostingIdentity: bot}) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if err := writeRawDossierArtifacts(artifacts, dossierInputs{ + CurrentPR: provider.pr, + ReviewPR: provider.pr, + ChangedFiles: parseDiffPatchesForTest(t, provider.diff.Raw), + Threads: threads, + ThreadContext: threadContext, + Catalog: agents.Catalog{}, + CurrentBaseSHA: provider.pr.Base.SHA, + CurrentHeadSHA: provider.pr.Head.SHA, + }); err != nil { + t.Fatalf("writeRawDossierArtifacts: %v", err) + } + + adapter := &llm.FakeAdapter{NameValue: "fake-llm"} + adapter.Queue(fakeLLMResult("dossier-summary-session", discussionSummaryJSON(nil, []threadSummary{{ + path: "a.go", + line: 1, + status: "unresolved", + summary: "Open thread summary", + resolved: false, + }}), 8, 2)) + if err := prepareDossierArtifacts(ctx, Options{ + Adapter: adapter, + Store: store, + Now: fixedNow, + NewSessionRowID: sequence("session"), + }, dossierPreparationRequest{ + RunID: run.RunID, + Profile: req.Profile, + Artifacts: artifacts, + }); err != nil { + t.Fatalf("prepareDossierArtifacts: %v", err) + } + var summary dossierDiscussionSummaryArtifact + if err := readJSONFile(filepath.Join(artifacts.DossierDir, "summary", "discussion.json"), &summary); err != nil { + t.Fatalf("read summary discussion: %v", err) + } + var found bool + for _, thread := range summary.InlineThreads { + if thread.ThreadID == "thread-cached" && thread.Summary == "Cached beyond cap" && thread.Status == "settled" { + found = true + } + } + if !found { + t.Fatalf("summary inline threads = %#v, want cached thread beyond cap", summary.InlineThreads) + } +} + +func TestSelectionThreadPromptsFromContextUsesCRSettledSummary(t *testing.T) { + bot := gitprovider.Identity{Login: "review-bot", ID: "bot-id"} + human := gitprovider.Identity{Login: "human", ID: "human-id"} + threads, err := threadcontext.Normalize([]gitprovider.InlineThread{ + crSettledReviewThread(t, "thread-1", "main.go", 2, bot, human, "Cached settled summary"), + }, threadcontext.Options{PostingIdentity: bot}) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + + prompts := selectionThreadPromptsFromContext(threads, dossierDiscussionSummaryArtifact{}) + if len(prompts) != 1 { + t.Fatalf("selection thread prompts = %#v, want one prompt", prompts) + } + got := prompts[0] + if got.ThreadID != "thread-1" || got.Resolved || got.Status != "settled" || got.Summary != "Cached settled summary" { + t.Fatalf("selection thread prompt = %#v, want unresolved settled cached summary", got) + } +} + func TestPrepareDossierArtifactsSummaryPromptBudgetFailure(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) @@ -6294,6 +6519,33 @@ func markedReviewThread(t *testing.T, id, path string, line int, bot, human gitp } } +func crSettledReviewThread(t *testing.T, id, path string, line int, bot, human gitprovider.Identity, summary string) gitprovider.InlineThread { + t.Helper() + thread := markedReviewThread(t, id, path, line, bot, human) + summaryMarker, err := marker.RenderThreadSummary(marker.ThreadSummaryMarker{ + RunID: "response-run", + ActionID: "summary-" + id, + }) + if err != nil { + t.Fatalf("RenderThreadSummary: %v", err) + } + created := fixedNow().Add(2 * time.Minute) + thread.Comments = append(thread.Comments, gitprovider.ThreadComment{ + ID: gitprovider.CommentID(id + "-summary"), + ThreadID: thread.ID, + Body: summaryMarker + "\n\n" + summary, + Author: bot, + CommitSHA: strings.Repeat("a", 40), + Path: path, + Side: review.DiffSideRight, + Line: line, + SubjectType: review.AnchorKindLine, + CreatedAt: created, + UpdatedAt: created, + }) + return thread +} + func fakeLLMResult(sessionID, structured string, tokensIn, tokensOut int) llm.FakeResult { return llm.FakeResult{ SessionID: sessionID, diff --git a/internal/threadcontext/threadcontext.go b/internal/threadcontext/threadcontext.go index 7236ec3..f8705c8 100644 --- a/internal/threadcontext/threadcontext.go +++ b/internal/threadcontext/threadcontext.go @@ -20,12 +20,13 @@ type Options struct { // Thread is one normalized inline review thread. type Thread struct { - ID gitprovider.ThreadID - Resolved bool - Anchor Anchor - Comments []Comment - Status Status - ResolvedSummary *ThreadSummary + ID gitprovider.ThreadID + Resolved bool + Anchor Anchor + Comments []Comment + Status Status + ResolvedSummary *ThreadSummary + CRSettledSummary *ThreadSummary } // Anchor identifies the review-thread location. @@ -63,7 +64,8 @@ type Status struct { PendingHumanReply bool } -// ThreadSummary is compact context for a resolved thread. +// ThreadSummary is compact reviewer context for a provider-resolved or +// CR-settled inline thread. type ThreadSummary struct { ThreadID gitprovider.ThreadID Anchor Anchor @@ -75,7 +77,7 @@ type ThreadSummary struct { LastCommentHasThreadSummaryMarker bool } -// FileContext groups resolved summaries by file path. +// FileContext groups compact thread summaries by file path. type FileContext struct { Path string Summaries []ThreadSummary @@ -104,16 +106,10 @@ func Normalize(threads []gitprovider.InlineThread, opts Options) ([]Thread, erro normalized.Status = statusForComments(normalized.Comments) if normalized.Resolved && len(normalized.Comments) > 0 { last := normalized.Comments[len(normalized.Comments)-1] - normalized.ResolvedSummary = &ThreadSummary{ - ThreadID: normalized.ID, - Anchor: firstNonZeroAnchor(last.Anchor, normalized.Anchor), - URL: last.URL, - Body: last.Body, - LastCommentID: last.ID, - LastCommentAuthor: last.Author, - LastCommentAuthoredByPostingIdentity: last.AuthoredByPostingIdentity, - LastCommentHasThreadSummaryMarker: last.HasThreadSummaryMarker, - } + normalized.ResolvedSummary = threadSummaryFromComment(normalized.ID, normalized.Anchor, last) + } + if !normalized.Resolved && normalized.Status.LatestCRComment != nil && normalized.Status.LatestCRComment.HasThreadSummaryMarker && !normalized.Status.PendingHumanReply && strings.TrimSpace(normalized.Status.LatestCRComment.Body) != "" { + normalized.CRSettledSummary = threadSummaryFromComment(normalized.ID, normalized.Anchor, *normalized.Status.LatestCRComment) } out = append(out, normalized) } @@ -123,7 +119,19 @@ func Normalize(threads []gitprovider.InlineThread, opts Options) ([]Thread, erro return out, nil } -// FileScopedResolvedSummaries returns compact resolved thread summaries grouped +// EffectiveSettledSummary returns the compact settled context that reviewer +// prompts should use, preserving provider resolution as the preferred source. +func (thread Thread) EffectiveSettledSummary() (*ThreadSummary, bool) { + if thread.ResolvedSummary != nil { + return thread.ResolvedSummary, true + } + if thread.CRSettledSummary != nil { + return thread.CRSettledSummary, true + } + return nil, false +} + +// FileScopedResolvedSummaries returns provider-resolved thread summaries grouped // by file path. func FileScopedResolvedSummaries(threads []Thread) []FileContext { byPath := map[string][]ThreadSummary{} @@ -155,6 +163,39 @@ func FileScopedResolvedSummaries(threads []Thread) []FileContext { return out } +// FileScopedSettledSummaries returns compact provider-resolved and CR-settled +// thread summaries grouped by file path. +func FileScopedSettledSummaries(threads []Thread) []FileContext { + byPath := map[string][]ThreadSummary{} + for _, thread := range threads { + summary, ok := thread.EffectiveSettledSummary() + if !ok { + continue + } + copied := *summary + path := copied.Anchor.Path + if strings.TrimSpace(path) == "" { + path = thread.Anchor.Path + copied.Anchor.Path = path + } + byPath[path] = append(byPath[path], copied) + } + paths := make([]string, 0, len(byPath)) + for path := range byPath { + paths = append(paths, path) + } + sort.Strings(paths) + out := make([]FileContext, 0, len(paths)) + for _, path := range paths { + summaries := byPath[path] + sort.SliceStable(summaries, func(i, j int) bool { + return summaryLess(summaries[i], summaries[j]) + }) + out = append(out, FileContext{Path: path, Summaries: summaries}) + } + return out +} + // PendingCRAuthoredFindingThreads filters to unresolved CR-authored finding // threads that have a human reply after the latest CR-authored comment. func PendingCRAuthoredFindingThreads(threads []Thread) []Thread { @@ -306,6 +347,19 @@ func firstNonZeroAnchor(primary, fallback Anchor) Anchor { return primary } +func threadSummaryFromComment(threadID gitprovider.ThreadID, threadAnchor Anchor, comment Comment) *ThreadSummary { + return &ThreadSummary{ + ThreadID: threadID, + Anchor: firstNonZeroAnchor(comment.Anchor, threadAnchor), + URL: comment.URL, + Body: comment.Body, + LastCommentID: comment.ID, + LastCommentAuthor: comment.Author, + LastCommentAuthoredByPostingIdentity: comment.AuthoredByPostingIdentity, + LastCommentHasThreadSummaryMarker: comment.HasThreadSummaryMarker, + } +} + func anchorPresent(anchor Anchor) bool { return strings.TrimSpace(anchor.Path) != "" || anchor.Side != "" || diff --git a/internal/threadcontext/threadcontext_test.go b/internal/threadcontext/threadcontext_test.go index 6a4e1c3..380c866 100644 --- a/internal/threadcontext/threadcontext_test.go +++ b/internal/threadcontext/threadcontext_test.go @@ -267,6 +267,93 @@ func TestResolvedThreadCollapsesToSanitizedLastCommentSummary(t *testing.T) { } } +func TestUnresolvedThreadWithLatestCRSummaryBecomesCRSettled(t *testing.T) { + threads, err := Normalize([]gitprovider.InlineThread{{ + ID: "thread-1", + Resolved: false, + Path: "main.go", + Side: review.DiffSideRight, + Line: 12, + SubjectType: review.AnchorKindLine, + Comments: []gitprovider.ThreadComment{ + comment("c-1", bot(), "finding\n"+actionMarker(t, marker.ActionKindInlineComment), at(1)), + comment("c-2", human(), "thanks, fixed", at(2)), + comment("c-3", bot(), "settled summary\n"+threadSummaryMarker(t), at(3)), + }, + }}, Options{PostingIdentity: bot()}) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + + thread := threads[0] + if thread.ResolvedSummary != nil { + t.Fatalf("ResolvedSummary = %#v, want nil for provider-unresolved thread", thread.ResolvedSummary) + } + if thread.CRSettledSummary == nil { + t.Fatal("CRSettledSummary = nil, want summary") + } + if thread.CRSettledSummary.Body != "settled summary" { + t.Fatalf("CRSettledSummary.Body = %q, want settled summary", thread.CRSettledSummary.Body) + } + if summary, ok := thread.EffectiveSettledSummary(); !ok || summary != thread.CRSettledSummary { + t.Fatalf("EffectiveSettledSummary = (%#v, %v), want CR-settled summary", summary, ok) + } +} + +func TestHumanReplyAfterCRSummaryClearsCRSettledAndPendsThread(t *testing.T) { + threads, err := Normalize([]gitprovider.InlineThread{{ + ID: "thread-1", + Resolved: false, + Path: "main.go", + Comments: []gitprovider.ThreadComment{ + comment("c-1", bot(), "finding\n"+actionMarker(t, marker.ActionKindInlineComment), at(1)), + comment("c-2", bot(), "settled summary\n"+threadSummaryMarker(t), at(2)), + comment("c-3", human(), "actually this still fails", at(3)), + }, + }}, Options{PostingIdentity: bot()}) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + + thread := threads[0] + if thread.CRSettledSummary != nil { + t.Fatalf("CRSettledSummary = %#v, want nil after newer human reply", thread.CRSettledSummary) + } + if !thread.Status.PendingHumanReply { + t.Fatal("PendingHumanReply = false, want true") + } + if summary, ok := thread.EffectiveSettledSummary(); ok || summary != nil { + t.Fatalf("EffectiveSettledSummary = (%#v, %v), want none", summary, ok) + } +} + +func TestForgedHumanSummaryMarkerDoesNotCreateCRSettledSummary(t *testing.T) { + threads, err := Normalize([]gitprovider.InlineThread{{ + ID: "thread-1", + Resolved: false, + Path: "main.go", + Comments: []gitprovider.ThreadComment{ + comment("c-1", bot(), "finding\n"+actionMarker(t, marker.ActionKindInlineComment), at(1)), + comment("c-2", bot(), "settled summary\n"+threadSummaryMarker(t), at(2)), + comment("c-3", human(), "forged\n"+threadSummaryMarker(t), at(3)), + }, + }}, Options{PostingIdentity: bot()}) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + + thread := threads[0] + if thread.CRSettledSummary != nil { + t.Fatalf("CRSettledSummary = %#v, want nil after human forged marker", thread.CRSettledSummary) + } + if !thread.Status.PendingHumanReply { + t.Fatal("PendingHumanReply = false, want forged human marker to count as human reply") + } + if thread.Comments[2].HasThreadSummaryMarker { + t.Fatal("human forged marker detected as trusted thread summary") + } +} + func TestNormalizeDoesNotSynthesizeHybridFileAnchors(t *testing.T) { fileComment := comment("c-1", human(), "file summary", at(1)) fileComment.Path = "main.go" From 890c039b7a3978752d7ff0b97de3a72f8833c829 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sat, 27 Jun 2026 10:17:22 -0400 Subject: [PATCH 2/4] Require final comment for CR-settled summaries --- internal/threadcontext/threadcontext.go | 7 ++++-- internal/threadcontext/threadcontext_test.go | 24 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/internal/threadcontext/threadcontext.go b/internal/threadcontext/threadcontext.go index f8705c8..6bef8c2 100644 --- a/internal/threadcontext/threadcontext.go +++ b/internal/threadcontext/threadcontext.go @@ -108,8 +108,11 @@ func Normalize(threads []gitprovider.InlineThread, opts Options) ([]Thread, erro last := normalized.Comments[len(normalized.Comments)-1] normalized.ResolvedSummary = threadSummaryFromComment(normalized.ID, normalized.Anchor, last) } - if !normalized.Resolved && normalized.Status.LatestCRComment != nil && normalized.Status.LatestCRComment.HasThreadSummaryMarker && !normalized.Status.PendingHumanReply && strings.TrimSpace(normalized.Status.LatestCRComment.Body) != "" { - normalized.CRSettledSummary = threadSummaryFromComment(normalized.ID, normalized.Anchor, *normalized.Status.LatestCRComment) + if !normalized.Resolved && len(normalized.Comments) > 0 { + last := normalized.Comments[len(normalized.Comments)-1] + if last.AuthoredByPostingIdentity && last.HasThreadSummaryMarker && strings.TrimSpace(last.Body) != "" { + normalized.CRSettledSummary = threadSummaryFromComment(normalized.ID, normalized.Anchor, last) + } } out = append(out, normalized) } diff --git a/internal/threadcontext/threadcontext_test.go b/internal/threadcontext/threadcontext_test.go index 380c866..2cc2788 100644 --- a/internal/threadcontext/threadcontext_test.go +++ b/internal/threadcontext/threadcontext_test.go @@ -327,6 +327,30 @@ func TestHumanReplyAfterCRSummaryClearsCRSettledAndPendsThread(t *testing.T) { } } +func TestCRReplyAfterCRSummaryClearsCRSettledSummary(t *testing.T) { + threads, err := Normalize([]gitprovider.InlineThread{{ + ID: "thread-1", + Resolved: false, + Path: "main.go", + Comments: []gitprovider.ThreadComment{ + comment("c-1", bot(), "finding\n"+actionMarker(t, marker.ActionKindInlineComment), at(1)), + comment("c-2", bot(), "settled summary\n"+threadSummaryMarker(t), at(2)), + comment("c-3", bot(), "later unmarked bot reply", at(3)), + }, + }}, Options{PostingIdentity: bot()}) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + + thread := threads[0] + if thread.CRSettledSummary != nil { + t.Fatalf("CRSettledSummary = %#v, want nil when final comment is not the summary marker", thread.CRSettledSummary) + } + if summary, ok := thread.EffectiveSettledSummary(); ok || summary != nil { + t.Fatalf("EffectiveSettledSummary = (%#v, %v), want none", summary, ok) + } +} + func TestForgedHumanSummaryMarkerDoesNotCreateCRSettledSummary(t *testing.T) { threads, err := Normalize([]gitprovider.InlineThread{{ ID: "thread-1", From ba495fa15c2041d8c8b5971af2718488e6f38fa9 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sat, 27 Jun 2026 10:23:14 -0400 Subject: [PATCH 3/4] Cover CR-settled cache edge cases --- internal/cmd/respondcmd/respondcmd_test.go | 38 +++++++++ internal/pipeline/pipeline_test.go | 93 ++++++++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/internal/cmd/respondcmd/respondcmd_test.go b/internal/cmd/respondcmd/respondcmd_test.go index 383edd7..36b1b9a 100644 --- a/internal/cmd/respondcmd/respondcmd_test.go +++ b/internal/cmd/respondcmd/respondcmd_test.go @@ -143,6 +143,44 @@ func TestRespondJSONRendersStableShape(t *testing.T) { } } +func TestRespondJSONRendersPostedProviderResolve(t *testing.T) { + result := testThreadRespondResult(ledger.OutcomeComment) + result.PlannedActions[1].Status = ledger.PlannedActionPosted + responder := &fakeResponder{result: result} + cmd, out := newTestCommand(t, testConfig(), fakeFactory(responder)) + + if err := root.Execute(cmd, []string{"respond", "https://github.com/open-cli-collective/codereview-cli/pull/29", "--json"}); err != nil { + t.Fatalf("Execute respond json: %v", err) + } + var decoded struct { + Counts counts `json:"counts"` + } + if err := json.Unmarshal(out.Bytes(), &decoded); err != nil { + t.Fatalf("Unmarshal stdout: %v\n%s", err, out.String()) + } + if decoded.Counts.Resolved != 1 || + decoded.Counts.ProviderResolved != 1 || + decoded.Counts.ResolvePlanned != 1 || + decoded.Counts.ResolveFailed != 0 { + t.Fatalf("counts = %#v, want posted provider resolve plus planned resolve", decoded.Counts) + } +} + +func TestRespondTextRendersPostedProviderResolve(t *testing.T) { + result := testThreadRespondResult(ledger.OutcomeComment) + result.PlannedActions[1].Status = ledger.PlannedActionPosted + responder := &fakeResponder{result: result} + cmd, out := newTestCommand(t, testConfig(), fakeFactory(responder)) + + if err := root.Execute(cmd, []string{"respond", "https://github.com/open-cli-collective/codereview-cli/pull/29"}); err != nil { + t.Fatalf("Execute respond: %v", err) + } + text := out.String() + if !strings.Contains(text, "provider resolved 1 (resolve planned 1, failed 0)") { + t.Fatalf("stdout = %q, want posted provider resolve count", text) + } +} + func TestRespondRendersFailedProviderResolveSeparately(t *testing.T) { result := testThreadRespondResult(ledger.OutcomeComment) result.PlannedActions[0].Status = ledger.PlannedActionPosted diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index ec31dec..d22084b 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -1636,6 +1636,57 @@ func TestPrepareDossierArtifactsReusesCRSettledThreadSummaryWithoutLLM(t *testin assertFileOmits(t, discussionPath, "Original finding") } +func TestWriteRawDossierArtifactsMarksCachedSummarySource(t *testing.T) { + store := openPipelineStore(t) + defer closeStore(t, store) + layout := statepaths.NewLayout(t.TempDir(), t.TempDir()) + run := allocatePipelineRun(t, store, layout, "run-dossier-cached-summary-source", ledger.PostModeDryRun, fixedNow()) + provider, req := dryRunHarness(t) + artifacts := ArtifactPathsFromDir(run.ArtifactPath) + bot := req.PostingIdentity + human := gitprovider.Identity{Login: "human", ID: "human-id"} + providerResolved := crSettledReviewThread(t, "thread-provider", "main.go", 2, bot, human, "Provider summary") + providerResolved.Resolved = true + threads := []gitprovider.InlineThread{ + providerResolved, + crSettledReviewThread(t, "thread-cr-settled", "main.go", 4, bot, human, "CR-settled summary"), + } + threadContext, err := threadcontext.Normalize(threads, threadcontext.Options{PostingIdentity: bot}) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if err := writeRawDossierArtifacts(artifacts, dossierInputs{ + CurrentPR: provider.pr, + ReviewPR: provider.pr, + ChangedFiles: parseDiffPatchesForTest(t, provider.diff.Raw), + Threads: threads, + ThreadContext: threadContext, + Catalog: agents.Catalog{}, + CurrentBaseSHA: provider.pr.Base.SHA, + CurrentHeadSHA: provider.pr.Head.SHA, + }); err != nil { + t.Fatalf("writeRawDossierArtifacts: %v", err) + } + + var rawThreads []dossierInlineThreadArtifact + if err := readJSONFile(mustDossierRawPath(artifacts, "inline-threads.json"), &rawThreads); err != nil { + t.Fatalf("read raw inline threads: %v", err) + } + got := map[string]string{} + for _, thread := range rawThreads { + if thread.CachedSummary != nil { + got[thread.ID] = thread.CachedSummary.Source + } + } + want := map[string]string{ + "thread-provider": dossierCachedSummaryProviderSource, + "thread-cr-settled": dossierCachedSummaryCRSource, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("cached summary sources = %#v, want %#v", got, want) + } +} + func TestPrepareDossierArtifactsKeepsSameAnchorCachedSummariesByThreadID(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) @@ -1881,6 +1932,48 @@ func TestDecodeDossierDiscussionSummaryRejectsUnknownAnchor(t *testing.T) { } } +func TestDecodeDossierDiscussionSummaryThreadIDWinsOverMismatchedAnchor(t *testing.T) { + promptData, err := dossierDiscussionPromptInputFromDiscussion(dossierDiscussionArtifact{ + InlineThreads: []dossierInlineThreadArtifact{{ + ID: "thread-1", + Path: "main.go", + Side: "RIGHT", + Line: 2, + AnchorKind: "line", + Resolved: false, + Comments: []dossierThreadCommentArtifact{{ + Author: "review-bot", + Body: "Original thread body", + }}, + }}, + }) + if err != nil { + t.Fatalf("dossierDiscussionPromptInputFromDiscussion: %v", err) + } + got, err := decodeDossierDiscussionSummary([]byte(`{ + "schema_version": 1, + "inline_threads": [{ + "thread_id": "thread-1", + "path": "other.go", + "side": "LEFT", + "line": 99, + "anchor_kind": "file", + "status": "unresolved", + "summary": "Thread summary" + }] + }`), promptData) + if err != nil { + t.Fatalf("decodeDossierDiscussionSummary: %v", err) + } + if len(got.InlineThreads) != 1 { + t.Fatalf("inline threads = %#v, want one", got.InlineThreads) + } + thread := got.InlineThreads[0] + if thread.ThreadID != "thread-1" || thread.Path != "main.go" || thread.Side != "RIGHT" || thread.Line != 2 || thread.AnchorKind != "line" || thread.Resolved { + t.Fatalf("decoded thread = %#v, want source thread fields selected by thread_id", thread) + } +} + func TestSelectionOnlyRejectsInvalidSelection(t *testing.T) { ctx := context.Background() provider, req := dryRunHarness(t) From de1425765c45fbe16b797569842fbf7772c32e68 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Sat, 27 Jun 2026 13:17:51 -0400 Subject: [PATCH 4/4] Allow selection thread actions from thread context --- internal/pipeline/pipeline.go | 10 +++++++- internal/pipeline/pipeline_test.go | 40 ++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 0964123..07fba1b 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -1075,7 +1075,7 @@ func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequ knownThreadIDs := knownThreads(req.Threads) if len(req.ThreadContext) > 0 { promptInput, promptDeps, err = selectionPromptInputFromThreadContext(req.Artifacts, req.ThreadContext) - knownThreadIDs = map[string]bool{} + knownThreadIDs = knownThreadContext(req.ThreadContext) } if err != nil { return llm.Selection{}, sessionDraft{}, ledger.Session{}, err @@ -5135,6 +5135,14 @@ func knownThreads(threads []gitprovider.InlineThread) map[string]bool { return out } +func knownThreadContext(threads []threadcontext.Thread) map[string]bool { + out := make(map[string]bool, len(threads)) + for _, thread := range threads { + out[string(thread.ID)] = true + } + return out +} + func patchPaths(patches []FilePatch) []string { paths := make([]string, 0, len(patches)) for _, patch := range patches { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index d22084b..347bf6b 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -878,6 +878,46 @@ func TestSelectionOnlyRunsSingleSelectionPhaseWithoutReviewArtifacts(t *testing. assertDossierIndexArtifact(t, result.Artifacts.DossierDir, "raw/pr-context.json") } +func TestSelectionOnlyAllowsThreadActionsWithThreadContext(t *testing.T) { + ctx := context.Background() + provider, req := dryRunHarness(t) + human := gitprovider.Identity{Login: "human", ID: "human-id"} + provider.threads = []gitprovider.InlineThread{ + crSettledReviewThread(t, "thread-1", "main.go", 2, req.PostingIdentity, human, "Cached settled summary"), + } + adapter := &llm.FakeAdapter{NameValue: "fake-llm"} + adapter.Queue(fakeLLMResult("selection-session", `{ + "schema_version": 1, + "selected_agents": [{ + "agent_id": "harness:reviewer", + "rationale": "go file changed", + "files": ["main.go"] + }], + "thread_actions": [{ + "thread_id": "thread-1", + "decision": "summarize_only", + "summary": "Thread remains settled" + }], + "reasoning": "select reviewer and keep cached thread context" + }`, 10, 2)) + + result, err := selectionOnlyForTest(ctx, Options{ + Provider: provider, + Adapter: adapter, + Now: fixedNow, + }, selectionRequestFromReview(req, t.TempDir())) + if err != nil { + t.Fatalf("SelectionOnly: %v", err) + } + if len(result.Selection.ThreadActions) != 1 { + t.Fatalf("thread actions = %#v, want one", result.Selection.ThreadActions) + } + got := result.Selection.ThreadActions[0] + if got.ThreadID != "thread-1" || got.Decision != review.ThreadDecisionSummarizeOnly || got.Summary != "Thread remains settled" { + t.Fatalf("thread action = %#v, want decoded action for normalized thread context", got) + } +} + func TestSelectionOnlyPromptPreservesRoutingContractWithoutReviewerPromptBodies(t *testing.T) { ctx := context.Background() provider, req := dryRunHarness(t)