fix: deterministically assign glob-matched files uncovered by selection#527
Conversation
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
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: e6d48e86d179
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 1 |
structure:repo-health (1 finding)
Major - internal/pipeline/pipeline.go:1343
ensureSelectedGlobCoverage can assign one uncovered file to every selected agent whose glob matches it (confirmed by TestEnsureSelectedGlobCoverageAssignsUncoveredMatchingFiles, where CHANGELOG.md lands in both shared:go and shared:docs). buildReviewerCoverage then folds each selected agent's widened Files/AllowedFiles into that agent's own
scope(reviewerAssignmentScope, pipeline.go:1979), and requires that agent to explicitly inspect-or-skip every file in its own scope or its status becomesincomplete_skipped(reviewplan.go coverageMissingFiles).hasIncompleteReviewerCoveragetreatsincomplete_skippedexactly likeincomplete_unassignedand downgrades approve to comment (reviewplan.go:339-352, 450). So this backstop trades one failure mode (#526's singleunassignedbucket entry) for a multiplied one: any changed file whose glob happens to match several selected agents (common when generic reviewers share broad globs, e.g.**/*.goplus a docs-file exception likeCHANGELOG.md) now obligates every one of those agents to individually account for it, even though the orchestrator only meant one of them to own it. A single non-code file matching several agents' globs can silently downgrade approvals from agents that never intended to review it and have no reason to mention it in inspected/skipped lists. Consider capping assignment to the single best-matching selected agent (e.g. first/most-specific glob owner) rather than fanning out to all matches, or excluding non-primary-scope agents from the coverage requirement for backstop-only assignments.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/prompts.go | unavailable | Sandbox did not permit running go build/go vet/go test in the checked-out repo; correctness was verified by static reading of pipeline.go, prompts.go, and pipeline_test.go rather than execution. |
| structure:repo-health | complete_broad | internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/prompts.go | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 17s | $2.40 | claude-sonnet-5 | cr 0.10.265
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 17s wall · 5m 33s compute |
| Cost | $2.40 |
| Tokens | 104 in / 24.2k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | 6 | 1.6k | 21.8k | 33.8k | $0.23 | 23s |
| go:implementation-tests | claude-sonnet-5 | 42 | 10.7k | 914.1k | 86.7k | $0.96 | 2m 35s |
| structure:repo-health | claude-sonnet-5 | 50 | 11.4k | 1.2M | 91.6k | $1.09 | 2m 25s |
| orchestrator-rollup | claude-sonnet-5 | 6 | 419 | 59.7k | 16.4k | $0.12 | 9s |
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
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 349098120f42
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 1 |
structure:repo-health (1 finding)
Minor - internal/pipeline/pipeline.go:1333
ensureSelectedGlobCoverage silently reassigns coverage ownership without leaving any trace of why a file ended up in an agent's Files/AllowedFiles. Compare capSelectionAgents (pipeline.go:1254), which calls opts.emitWarning when it truncates a selection, and ensureRequiredOnMatchAgents (pipeline.go:1292), which at least stamps a distinguishing Rationale ("required_on_match matched changed files") on agents it adds. Here, a file force-assigned to an already-selected agent is appended straight into that agent's existing Files/AllowedFiles with the agent's original orchestrator Rationale left untouched, so selection.json, the reviewer prompt, and the progress events (emitReviewerSelection) can't distinguish an orchestrator-chosen file from one this backstop injected. The whole point of this PR is to compensate for exactly the kind of anchoring bug (#526) whose diagnosis required deep run-artifact/session forensics per the PR description; a future recurrence of stale-session drift that this backstop silently papers over will be just as hard to detect from the artifacts, since coverage will look complete with no signal that the backstop fired. Emit a warning (opts.emitWarning, mirroring capSelectionAgents) or record a lightweight marker (e.g. a distinct rationale suffix or a coverage diagnostic) when ensureSelectedGlobCoverage actually assigns a file, so operators can tell 'model selected this' from 'backstop assigned this' when debugging future coverage anomalies.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/prompts.go | unavailable | unavailable |
| structure:repo-health | complete_broad | internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/prompts.go | unavailable | unavailable |
1 PR discussion threads considered. 1 summarized; 1 resolved.
Completed in 3m 19s | $2.01 | claude-sonnet-5 | cr 0.10.265
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 19s wall · 4m 28s compute |
| Cost | $2.01 |
| Tokens | 74 in / 21.8k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | 6 | 979 | 69.7k | 23.7k | $0.18 | 13s |
| go:implementation-tests | claude-sonnet-5 | 28 | 8.5k | 579.6k | 72.3k | $0.74 | 1m 41s |
| structure:repo-health | claude-sonnet-5 | 34 | 12.0k | 802.3k | 81.0k | $0.91 | 2m 23s |
| orchestrator-rollup | claude-sonnet-5 | 6 | 405 | 80.7k | 26.8k | $0.19 | 10s |
| // 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 { |
There was a problem hiding this comment.
ensureSelectedGlobCoverage silently reassigns coverage ownership without leaving any trace of why a file ended up in an agent's Files/AllowedFiles. Compare capSelectionAgents (pipeline.go:1254), which calls opts.emitWarning when it truncates a selection, and ensureRequiredOnMatchAgents (pipeline.go:1292), which at least stamps a distinguishing Rationale ("required_on_match matched changed files") on agents it adds. Here, a file force-assigned to an already-selected agent is appended straight into that agent's existing Files/AllowedFiles with the agent's original orchestrator Rationale left untouched, so selection.json, the reviewer prompt, and the progress events (emitReviewerSelection) can't distinguish an orchestrator-chosen file from one this backstop injected. The whole point of this PR is to compensate for exactly the kind of anchoring bug (#526) whose diagnosis required deep run-artifact/session forensics per the PR description; a future recurrence of stale-session drift that this backstop silently papers over will be just as hard to detect from the artifacts, since coverage will look complete with no signal that the backstop fired. Emit a warning (opts.emitWarning, mirroring capSelectionAgents) or record a lightweight marker (e.g. a distinct rationale suffix or a coverage diagnostic) when ensureSelectedGlobCoverage actually assigns a file, so operators can tell 'model selected this' from 'backstop assigned this' when debugging future coverage anomalies.
Reply inline to this comment.
2 participants
Summary
Fixes #526 — the selection stage, when resumed on the long-lived per-PR session, anchors on file assignments from earlier passes and ignores updated
.codereview/agents/definitions. Files its agents' globs now match land in theunassignedcoverage bucket, andhasIncompleteReviewerCoveragethen downgrades everyapproveverdict tocomment, permanently (each re-review resumes the same session — full evidence with run-artifact session/token forensics in the issue).Approach
Rather than changing session semantics, add a deterministic backstop in
runSelectionPhaseafterensureRequiredOnMatchAgentsandcapSelectionAgents:ensureSelectedGlobCoverageassigns every changed file outside all selected agents' scopes to each selected agent whosefile_globsmatch it.AllowedFilesis only extended when it is what defines the agent's scope (reviewerAssignmentScopeprefers it), and agents with no explicit files (full scope) are untouched.globsMatchFileis extracted fromrequiredOnMatchFiles(samegobwas/globcompile +**/root-anchor handling) for reuse.This guarantees the invariant the coverage gate implicitly assumes: a changed file that matches a selected agent's globs is never unassigned, regardless of model anchoring or sloppiness.
Testing
TestEnsureSelectedGlobCoverageAssignsUncoveredMatchingFiles— the Selection stage resumes per-PR session and anchors on stale file assignments when agent definitions change #526 shape: uncovered CHANGELOG/testdata files get assigned to matching selected agents (Files-scoped and AllowedFiles-scoped variants), non-matching files stay unassigned, unrelated agents untouched.TestEnsureSelectedGlobCoverageLeavesFullScopeAgentsAlone.go build ./...,go vet ./...clean; fullgo test ./...passes exceptTestDryRunResumesPinnedReviewRunByPinnedSHAs/TestDryRunWithPinnedReviewSHAsUsesCompareDiffAndPinnedFileRefs, which fail identically on unmodified main in this environment (git temp-file creation in the test fixture) — pre-existing, unrelated.Follow-up candidates (not in this PR)
review.noop_same_headmeans config-only changes can never trigger a re-review without an empty commit — worth a--rerun-style escape via re-request.https://claude.ai/code/session_01NfzotSGPeRHabbn66nNkEC