From 6c59cfbcaed4a9d33e3df5f100a4b308891c3d9e Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 07:49:34 -0400 Subject: [PATCH 1/6] fix(dossier): allow domain process vocabulary Contextualize dossier process-state validation so legitimate domain vocabulary remains valid while explicit PR state and bookkeeping remain excluded. Closes #559 --- internal/dossier/dossier.go | 21 ++++----- internal/dossier/dossier_test.go | 73 +++++++++++++++++++++++++++++- internal/pipeline/pipeline_test.go | 17 +++++-- 3 files changed, 94 insertions(+), 17 deletions(-) diff --git a/internal/dossier/dossier.go b/internal/dossier/dossier.go index deb06383..3f19fb4a 100644 --- a/internal/dossier/dossier.go +++ b/internal/dossier/dossier.go @@ -263,23 +263,18 @@ const ( const SummaryTaskID = dossierSummaryTaskID var forbiddenDiscussionSummaryPatterns = []*regexp.Regexp{ - regexp.MustCompile(`\bprovider[_ ]session[_ ]id\b`), - regexp.MustCompile(`\bsession[_ ]row[_ ]id\b`), - regexp.MustCompile(`\bsession[_ ]id\b`), - regexp.MustCompile(`\brun[_ ]id\b`), - regexp.MustCompile(`\bretry[_ ]state\b`), - regexp.MustCompile(`\bcache[_ ]state\b`), - regexp.MustCompile(`\bcache hit\b`), - regexp.MustCompile(`\bledger\b`), - regexp.MustCompile(`\bmergeab(?:le|ility)\b`), - regexp.MustCompile(`\bdraft\b`), - regexp.MustCompile(`\bapprovals?\b`), - regexp.MustCompile(`\bapproved\b`), + regexp.MustCompile(`\b(?:provider[_ ]session[_ ]id|session[_ ]row[_ ]id|session[_ ]id|run[_ ]id|retry[_ ]state|cache[_ ]state|cache hit)\s*[:=]\s*\S+`), + regexp.MustCompile(`\b(?:pr|pull request)\b(?:'s)?\s+(?:(?:is|was)\s+)?(?:approved(?:\s+by\s+\S+)?|a\s+draft|draft)\b`), + regexp.MustCompile(`\b(?:approved|draft)\s+(?:pr|pull request)\b`), + regexp.MustCompile(`\b(?:pr|pull request)\b(?:'s)?\s+(?:(?:is|was)\s+)?mergeab(?:le|ility)(?:\s+status)?\b`), + regexp.MustCompile(`\b(?:pr|pull request)\b(?:'s)?\s+approvals?\s+(?:state|status)\b`), regexp.MustCompile(`\brequested reviewers?\b`), regexp.MustCompile(`\brequested review\b`), regexp.MustCompile(`\bci status\b`), - regexp.MustCompile(`\bbuild failed\b`), + regexp.MustCompile(`\b(?:build|builds) failed\b`), + regexp.MustCompile(`\bfailed (?:build|builds)\b`), regexp.MustCompile(`\bcheck(s)? failed\b`), + regexp.MustCompile(`\bfailed check(s)?\b`), } // WriteRaw writes the source dossier artifacts used by Prepare. diff --git a/internal/dossier/dossier_test.go b/internal/dossier/dossier_test.go index 352e8dc6..e8d2bb64 100644 --- a/internal/dossier/dossier_test.go +++ b/internal/dossier/dossier_test.go @@ -1112,7 +1112,7 @@ func TestDecodeDossierDiscussionSummaryRejectsProcessState(t *testing.T) { if err != nil { t.Fatalf("dossierDiscussionPromptInputFromDiscussion: %v", err) } - for _, text := range []string{"CI status is red", "Build failed in CI", "Approved by alice", "run_id=1234"} { + for _, text := range []string{"CI status is red", "Build failed in CI", "The pull request was approved by alice", "run_id=1234"} { _, err := decodeDossierDiscussionSummary([]byte(fmt.Sprintf(`{ "schema_version": 1, "top_level_comments": [{"summary": %q}] @@ -1123,6 +1123,77 @@ func TestDecodeDossierDiscussionSummaryRejectsProcessState(t *testing.T) { } } +func TestDossierDiscussionSummaryProcessVocabularyMatrix(t *testing.T) { + accepted := []string{ + "Existing matching approval records remain retrievable after a run reaches a terminal state.", + "The approval request was approved by an administrator.", + "Draft invoices are stored in the ledger.", + "The session_id and run_id columns are indexed.", + "Cache hits update retry state transitions.", + } + rejected := []string{ + "The PR is approved.", + "The PR is approved by alice.", + "The PR was approved by alice.", + "The pull request is approved by alice.", + "The pull request was approved by alice.", + "The pull request is a draft.", + "PR mergeability status is clean.", + "The PR is mergeable.", + "The PR was mergeable.", + "The pull request is mergeable.", + "The pull request was mergeable.", + "Requested reviewers are listed.", + "Requested review is pending.", + "CI status is red.", + "Build failed in CI.", + "Checks failed in CI.", + "session_id=019fe123.", + "retry_state: exhausted.", + "cache_state=hit.", + } + for i, text := range accepted { + t.Run(fmt.Sprintf("accept_%d", i), func(t *testing.T) { + promptData, err := dossierDiscussionPromptInputFromDiscussion(dossierDiscussionArtifact{ + TopLevelComments: []dossierTopLevelCommentArtifact{{Body: text}}, + }) + if err != nil { + t.Fatalf("dossierDiscussionPromptInputFromDiscussion: %v", err) + } + if len(promptData.Input.TopLevelComments) != 1 { + t.Fatalf("prompt comments = %#v, want accepted vocabulary", promptData.Input.TopLevelComments) + } + _, err = decodeDossierDiscussionSummary([]byte(fmt.Sprintf(`{ + "schema_version": 1, + "top_level_comments": [{"summary": %q}] + }`, text)), promptData) + if err != nil { + t.Fatalf("decodeDossierDiscussionSummary(%q): %v, want accepted vocabulary", text, err) + } + }) + } + for i, text := range rejected { + t.Run(fmt.Sprintf("reject_%d", i), func(t *testing.T) { + promptData, err := dossierDiscussionPromptInputFromDiscussion(dossierDiscussionArtifact{ + TopLevelComments: []dossierTopLevelCommentArtifact{{Body: text}}, + }) + if err != nil { + t.Fatalf("dossierDiscussionPromptInputFromDiscussion: %v", err) + } + if len(promptData.Input.TopLevelComments) != 0 { + t.Fatalf("prompt comments = %#v, want process state omitted", promptData.Input.TopLevelComments) + } + _, err = decodeDossierDiscussionSummary([]byte(fmt.Sprintf(`{ + "schema_version": 1, + "top_level_comments": [{"summary": %q}] + }`, text)), promptData) + if err == nil || !strings.Contains(err.Error(), "excluded reviewer-facing process state") { + t.Fatalf("decodeDossierDiscussionSummary(%q) error = %v, want excluded process state", text, err) + } + }) + } +} + func TestDecodeDossierDiscussionSummaryRejectsUnknownAnchor(t *testing.T) { promptData, err := dossierDiscussionPromptInputFromDiscussion(dossierDiscussionArtifact{ InlineThreads: []dossierInlineThreadArtifact{{ diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index ea136f20..590aa1e2 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -219,12 +219,13 @@ func TestBuildPlanClassifiesActionIDFailureTerminalAcrossPaths(t *testing.T) { } } -func TestReviewPipelineAcceptanceHarnessPiRPCPermissionBoundedDryRunCompletesWithFakes(t *testing.T) { +func TestReviewPipelineAcceptanceHarnessPiRPCPermissionBoundedDryRunAllowsDossierDomainVocabulary(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) defer closeStore(t, store) provider, req := dryRunHarness(t) provider.pr.Body = "Document the checkout-native review contract." + dossierDomainVocabulary := "Existing matching approval records remain retrievable after a run reaches a terminal state." provider.threads = []gitprovider.InlineThread{{ ID: "thread-1", Resolved: false, @@ -242,6 +243,10 @@ func TestReviewPipelineAcceptanceHarnessPiRPCPermissionBoundedDryRunCompletesWit ID: "issue-1", Body: "Top-level concern", Author: gitprovider.Identity{Login: "maintainer"}, + }, { + ID: "issue-2", + Body: dossierDomainVocabulary, + Author: gitprovider.Identity{Login: "maintainer"}, }} provider.reviews = []gitprovider.Review{{ ID: "review-1", @@ -261,7 +266,7 @@ func TestReviewPipelineAcceptanceHarnessPiRPCPermissionBoundedDryRunCompletesWit QuotaValue: llm.Quota{BlockRemainingPct: 87, WeeklyRemainingPct: 64}, QuotaSupported: true, } - baseAdapter.Queue(fakeLLMResult("dossier-summary-session", discussionSummaryJSON([]string{"Top-level concern", "Review body"}, []threadSummary{{path: "main.go", line: 2, status: "unresolved", summary: "Inline concern"}}), 8, 2)) + baseAdapter.Queue(fakeLLMResult("dossier-summary-session", discussionSummaryJSON([]string{"Top-level concern", "Review body", dossierDomainVocabulary}, []threadSummary{{path: "main.go", line: 2, status: "unresolved", summary: "Inline concern"}}), 8, 2)) baseAdapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 10, 2)) baseAdapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) baseAdapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 30, 6)) @@ -272,6 +277,7 @@ func TestReviewPipelineAcceptanceHarnessPiRPCPermissionBoundedDryRunCompletesWit "Top-level concern", "Inline concern", "Review body", + dossierDomainVocabulary, }, }, promptValidation{ @@ -332,6 +338,10 @@ func TestReviewPipelineAcceptanceHarnessPiRPCPermissionBoundedDryRunCompletesWit t.Fatalf("DryRun: %v", err) } adapter.AssertConsumed(t) + dossierMeta, ok, err := llmlifecycle.ReadMetadata(lifecyclePaths(result.Artifacts), dossierSummaryTaskID) + if err != nil || !ok || dossierMeta.Status != llmlifecycle.StatusSucceeded { + t.Fatalf("dossier metadata = %#v ok=%t err=%v, want succeeded task before selection", dossierMeta, ok, err) + } if result.Run.RunID != "run-1" || result.Run.PostMode != ledger.PostModeDryRun { t.Fatalf("run = %#v, want dry-run run-1", result.Run) @@ -424,11 +434,12 @@ func TestReviewPipelineAcceptanceHarnessPiRPCPermissionBoundedDryRunCompletesWit assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "discussion.md"), "main.go:2") assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "discussion.md"), "Top-level concern") assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "discussion.md"), "Review body") + assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "discussion.md"), dossierDomainVocabulary) assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "repo-guidance.md"), "Guidance provenance: repo@refs/heads/main:") assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "repo-guidance.md"), "Guidance source status: available") assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "final", "repo-guidance.md"), "PR-head .codereview/agents changes do not affect this listing.") assertDossierIndexArtifact(t, result.Artifacts.DossierDir, "final/discussion.md") - assertFileOmits(t, filepath.Join(result.Artifacts.DossierDir, "final", "discussion.md"), "provider_session_id", "session_row_id", "mergeability", "approval", "CI status", "Approved body should stay out of reviewer-facing discussion") + assertFileOmits(t, filepath.Join(result.Artifacts.DossierDir, "final", "discussion.md"), "provider_session_id", "session_row_id", "mergeability", "approval state", "CI status", "Approved body should stay out of reviewer-facing discussion") assertFileContains(t, filepath.Join(result.Artifacts.DossierDir, "raw", "top-level-comments.json"), "Approved body should stay out of reviewer-facing discussion") slicePath, err := result.Artifacts.SlicePatch("harness:reviewer", "main.go") if err != nil { From f82b8908de9b09e0ab9f8c56f991d8be43600b97 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 08:28:38 -0400 Subject: [PATCH 2/6] fix(review): tolerate inherently unassigned cohort files Preserve saved cohort reuse when no current catalog agent can cover a changed file, while retaining fresh-session guidance when a broad or matching out-of-cohort agent can cover it. Closes #562 --- docs/review-lifecycle.md | 8 +++- internal/pipeline/pipeline.go | 6 ++- internal/pipeline/pipeline_test.go | 65 +++++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/docs/review-lifecycle.md b/docs/review-lifecycle.md index c519f99f..cfd18d52 100644 --- a/docs/review-lifecycle.md +++ b/docs/review-lifecycle.md @@ -54,8 +54,12 @@ new cohort, and it starts new provider conversations only through the same missing-conversation fallback. Reuse fails with `--fresh-session` guidance when a cohort member is missing or -runtime-incompatible, `--max-agents` is smaller than the saved cohort, or the -cohort cannot cover every changed file. +runtime-incompatible, `--max-agents` is smaller than the saved cohort, or a +changed file is coverable by a current catalog agent (broad with no file globs +or matching a file glob) but no saved cohort member can take it. If no current +catalog agent can cover a changed file, reuse leaves it unassigned and the +downstream coverage gate reports `incomplete_unassigned` without forcing a +fresh session. ## `--fresh-session` diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 019c0faa..ea972371 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -1655,7 +1655,11 @@ func rebaseReviewerCohort(req Request, catalog agents.Catalog, cohort ledger.Rev break } if !assigned { - return freshError("saved reviewer cohort leaves changed file %q uncovered", file) + for _, current := range catalog.Agents { + if len(current.FileGlobs) == 0 || globsMatchFile(current.FileGlobs, file) { + return freshError("saved reviewer cohort leaves changed file %q uncovered", file) + } + } } } diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 590aa1e2..33fe96ac 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -5165,6 +5165,61 @@ func TestRebaseReviewerCohortAssignsNewFilesToPersistedBroadMember(t *testing.T) } } +func TestRebaseReviewerCohortLeavesInherentlyUnmatchedFilesUnassigned(t *testing.T) { + req := Request{Profile: testProfile(""), ProfileName: "default"} + cohort := ledger.ReviewerCohort{Adapter: "fake-llm", Members: []ledger.ReviewerCohortMember{{ + AgentID: "repo:go", AssignmentMode: ledger.ReviewerAssignmentScoped, + Files: []string{"main.go"}, AllowedFiles: []string{"main.go"}, + Model: "claude-sonnet-5", Effort: "medium", ProviderSessionID: "go-session", + }}} + catalog := agents.Catalog{Agents: []agents.Agent{{ + ID: "repo:go", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.go"}, + }}} + + selection, resumes, err := rebaseReviewerCohort(req, catalog, cohort, []string{"main.go", ".github/workflows/ci.yml"}, 0, "fake-llm") + if err != nil { + t.Fatalf("rebaseReviewerCohort: %v", err) + } + want := []llm.SelectedAgent{{ + AgentID: "repo:go", Rationale: "reused reviewer cohort", + Files: []string{"main.go"}, AllowedFiles: []string{"main.go"}, + }} + if !reflect.DeepEqual(selection.SelectedAgents, want) { + t.Fatalf("selected agents = %#v, want %#v", selection.SelectedAgents, want) + } + if !reflect.DeepEqual(resumes, map[string]string{"repo:go": "go-session"}) { + t.Fatalf("reviewer resumes = %#v, want exact saved session", resumes) + } + coverage := buildReviewerCoverage( + selection.SelectedAgents, + []llm.Findings{{AgentID: "repo:go", InspectedFiles: []string{"main.go"}}}, + nil, + []string{"main.go", ".github/workflows/ci.yml"}, + ) + if len(coverage) != 2 || coverage[1].AgentID != "unassigned" || coverage[1].Status != reviewerCoverageIncompleteUnassigned || + !reflect.DeepEqual(coverage[1].SkippedFiles, []string{".github/workflows/ci.yml"}) { + t.Fatalf("coverage = %#v, want workflow file unassigned", coverage) + } +} + +func TestRebaseReviewerCohortRejectsFileCoveredByBroadCatalogAgent(t *testing.T) { + req := Request{Profile: testProfile(""), ProfileName: "default"} + cohort := ledger.ReviewerCohort{Adapter: "fake-llm", Members: []ledger.ReviewerCohortMember{{ + AgentID: "repo:go", AssignmentMode: ledger.ReviewerAssignmentScoped, + Files: []string{"main.go"}, AllowedFiles: []string{"main.go"}, + Model: "claude-sonnet-5", Effort: "medium", + }}} + catalog := agents.Catalog{Agents: []agents.Agent{ + {ID: "repo:go", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.go"}}, + {ID: "shared:general", ModelTier: "medium", Effort: "medium"}, + }} + + _, _, err := rebaseReviewerCohort(req, catalog, cohort, []string{"main.go", ".github/workflows/ci.yml"}, 0, "fake-llm") + if err == nil || !strings.Contains(err.Error(), ".github/workflows/ci.yml") || !strings.Contains(err.Error(), "--fresh-session") { + t.Fatalf("rebaseReviewerCohort error = %v, want broad catalog coverage to retain fresh-session guidance", err) + } +} + func TestPersistReviewerCohortTreatsFilesOnlyAssignmentAsScoped(t *testing.T) { store := openPipelineStore(t) defer closeStore(t, store) @@ -5262,13 +5317,19 @@ func TestRebaseReviewerCohortRejectsIncompatibleOrUncoveredState(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { candidate := cohort + candidateCatalog := catalog + if tc.name == "uncovered" { + candidateCatalog.Agents = append(candidateCatalog.Agents, agents.Agent{ + ID: "repo:sql", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.sql"}, + }) + } if tc.name == "max agents" { candidate.Members = append(candidate.Members, ledger.ReviewerCohortMember{AgentID: "repo:other", AssignmentMode: ledger.ReviewerAssignmentBroad, Model: "claude-sonnet-5", Effort: "medium"}) - catalog.Agents = append(catalog.Agents, agents.Agent{ID: "repo:other", ModelTier: "medium", Effort: "medium"}) + candidateCatalog.Agents = append(candidateCatalog.Agents, agents.Agent{ID: "repo:other", ModelTier: "medium", Effort: "medium"}) tc.maxAgents = 1 tc.wantDetail = "--max-agents" } - _, _, err := rebaseReviewerCohort(req, catalog, candidate, tc.files, tc.maxAgents, tc.adapter) + _, _, err := rebaseReviewerCohort(req, candidateCatalog, candidate, tc.files, tc.maxAgents, tc.adapter) if err == nil || !strings.Contains(err.Error(), tc.wantDetail) || !strings.Contains(err.Error(), "--fresh-session") { t.Fatalf("rebaseReviewerCohort error = %v, want %q and fresh-session guidance", err, tc.wantDetail) } From 5ff5d1c280f397524428db77bff92461db67467b Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 08:53:38 -0400 Subject: [PATCH 3/6] fix(dossier): reject process-state variants --- internal/dossier/dossier.go | 3 ++- internal/dossier/dossier_test.go | 44 ++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/internal/dossier/dossier.go b/internal/dossier/dossier.go index 3f19fb4a..aeaf65fc 100644 --- a/internal/dossier/dossier.go +++ b/internal/dossier/dossier.go @@ -264,13 +264,14 @@ const SummaryTaskID = dossierSummaryTaskID var forbiddenDiscussionSummaryPatterns = []*regexp.Regexp{ regexp.MustCompile(`\b(?:provider[_ ]session[_ ]id|session[_ ]row[_ ]id|session[_ ]id|run[_ ]id|retry[_ ]state|cache[_ ]state|cache hit)\s*[:=]\s*\S+`), - regexp.MustCompile(`\b(?:pr|pull request)\b(?:'s)?\s+(?:(?:is|was)\s+)?(?:approved(?:\s+by\s+\S+)?|a\s+draft|draft)\b`), + regexp.MustCompile(`\b(?:pr|pull request)\b(?:'s)?\s+(?:(?:is|was|has been)\s+)?(?:approved(?:\s+by\s+\S+)?|a\s+draft|draft)\b`), regexp.MustCompile(`\b(?:approved|draft)\s+(?:pr|pull request)\b`), regexp.MustCompile(`\b(?:pr|pull request)\b(?:'s)?\s+(?:(?:is|was)\s+)?mergeab(?:le|ility)(?:\s+status)?\b`), regexp.MustCompile(`\b(?:pr|pull request)\b(?:'s)?\s+approvals?\s+(?:state|status)\b`), regexp.MustCompile(`\brequested reviewers?\b`), regexp.MustCompile(`\brequested review\b`), regexp.MustCompile(`\bci status\b`), + regexp.MustCompile(`\b(?:(?:ci|check|build)\s+(?:is|was|has been)|(?:checks|builds)\s+(?:are|were|have been))\s+failing\b`), regexp.MustCompile(`\b(?:build|builds) failed\b`), regexp.MustCompile(`\bfailed (?:build|builds)\b`), regexp.MustCompile(`\bcheck(s)? failed\b`), diff --git a/internal/dossier/dossier_test.go b/internal/dossier/dossier_test.go index e8d2bb64..c8f2971a 100644 --- a/internal/dossier/dossier_test.go +++ b/internal/dossier/dossier_test.go @@ -1137,6 +1137,7 @@ func TestDossierDiscussionSummaryProcessVocabularyMatrix(t *testing.T) { "The PR was approved by alice.", "The pull request is approved by alice.", "The pull request was approved by alice.", + "The pull request has been approved.", "The pull request is a draft.", "PR mergeability status is clean.", "The PR is mergeable.", @@ -1145,6 +1146,18 @@ func TestDossierDiscussionSummaryProcessVocabularyMatrix(t *testing.T) { "The pull request was mergeable.", "Requested reviewers are listed.", "Requested review is pending.", + "CI is failing.", + "CI was failing.", + "CI has been failing.", + "Checks are failing.", + "Checks were failing.", + "Checks have been failing.", + "Build is failing.", + "Build was failing.", + "Build has been failing.", + "Builds are failing.", + "Builds were failing.", + "Builds have been failing.", "CI status is red.", "Build failed in CI.", "Checks failed in CI.", @@ -1194,6 +1207,37 @@ func TestDossierDiscussionSummaryProcessVocabularyMatrix(t *testing.T) { } } +func TestDossierDiscussionSummaryFiltersProcessStateFromInlineContent(t *testing.T) { + promptData, err := dossierDiscussionPromptInputFromDiscussion(dossierDiscussionArtifact{ + InlineThreads: []dossierInlineThreadArtifact{ + { + ID: "inline-comment", Path: "main.go", Side: "RIGHT", Line: 2, AnchorKind: "line", + Comments: []dossierThreadCommentArtifact{{Body: "Checks are failing."}}, + }, + { + ID: "cached-summary", Path: "other.go", Side: "RIGHT", Line: 4, AnchorKind: "line", + CachedSummary: &dossierCachedThreadSummaryArtifact{ + ThreadID: "cached-summary", Body: "The pull request has been approved.", + }, + }, + }, + }) + if err != nil { + t.Fatalf("dossierDiscussionPromptInputFromDiscussion: %v", err) + } + if len(promptData.CachedInlineSummaries) != 0 { + t.Fatalf("cached inline summaries = %#v, want process state excluded", promptData.CachedInlineSummaries) + } + if len(promptData.Input.InlineThreads) != 2 { + t.Fatalf("inline threads = %#v, want both threads without excluded text", promptData.Input.InlineThreads) + } + for _, thread := range promptData.Input.InlineThreads { + if len(thread.Comments) != 0 { + t.Fatalf("inline thread %q comments = %#v, want process state excluded", thread.ThreadID, thread.Comments) + } + } +} + func TestDecodeDossierDiscussionSummaryRejectsUnknownAnchor(t *testing.T) { promptData, err := dossierDiscussionPromptInputFromDiscussion(dossierDiscussionArtifact{ InlineThreads: []dossierInlineThreadArtifact{{ From b838e2b55c210b59728db64ef552a1095d2dd129 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 08:54:03 -0400 Subject: [PATCH 4/6] fix(review): retain all-unmatched coverage --- internal/pipeline/pipeline.go | 2 +- internal/pipeline/pipeline_test.go | 46 ++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index ea972371..841b89c3 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -2438,7 +2438,7 @@ func reviewerToolEvidenceByAgent(sessions []sessionDraft) map[string]*llm.Review } func buildReviewerCoverage(selected []llm.SelectedAgent, results []llm.Findings, failures []ReviewerFailure, changedFiles []string, toolEvidence ...map[string]*llm.ReviewerToolEvidence) []reviewplan.ReviewerCoverageSummary { - if len(selected) == 0 { + if len(selected) == 0 && len(changedFiles) == 0 { return nil } resultByAgent := make(map[string]llm.Findings, len(results)) diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 33fe96ac..2cbb814b 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -5220,6 +5220,52 @@ func TestRebaseReviewerCohortRejectsFileCoveredByBroadCatalogAgent(t *testing.T) } } +func TestRebaseReviewerCohortOnlyInherentlyUnmatchedFileRemainsUnassigned(t *testing.T) { + req := Request{Profile: testProfile(""), ProfileName: "default"} + cohort := ledger.ReviewerCohort{Adapter: "fake-llm", Members: []ledger.ReviewerCohortMember{{ + AgentID: "repo:go", AssignmentMode: ledger.ReviewerAssignmentScoped, + Files: []string{"main.go"}, AllowedFiles: []string{"main.go"}, + Model: "claude-sonnet-5", Effort: "medium", + }}} + catalog := agents.Catalog{Agents: []agents.Agent{{ + ID: "repo:go", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.go"}, + }}} + changedFiles := []string{".github/workflows/ci.yml"} + + selection, resumes, err := rebaseReviewerCohort(req, catalog, cohort, changedFiles, 0, "fake-llm") + if err != nil { + t.Fatalf("rebaseReviewerCohort: %v", err) + } + if len(selection.SelectedAgents) != 0 || len(resumes) != 0 { + t.Fatalf("reused cohort selection = %#v resumes = %#v, want no assigned reviewers or resumes", selection.SelectedAgents, resumes) + } + coverage := buildReviewerCoverage(selection.SelectedAgents, nil, nil, changedFiles) + wantCoverage := []reviewplan.ReviewerCoverageSummary{{ + AgentID: "unassigned", + Status: reviewerCoverageIncompleteUnassigned, + SkippedFiles: changedFiles, + Diagnostic: "changed files were not assigned to a selected reviewer", + }} + if !reflect.DeepEqual(coverage, wantCoverage) { + t.Fatalf("coverage = %#v, want exactly %#v", coverage, wantCoverage) + } + plan, err := reviewplan.Build(reviewplan.Request{ + PostMode: reviewplan.PostModeDryRun, + Rollup: review.Rollup{ReviewEvent: review.ReviewEventApprove}, + RunSummary: reviewplan.RunSummary{ + ReviewerCoverage: coverage, + }, + Now: fixedNow, + NewActionID: actionSequence(), + }) + if err != nil { + t.Fatalf("reviewplan.Build: %v", err) + } + if plan.Outcome == reviewplan.OutcomeApproved { + t.Fatalf("plan outcome = %q, want incomplete coverage to prevent approval", plan.Outcome) + } +} + func TestPersistReviewerCohortTreatsFilesOnlyAssignmentAsScoped(t *testing.T) { store := openPipelineStore(t) defer closeStore(t, store) From dd14296e9acf3c658f5d27dbec7f4b711ddc9764 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 09:05:55 -0400 Subject: [PATCH 5/6] fix(dossier): cover prose process-state leaks --- internal/dossier/dossier.go | 3 ++- internal/dossier/dossier_test.go | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/dossier/dossier.go b/internal/dossier/dossier.go index aeaf65fc..74e95e53 100644 --- a/internal/dossier/dossier.go +++ b/internal/dossier/dossier.go @@ -263,7 +263,7 @@ const ( const SummaryTaskID = dossierSummaryTaskID var forbiddenDiscussionSummaryPatterns = []*regexp.Regexp{ - regexp.MustCompile(`\b(?:provider[_ ]session[_ ]id|session[_ ]row[_ ]id|session[_ ]id|run[_ ]id|retry[_ ]state|cache[_ ]state|cache hit)\s*[:=]\s*\S+`), + regexp.MustCompile(`\b(?:provider[_ ]session[_ ]id|session[_ ]row[_ ]id|session[_ ]id|run[_ ]id|retry[_ ]state|cache[_ ]state|cache hit)(?:\s*[:=]\s*|\s+(?:is|was|has been|had been)\s+)\S+`), regexp.MustCompile(`\b(?:pr|pull request)\b(?:'s)?\s+(?:(?:is|was|has been)\s+)?(?:approved(?:\s+by\s+\S+)?|a\s+draft|draft)\b`), regexp.MustCompile(`\b(?:approved|draft)\s+(?:pr|pull request)\b`), regexp.MustCompile(`\b(?:pr|pull request)\b(?:'s)?\s+(?:(?:is|was)\s+)?mergeab(?:le|ility)(?:\s+status)?\b`), @@ -271,6 +271,7 @@ var forbiddenDiscussionSummaryPatterns = []*regexp.Regexp{ regexp.MustCompile(`\brequested reviewers?\b`), regexp.MustCompile(`\brequested review\b`), regexp.MustCompile(`\bci status\b`), + regexp.MustCompile(`\bci\s+(?:failed|has failed|had failed)\b`), regexp.MustCompile(`\b(?:(?:ci|check|build)\s+(?:is|was|has been)|(?:checks|builds)\s+(?:are|were|have been))\s+failing\b`), regexp.MustCompile(`\b(?:build|builds) failed\b`), regexp.MustCompile(`\bfailed (?:build|builds)\b`), diff --git a/internal/dossier/dossier_test.go b/internal/dossier/dossier_test.go index c8f2971a..197298ed 100644 --- a/internal/dossier/dossier_test.go +++ b/internal/dossier/dossier_test.go @@ -1158,10 +1158,15 @@ func TestDossierDiscussionSummaryProcessVocabularyMatrix(t *testing.T) { "Builds are failing.", "Builds were failing.", "Builds have been failing.", + "CI failed.", + "CI has failed.", + "CI had failed.", "CI status is red.", "Build failed in CI.", "Checks failed in CI.", "session_id=019fe123.", + "The session ID is 019fe123.", + "The run_id was abc.", "retry_state: exhausted.", "cache_state=hit.", } From 8b3efbe3f95c18277ad25a3590f63976c4a96c54 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Mon, 10 Aug 2026 09:13:39 -0400 Subject: [PATCH 6/6] fix(dossier): cover terminal CI variants --- internal/dossier/dossier.go | 9 +++------ internal/dossier/dossier_test.go | 3 +++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/dossier/dossier.go b/internal/dossier/dossier.go index 74e95e53..153dbb97 100644 --- a/internal/dossier/dossier.go +++ b/internal/dossier/dossier.go @@ -271,12 +271,9 @@ var forbiddenDiscussionSummaryPatterns = []*regexp.Regexp{ regexp.MustCompile(`\brequested reviewers?\b`), regexp.MustCompile(`\brequested review\b`), regexp.MustCompile(`\bci status\b`), - regexp.MustCompile(`\bci\s+(?:failed|has failed|had failed)\b`), - regexp.MustCompile(`\b(?:(?:ci|check|build)\s+(?:is|was|has been)|(?:checks|builds)\s+(?:are|were|have been))\s+failing\b`), - regexp.MustCompile(`\b(?:build|builds) failed\b`), - regexp.MustCompile(`\bfailed (?:build|builds)\b`), - regexp.MustCompile(`\bcheck(s)? failed\b`), - regexp.MustCompile(`\bfailed check(s)?\b`), + regexp.MustCompile(`\b(?:ci|build|check)\s+(?:failed|(?:has|had) failed|(?:is|was|has been) failing)\b`), + regexp.MustCompile(`\b(?:builds|checks)\s+(?:failed|(?:have|had) failed|(?:are|were|have been) failing)\b`), + regexp.MustCompile(`\bfailed (?:build|builds|check|checks)\b`), } // WriteRaw writes the source dossier artifacts used by Prepare. diff --git a/internal/dossier/dossier_test.go b/internal/dossier/dossier_test.go index 197298ed..776c2fe2 100644 --- a/internal/dossier/dossier_test.go +++ b/internal/dossier/dossier_test.go @@ -1161,6 +1161,9 @@ func TestDossierDiscussionSummaryProcessVocabularyMatrix(t *testing.T) { "CI failed.", "CI has failed.", "CI had failed.", + "Build has failed.", + "Checks have failed.", + "Builds had failed.", "CI status is red.", "Build failed in CI.", "Checks failed in CI.",