From e6d48e86d17973c679307554ee0efd07d89bcfaf Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 22 Jul 2026 21:27:54 -0400 Subject: [PATCH 1/2] fix: deterministically assign glob-matched files uncovered by selection The orchestrator-selection model proposes file-to-reviewer assignments, but a resumed per-PR session can anchor on assignments from an earlier pass and ignore updated agent definitions (#526). Changed files its agents' globs now match then land in the unassigned coverage bucket, and hasIncompleteReviewerCoverage downgrades every approve verdict to comment - permanently, since each re-review resumes the same session. Add ensureSelectedGlobCoverage after required-on-match injection and agent capping: any changed file outside every selected agent's scope is assigned to each selected agent whose file_globs match it. Files that match no selected agent still surface as unassigned, so the gate keeps catching genuinely uncovered files. Scope semantics are preserved - AllowedFiles is only extended when it defines the agent's scope, and full-scope agents (no explicit files) are untouched. Extracts globsMatchFile from requiredOnMatchFiles for reuse. Fixes #526 Claude-Session: https://claude.ai/code/session_01NfzotSGPeRHabbn66nNkEC --- internal/pipeline/pipeline.go | 45 +++++++++++++++++++++++++ internal/pipeline/pipeline_test.go | 53 ++++++++++++++++++++++++++++++ internal/pipeline/prompts.go | 27 ++++++++++----- 3 files changed, 117 insertions(+), 8 deletions(-) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index d123b4c..13c4be9 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -1218,6 +1218,7 @@ func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequ if err != nil { return llm.Selection{}, selectionSession, ledgerSession, Failure(FailureTerminal, err) } + selection = ensureSelectedGlobCoverage(selection, req.Catalog, changed) opts.emitReviewerSelection(req.Catalog, selection) return selection, selectionSession, ledgerSession, nil } @@ -1316,6 +1317,50 @@ func ensureRequiredOnMatchAgents(selection llm.Selection, catalog agents.Catalog return selection } +// ensureSelectedGlobCoverage assigns every changed file that no selected +// agent's scope covers to each selected agent whose file globs match it. The +// orchestrator proposes assignments, but a resumed selection session can +// anchor on assignments from an earlier pass and ignore updated agent +// definitions (#526); without this backstop such files land in the +// unassigned coverage bucket, and hasIncompleteReviewerCoverage then blocks +// approval on every subsequent pass. Agents whose scope is already all +// changed files (no explicit Files/AllowedFiles) need no widening, and +// AllowedFiles is only extended when it is what defines the agent's scope. +func ensureSelectedGlobCoverage(selection llm.Selection, catalog agents.Catalog, changedFiles []string) llm.Selection { + if len(selection.SelectedAgents) == 0 { + return selection + } + agentByID := make(map[string]agents.Agent, len(catalog.Agents)) + for _, candidate := range catalog.Agents { + agentByID[candidate.ID] = candidate + } + covered := map[string]bool{} + for _, selected := range selection.SelectedAgents { + for _, file := range reviewerAssignmentScope(selected, changedFiles) { + covered[file] = true + } + } + for _, file := range changedFiles { + if covered[file] { + continue + } + for i := range selection.SelectedAgents { + selected := &selection.SelectedAgents[i] + candidate, ok := agentByID[selected.AgentID] + if !ok || !globsMatchFile(candidate.FileGlobs, file) { + continue + } + if len(selected.AllowedFiles) > 0 { + selected.AllowedFiles = append(selected.AllowedFiles, file) + } + if len(selected.Files) > 0 { + selected.Files = append(selected.Files, file) + } + } + } + return selection +} + func (opts Options) emitReviewerCatalog(catalog agents.Catalog, changedFiles []string) { if opts.ReviewProgress == nil { return diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 9886ceb..a47c624 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -12,6 +12,7 @@ import ( "path/filepath" "reflect" "regexp" + "slices" "strings" "sync" "testing" @@ -1356,6 +1357,58 @@ func TestRequiredOnMatchAgentsHaveCapPriorityAndCannotBeTruncated(t *testing.T) } } +func TestEnsureSelectedGlobCoverageAssignsUncoveredMatchingFiles(t *testing.T) { + // Regression for #526: a resumed selection session anchored on stale + // assignments and omitted files its agents' updated globs now match, + // leaving them in the unassigned coverage bucket. + catalog := agents.Catalog{Agents: []agents.Agent{ + {ID: "shared:go", FileGlobs: []string{"**/*.go", "**/testdata/**", "CHANGELOG.md"}}, + {ID: "shared:docs", FileGlobs: []string{"README*", "CHANGELOG.md"}}, + {ID: "shared:unrelated", FileGlobs: []string{"*.rs"}}, + }} + selection := llm.Selection{SelectedAgents: []llm.SelectedAgent{ + {AgentID: "shared:go", Files: []string{"main.go"}}, + {AgentID: "shared:docs", Files: []string{"README.md"}, AllowedFiles: []string{"README.md"}}, + {AgentID: "shared:unrelated", Files: []string{"main.rs"}}, + }} + changed := []string{"main.go", "README.md", "main.rs", "CHANGELOG.md", "api/testdata/fixture.json", "LICENSE"} + + got := ensureSelectedGlobCoverage(selection, catalog, changed) + + wantGo := []string{"main.go", "CHANGELOG.md", "api/testdata/fixture.json"} + if !reflect.DeepEqual(got.SelectedAgents[0].Files, wantGo) { + t.Fatalf("go files = %#v, want %#v", got.SelectedAgents[0].Files, wantGo) + } + if len(got.SelectedAgents[0].AllowedFiles) != 0 { + t.Fatalf("go allowed files = %#v, want empty so scope stays Files-driven", got.SelectedAgents[0].AllowedFiles) + } + wantDocs := []string{"README.md", "CHANGELOG.md"} + if !reflect.DeepEqual(got.SelectedAgents[1].Files, wantDocs) || !reflect.DeepEqual(got.SelectedAgents[1].AllowedFiles, wantDocs) { + t.Fatalf("docs assignment = %#v, want files and allowed files %#v", got.SelectedAgents[1], wantDocs) + } + if !reflect.DeepEqual(got.SelectedAgents[2].Files, []string{"main.rs"}) { + t.Fatalf("unrelated files = %#v, want unchanged", got.SelectedAgents[2].Files) + } + // LICENSE matches no selected agent's globs and must stay unassigned so + // the coverage gate still reports genuinely uncovered files. + for _, selected := range got.SelectedAgents { + if slices.Contains(selected.Files, "LICENSE") || slices.Contains(selected.AllowedFiles, "LICENSE") { + t.Fatalf("LICENSE assigned to %s, want unassigned", selected.AgentID) + } + } +} + +func TestEnsureSelectedGlobCoverageLeavesFullScopeAgentsAlone(t *testing.T) { + catalog := agents.Catalog{Agents: []agents.Agent{{ID: "shared:all", FileGlobs: []string{"**"}}}} + selection := llm.Selection{SelectedAgents: []llm.SelectedAgent{{AgentID: "shared:all"}}} + + got := ensureSelectedGlobCoverage(selection, catalog, []string{"a.go", "b.md"}) + + if len(got.SelectedAgents[0].Files) != 0 || len(got.SelectedAgents[0].AllowedFiles) != 0 { + t.Fatalf("full-scope agent = %#v, want untouched (scope already covers all changed files)", got.SelectedAgents[0]) + } +} + func TestReviewerProgressReportsWinningSourcesAndFinalAssignments(t *testing.T) { recorder := &reviewerProgressRecorder{} opts := Options{ReviewProgress: recorder} diff --git a/internal/pipeline/prompts.go b/internal/pipeline/prompts.go index 7bd9ee4..ccf0e97 100644 --- a/internal/pipeline/prompts.go +++ b/internal/pipeline/prompts.go @@ -139,22 +139,33 @@ func requiredOnMatchFiles(agent agents.Agent, changedFiles []string) []string { return nil } var matched []string - for _, pattern := range agent.FileGlobs { + for _, file := range changedFiles { + if globsMatchFile(agent.FileGlobs, file) && !slices.Contains(matched, file) { + matched = append(matched, file) + } + } + return matched +} + +// globsMatchFile reports whether any of the agent file glob patterns match +// the file. A "**/"-prefixed pattern also matches at the repository root, +// mirroring gitignore-style expectations. +func globsMatchFile(patterns []string, file string) bool { + for _, pattern := range patterns { matcher, err := glob.Compile(pattern, '/') if err != nil { continue } - var rootMatcher glob.Glob - if strings.HasPrefix(pattern, "**/") { - rootMatcher, _ = glob.Compile(strings.TrimPrefix(pattern, "**/"), '/') + if matcher.Match(file) { + return true } - for _, file := range changedFiles { - if (matcher.Match(file) || rootMatcher != nil && rootMatcher.Match(file)) && !slices.Contains(matched, file) { - matched = append(matched, file) + if strings.HasPrefix(pattern, "**/") { + if rootMatcher, err := glob.Compile(strings.TrimPrefix(pattern, "**/"), '/'); err == nil && rootMatcher.Match(file) { + return true } } } - return matched + return false } type reviewerAgentPrompt struct { From 349098120f4283f776febce1d7ca4a1f4c7164a9 Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 22 Jul 2026 21:35:33 -0400 Subject: [PATCH 2/2] fix: cap glob-coverage backstop assignment to a single owning agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #527 review: assigning an uncovered file to every matching selected agent widens each agent's scope, and coverage requires every agent to inspect-or-skip everything in its own scope — so a single file matching several agents' globs multiplied the obligation and let any one economizing reviewer downgrade coverage to incomplete_skipped. Assign each uncovered file to the first selected agent whose globs match instead. Claude-Session: https://claude.ai/code/session_01NfzotSGPeRHabbn66nNkEC --- internal/pipeline/pipeline.go | 17 +++++++++++------ internal/pipeline/pipeline_test.go | 10 +++++++--- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 13c4be9..1971d26 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -1318,14 +1318,18 @@ func ensureRequiredOnMatchAgents(selection llm.Selection, catalog agents.Catalog } // ensureSelectedGlobCoverage assigns every changed file that no selected -// agent's scope covers to each selected agent whose file globs match it. The -// orchestrator proposes assignments, but a resumed selection session can -// anchor on assignments from an earlier pass and ignore updated agent +// agent's scope covers to the first selected agent whose file globs match +// it. The orchestrator proposes assignments, but a resumed selection session +// can anchor on assignments from an earlier pass and ignore updated agent // definitions (#526); without this backstop such files land in the // unassigned coverage bucket, and hasIncompleteReviewerCoverage then blocks -// approval on every subsequent pass. Agents whose scope is already all -// changed files (no explicit Files/AllowedFiles) need no widening, and -// AllowedFiles is only extended when it is what defines the agent's scope. +// approval on every subsequent pass. Exactly one owner is chosen because a +// file in an agent's scope obligates that agent to inspect-or-skip it — +// fanning out to every glob match would multiply that obligation and let +// any one economizing reviewer downgrade coverage to incomplete_skipped. +// Agents whose scope is already all changed files (no explicit +// Files/AllowedFiles) need no widening, and AllowedFiles is only extended +// when it is what defines the agent's scope. func ensureSelectedGlobCoverage(selection llm.Selection, catalog agents.Catalog, changedFiles []string) llm.Selection { if len(selection.SelectedAgents) == 0 { return selection @@ -1356,6 +1360,7 @@ func ensureSelectedGlobCoverage(selection llm.Selection, catalog agents.Catalog, if len(selected.Files) > 0 { selected.Files = append(selected.Files, file) } + break } } return selection diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index a47c624..9584560 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -1371,10 +1371,12 @@ func TestEnsureSelectedGlobCoverageAssignsUncoveredMatchingFiles(t *testing.T) { {AgentID: "shared:docs", Files: []string{"README.md"}, AllowedFiles: []string{"README.md"}}, {AgentID: "shared:unrelated", Files: []string{"main.rs"}}, }} - changed := []string{"main.go", "README.md", "main.rs", "CHANGELOG.md", "api/testdata/fixture.json", "LICENSE"} + changed := []string{"main.go", "README.md", "main.rs", "CHANGELOG.md", "api/testdata/fixture.json", "README.rst", "LICENSE"} got := ensureSelectedGlobCoverage(selection, catalog, changed) + // Each uncovered file gains exactly one owner — the first selected agent + // whose globs match — so a single file cannot obligate several agents. wantGo := []string{"main.go", "CHANGELOG.md", "api/testdata/fixture.json"} if !reflect.DeepEqual(got.SelectedAgents[0].Files, wantGo) { t.Fatalf("go files = %#v, want %#v", got.SelectedAgents[0].Files, wantGo) @@ -1382,9 +1384,11 @@ func TestEnsureSelectedGlobCoverageAssignsUncoveredMatchingFiles(t *testing.T) { if len(got.SelectedAgents[0].AllowedFiles) != 0 { t.Fatalf("go allowed files = %#v, want empty so scope stays Files-driven", got.SelectedAgents[0].AllowedFiles) } - wantDocs := []string{"README.md", "CHANGELOG.md"} + // README.rst matches only the docs agent, whose scope is AllowedFiles- + // driven, so both lists widen together. + wantDocs := []string{"README.md", "README.rst"} if !reflect.DeepEqual(got.SelectedAgents[1].Files, wantDocs) || !reflect.DeepEqual(got.SelectedAgents[1].AllowedFiles, wantDocs) { - t.Fatalf("docs assignment = %#v, want files and allowed files %#v", got.SelectedAgents[1], wantDocs) + t.Fatalf("docs assignment = %#v, want CHANGELOG.md owned by shared:go only and README.rst here, files %#v", got.SelectedAgents[1], wantDocs) } if !reflect.DeepEqual(got.SelectedAgents[2].Files, []string{"main.rs"}) { t.Fatalf("unrelated files = %#v, want unchanged", got.SelectedAgents[2].Files)