fix(review): Preserve deletion metadata in reconstructed diffs - #588
Conversation
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: cd60d159c7f9
Profile: pi-codex-reviewer-gh-incognito - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 1 |
| harness-engineering:repo-health | 0 |
architecture:solid-reviewer-agnostic (1 finding)
Major - internal/pipeline/pipeline.go:2671
U-L1, U-T1: A deletion-only diff produces an empty reviewable set, but this filter retains a broad selected agent (both assignment lists empty).
runReviewerthen invokes that agent with no changed files, whilellm.DecodeFindingsrequires at least one inspected or skipped changed file; the reviewer therefore has no valid response and is recorded as failed. Drop broad selections whenchangedFilesis empty (while retaining selection thread actions), and add an end-to-end deletion-only selection/reviewer test.
Reviewer Coverage
go:implementation-tests— complete (broad); skipped: none; constraints: Read-only review; I inspected the pinned diff and source/test context but could not execute tests.structure:repo-health— complete (broad); inspected 3 assigned files (5 inspected across reviewers):internal/gitprovider/github/rest.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Read-only review; tests were not executed.architecture:solid-reviewer-agnostic— complete (broad); inspected 2 assigned files (5 inspected across reviewers):internal/gitprovider/github/rest.go,internal/pipeline/pipeline.go; skipped: none; constraints: CR tools are read-only, so I inspected code and tests but did not execute the test suite.harness-engineering:repo-health— complete (broad); inspected 3 assigned files (5 inspected across reviewers):internal/pipeline/diff_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Read-only review; I inspected the pinned diff and relevant pipeline paths but did not execute tests.
Inspected files (5)
internal/gitprovider/github/rest.gointernal/gitprovider/github/rest_test.gointernal/pipeline/diff_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 13s | $0.11 | openai-codex/gpt-5.6-terra | cr 0.10.302
| Field | Value |
|---|---|
| Model | openai-codex/gpt-5.6-terra |
| Reviewers | go:implementation-tests, structure:repo-health, architecture:solid-reviewer-agnostic, harness-engineering:repo-health |
| Engine | pi_rpc · openai-codex/gpt-5.6-terra |
| Reviewed by | cr · monit-reviewer |
| Duration | 2m 13s wall · 5m 31s compute |
| Cost | $0.11 |
| Tokens | 19.2k in / 3.5k out |
Per-workstream usage
orchestrator-selection— openai-codex/gpt-5.6-terra- In: 5.8k
- Out: 404
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.02
- Duration: 9s
go:implementation-tests— openai-codex/gpt-5.6-terra- In: 1.6k
- Out: 1.1k
- Cache read: 24.1k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 16s
structure:repo-health— openai-codex/gpt-5.6-terra- In: 1.8k
- Out: 862
- Cache read: 33.3k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 13s
architecture:solid-reviewer-agnostic— openai-codex/gpt-5.6-terra- In: 2.6k
- Out: 512
- Cache read: 54.8k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 48s
harness-engineering:repo-health— openai-codex/gpt-5.6-terra- In: 5.5k
- Out: 457
- Cache read: 21.0k
- Cache create: 0
- Cost: $0.02
- Duration: 59s
orchestrator-rollup— openai-codex/gpt-5.6-terra- In: 1.9k
- Out: 168
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.01
- Duration: 5s
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 182308867d52
Profile: pi-codex-reviewer-gh-incognito - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 1 |
| architecture:solid-reviewer-agnostic | 0 |
| harness-engineering:repo-health | 0 |
structure:repo-health (1 finding)
Minor - internal/gitprovider/github/rest.go:179
The reconstructed diff invents
deleted file mode 100644for every patchless removal. Removed executable files and symlinks have different modes, so this emits false metadata into the durable diff/dossier boundary. The parser only needs the/dev/nullheader to classify a deletion; omit the mode line unless the provider can supply the actual mode.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: Read-only review; I inspected the changed implementation and tests but did not execute the test suite.structure:repo-health— complete (constrained); inspected 3 assigned files (5 inspected across reviewers):internal/gitprovider/github/rest.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Review scoped to the three assigned changed files; related changed tests outside the assignment were not inspected.architecture:solid-reviewer-agnostic— complete (constrained); inspected 2 assigned files (5 inspected across reviewers):internal/gitprovider/github/rest.go,internal/pipeline/pipeline.go; skipped: none; constraints: CR tools are read-only, so I could not execute the stated test suite. Review limited to the two assigned implementation files; related test changes were visible in the pinned diff but not separately inspected.harness-engineering:repo-health— complete (constrained); inspected 3 assigned files (5 inspected across reviewers):internal/pipeline/diff_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Review scope was limited to the three assigned pipeline files; GitHub reconstruction changes were visible in the pinned diff but not independently inspected.
Inspected files (5)
internal/gitprovider/github/rest.gointernal/gitprovider/github/rest_test.gointernal/pipeline/diff_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 22s | $0.07 | openai-codex/gpt-5.6-terra | cr 0.10.302
| Field | Value |
|---|---|
| Model | openai-codex/gpt-5.6-terra |
| Reviewers | go:implementation-tests, structure:repo-health, architecture:solid-reviewer-agnostic, harness-engineering:repo-health |
| Engine | pi_rpc · openai-codex/gpt-5.6-terra |
| Reviewed by | cr · monit-reviewer |
| Duration | 1m 22s wall · 3m 28s compute |
| Cost | $0.07 |
| Tokens | 16.8k in / 1.5k out |
Per-workstream usage
go:implementation-tests— openai-codex/gpt-5.6-terra- In: 5.5k
- Out: 221
- Cache read: 42.5k
- Cache create: 0
- Cost: $0.02
- Duration: 1m 06s
structure:repo-health— openai-codex/gpt-5.6-terra- In: 2.3k
- Out: 644
- Cache read: 22.0k
- Cache create: 0
- Cost: $0.02
- Duration: 48s
architecture:solid-reviewer-agnostic— openai-codex/gpt-5.6-terra- In: 830
- Out: 378
- Cache read: 29.2k
- Cache create: 0
- Cost: $0.01
- Duration: 53s
harness-engineering:repo-health— openai-codex/gpt-5.6-terra- In: 6.5k
- Out: 160
- Cache read: 18.9k
- Cache create: 0
- Cost: $0.02
- Duration: 35s
orchestrator-rollup— openai-codex/gpt-5.6-terra- In: 1.7k
- Out: 66
- Cache read: 2.8k
- Cache create: 0
- Cost: $0.00
- Duration: 3s
zzwong
changed the title
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 820107a0d6a2
Profile: claude-reviewer-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 2 |
| structure:repo-health | 1 |
| architecture:solid-reviewer-agnostic | 3 |
go:implementation-tests (2 findings)
Major - internal/pipeline/pipeline.go:1276
Dropping
changedFiles(patches)in favor ofstringSet(reviewerFiles)narrows the selection decode contract for renames, and nothing pins the old behavior.
changedFilesdeliberately seeded the set with bothpatch.Pathandpatch.OldPath, so a selection that named a rename's previous path validated.reviewablePatchPathsis built frompatchPaths, which appends onlypatch.Path(pipeline.go:3281-3288).DecodeSelectiontreats an unknown file as a hard error (llm: selected file %q is not in changed files, internal/llm/contracts.go:185-192), so for a PR containing a rename the model can now fail the entire selection phase by citing therename frompath that is plainly visible in the diff it was given. Previously that same output decoded fine.The telling part is that removing the helper broke no test: there is no rename case anywhere in the selection decode path, which is exactly the kind of boundary mapping this reviewer expects to be pinned.
Fix: keep old paths in the validation set while leaving the prompt's
changed_filesas the reviewable head paths — e.g. build the decode set asstringSet(append(reviewerFiles, reviewableOldPaths(req.ParsedDiff.Patches)...))where the helper collects non-emptypatch.OldPathfor non-deleted patches — and add a selection-phase test with a renamed file whose selection cites the old path, asserting decode succeeds and the resulting assignment is normalized to the new path.
Minor - internal/gitprovider/github/rest.go:189
deleted file mode 100644is fabricated metadata that the parser does not need, and the new tests now cement it.
parseUnifiedDiffsetsDeletedfrom either signal:+++ /dev/null(internal/pipeline/diff.go:82-83) or adeleted file modeprefix (diff.go:85-86). The/dev/nullheader this change already emits is therefore sufficient to classify the removal. The mode line adds nothing the parser consumes, while writing a concretely wrong value into the durable reconstructed diff for executable files (100755) and symlinks (120000) — thepullFileResponsepayload carries no mode, so the code cannot know it.This also weakens the two new tests:
TestReconstructUnifiedDiffasserts the literaldeleted file mode 100644string, andTestParseUnifiedDiffPreservesPatchlessRemovedStatusfeeds a fixture containing both the mode line and+++ /dev/null, so neither test shows which signal actually drives classification.Fix: drop the mode line and emit only
--- a/<old>/+++ /dev/null; update the rest_test expectation accordingly, and drop the mode line from the diff_test fixture so it proves the/dev/nullheader alone yieldsDeleted. (Incidentally,fmt.Fprintfwith no verbs here should beb.WriteString, matching the+++ /dev/nullline directly below.)
structure:repo-health (1 finding)
Minor - internal/pipeline/pipeline.go:1308
The deletion-only skip is load-bearing behavior hidden in an early return of a helper named for a different job. In
runSelectionPhase,DecodeSelectionalready hard-rejects any selected/allowed file outsidereviewerFiles(contracts.go:185-191), so by the timefilterSelectedReviewerAssignmentsruns, its per-file filtering can never drop anything. Its only observable effect on this path is thelen(changedFiles) == 0branch that clears every selected reviewer — i.e. the mechanism that makes a deletion-only diff proceed straight to rollup. A future reader who notices the filtering is redundant with decode validation can delete the call and silently re-enable full reviewer runs (and reviewer prompts with zero reviewable files) on deletion-only diffs; the regression would only surface via the dry-run test, not the helper's name or contract.Fix: make the intent explicit at the call site, e.g.
if len(reviewerFiles) == 0 { selection.SelectedAgents = nil }with the reason in a comment, and keepfilterSelectedReviewerAssignmentsas a pure per-assignment filter for the paths where inputs are not decode-validated (reused cohorts,runReviewer). Same invariant, but it is stated where it takes effect instead of inferred from an early return.
architecture:solid-reviewer-agnostic (3 findings)
Major - internal/pipeline/pipeline.go:2063
U-I1: this line narrows one set that is doing two jobs — the reviewer's obligation set and the reviewer's acceptance set — so removing deletions from obligations also silently makes deleted paths illegal to talk about.
changedFilePathsflows intoassignmentScope(2065) and then intollm.DecodeFindings(... ChangedFiles: stringSet(assignmentScope))(2116).DecodeFindingshard-rejects the entire payload when anyfile_pathis outside that set (internal/llm/contracts.go:299) anddecodeCoverageFilesdoes the same forinspected_files/skipped_files(contracts.go:341-354). Before this diffpatchPathsincluded deleted paths, so a reviewer could report "deleting X breaks Y" on X, or list a deleted path inskipped_fileswith a reason. Now either response fails the reviewer task and discards every other finding it produced — the opposite of the intent of c5f35b9/fb9759a.That the acceptance narrowing is unintentional is visible downstream: internal/reviewplan/reviewplan.go:727 explicitly supports findings on deleted files by degrading them to non-inline anchoring. This diff makes that branch unreachable for reviewer-authored findings.
Suggested fix: keep the two concepts separate rather than collapsing them. Use
reviewablePatchPaths(...)for the assignment/prompt/coverage obligation (as done at 2065 and inbuildReviewerCoverage), but pass the fullpatchPaths(parsed.Patches)as the decode gate at 2116 — permitted subject matter is a superset of assigned work. The same split applies to the selection decode at 1276, which additionally loses rename source paths: the removedchangedFiles()helper includedOldPath, andpatchPathsdoes not, so an orchestrator naming a rename's old path is now a terminal selection decode error rather than an accepted assignment.
Minor - internal/pipeline/pipeline.go:2064
U-L1:
filterSelectedReviewerAssignment(singular) andfilterSelectedReviewerAssignments(plural, 2671) differ by one character and have opposite semantics for the same edge case. When filtering empties an agent's explicit assignment, the plural drops the reviewer; this singular call keeps it, andreviewerAssignmentScope(2783) then falls back tocopySortedStrings(changedFiles)— silently widening a reviewer that was scoped to two paths into a whole-diff reviewer. It also emptiesselected.AllowedFiles, which is passed as the workbench file allowlist at 2079, widening the sandbox the same way.As written the branch looks unreachable: both routes into
runRevieweralready narrow assignments against the reviewable set (selection phase at 1310,rebaseReviewerCohortat 1719-1726 plus itslen(candidate.files) == 0drop), andparsedis the sameprepared.parsed. So this is a guard that cannot fire today but whose only possible behavior is the wrong one, and it is the one filter in the change with no direct test.Suggested fix: either drop the call and rely on the upstream narrowing (keeping 2063 for the prompt/scope), or, if it stays as a defensive guard, make it agree with its sibling — return a
ReviewerFailure/skip for an agent whose explicit assignment filtered to empty rather than lettingreviewerAssignmentScopefall through to the full diff. Distinguishable names would help either way.
Minor - internal/pipeline/pipeline.go:783
U-S1/U-D2: "the reviewable changed files" is now a real domain concept with no single owner. It is recomputed from raw patches at three independent call sites — here, 1258, and 2063 — while
preparedSelectionContext.changedFiles(populated at 1179 frompatchPaths) keeps the unqualified name for the other set (all paths, deletions included) and is still handed to 513, 692, 864, and 1056. Two same-named notions in one file is exactly the divergence that produced the bug this PR fixes, and the next consumer has nothing to tell it which set it wants.A related symptom is 1259:
selectionPromptInputFromArtifactssetsChangedFilesfrom the durable dossier metadata (internal/pipeline/prompts.go:359) and the caller then overwrites it. A reader of the builder will conclude the dossier metadata is the source of truth; the override is easy to drop in the next refactor of that function, and there is no test that would notice.Suggested fix: compute it once where
changedFilesis populated — addreviewableFiles []stringtopreparedSelectionContextat 393/1179 and thread it to 783 and intoselectionPhaseRequest— and renamechangedFilesto something that says it includes deletions (allChangedFiles). Then pass the reviewable set intoselectionPromptInputFromArtifacts/selectionPromptInputFromThreadContextas a parameter instead of overwriting the field after the fact, soselectionPromptInput.ChangedFileshas one owner.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: Reachability of the runReviewer assignment-filter path was assessed from callers in internal/pipeline/pipeline.go only. Static review only; I did not executego test ./internal/gitprovider/github ./internal/pipelinein this session, so the PR's reported test results are taken as stated.structure:repo-health— complete (constrained); skipped: none; constraints: The hardcodeddeleted file mode 100644in reconstructUnifiedDiff is already an open unresolved inline thread on this PR (rest.go:179); not re-reported here to avoid duplicating live review discussion.go test ./internal/pipelinepasses andgo test ./internal/gitprovider/github -run ReconstructUnifiedDiffpasses. The full github package run fails only in TestProductionImportsStayInAdapterLayer because this workbench path contains a space; unrelated to the diff.architecture:solid-reviewer-agnostic— complete (constrained); skipped: none; constraints: The hardcodeddeleted file mode 100644in internal/gitprovider/github/rest.go is already raised in an unresolved inline thread on this PR, so it is not re-filed here. Verified: go build ./... clean, go vet clean on both packages, go test ./internal/pipeline ok. internal/gitprovider/github fails only TestProductionImportsStayInAdapterLayer, which mis-parses the workbench path (spaces/%3A) ingo listoutput; not diff-caused, treated as environment noise.
Inspected files (5)
internal/gitprovider/github/rest.gointernal/gitprovider/github/rest_test.gointernal/pipeline/diff_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 9m 31s | ~$4.89 (est.) | claude-opus-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-opus-5 |
| Reviewers | go:implementation-tests, structure:repo-health, architecture:solid-reviewer-agnostic |
| Engine | claude_cli · claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 9m 31s wall · 17m 29s compute |
| Cost | ~$4.89 (est.) |
| Pricing basis | anthropic-public-2026-09-02 |
| Tokens | 144 in / 47.4k out |
Per-workstream usage
orchestrator-selection— claude-opus-5- In: 6
- Out: 2.3k
- Cache read: 38.5k
- Cache create: 28.2k
- Cost: ~$0.36 (est.)
- Duration: 33s
go:implementation-tests— claude-opus-5- In: 40
- Out: 10.6k
- Cache read: 706.7k
- Cache create: 53.3k
- Cost: ~$1.15 (est.)
- Duration: 3m 38s
structure:repo-health— claude-opus-5- In: 38
- Out: 10.8k
- Cache read: 677.3k
- Cache create: 55.1k
- Cost: ~$1.16 (est.)
- Duration: 4m 52s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 54
- Out: 21.9k
- Cache read: 1.3M
- Cache create: 78.9k
- Cost: ~$1.99 (est.)
- Duration: 7m 17s
orchestrator-rollup— claude-opus-5- In: 6
- Out: 1.8k
- Cache read: 132.3k
- Cache create: 12.0k
- Cost: ~$0.23 (est.)
- Duration: 1m 06s
| return llm.DecodeSelection(data, llm.SelectionOptions{ | ||
| KnownAgents: knownAgents(req.Catalog), | ||
| ChangedFiles: changedFiles(req.ParsedDiff.Patches), | ||
| ChangedFiles: stringSet(reviewerFiles), |
There was a problem hiding this comment.
Dropping changedFiles(patches) in favor of stringSet(reviewerFiles) narrows the selection decode contract for renames, and nothing pins the old behavior.
changedFiles deliberately seeded the set with both patch.Path and patch.OldPath, so a selection that named a rename's previous path validated. reviewablePatchPaths is built from patchPaths, which appends only patch.Path (pipeline.go:3281-3288). DecodeSelection treats an unknown file as a hard error (llm: selected file %q is not in changed files, internal/llm/contracts.go:185-192), so for a PR containing a rename the model can now fail the entire selection phase by citing the rename from path that is plainly visible in the diff it was given. Previously that same output decoded fine.
The telling part is that removing the helper broke no test: there is no rename case anywhere in the selection decode path, which is exactly the kind of boundary mapping this reviewer expects to be pinned.
Fix: keep old paths in the validation set while leaving the prompt's changed_files as the reviewable head paths — e.g. build the decode set as stringSet(append(reviewerFiles, reviewableOldPaths(req.ParsedDiff.Patches)...)) where the helper collects non-empty patch.OldPath for non-deleted patches — and add a selection-phase test with a renamed file whose selection cites the old path, asserting decode succeeds and the resulting assignment is normalized to the new path.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in 8b095c6.
Added renamedPatchOldPaths beside deletedPatchPaths/reviewablePatchPaths, collecting non-empty OldPath for non-deleted patches where it differs from Path. runSelectionPhase builds selectableFiles = reviewerFiles + those, and only the DecodeSelection gate reads it — the prompt's changed_files still gets reviewerFiles, so obligations are unchanged.
TestSelectionOnlyAcceptsRenameSourcePath pins it. Reverting the gate to stringSet(reviewerFiles) fails with llm: selected file "main.go" is not in changed files.
One correction to the suggested fix: decode does not normalize the assignment to the head path. What actually happens is the assignment filter drops the rename source and glob coverage backfills the head path, giving ["other.go", "renamed.go"]. The test asserts that rather than adding normalization the code does not do.
There was a problem hiding this comment.
Confirmed, and your correction is right: I asserted decode normalizes a rename source to the head path, and it does not. The assignment filter drops the source and glob coverage backfills renamed.go, so ["other.go", "renamed.go"] is the accurate expectation — pinning observed behavior beats pinning normalization the code never performed.
The fix matches the split I was after: selectableFiles widens only the DecodeSelection gate while the prompt's changed_files stays reviewerFiles, so acceptance is a superset of obligations rather than the two collapsing again. Restricting renamedPatchOldPaths to non-deleted patches where OldPath differs from Path keeps deletions out of the selectable set, which was the point of the PR. The revert check (llm: selected file "main.go" is not in changed files) shows TestSelectionOnlyAcceptsRenameSourcePath actually holds the gate.
Resolving.
Summary:
Replacing changedFiles(patches) with stringSet(reviewerFiles) at the selection decode gate silently dropped rename source paths: changedFiles seeded both patch.Path and patch.OldPath, while reviewablePatchPaths/patchPaths emit only patch.Path. Because DecodeSelection treats an unknown file as a hard error (internal/llm/contracts.go:185-192), a selection citing a rename's pre-rename path — visible in the diff the model was given — would fail the entire selection phase, and no test covered renames in the decode path. Fixed in 8b095c6: renamedPatchOldPaths (beside deletedPatchPaths/reviewablePatchPaths) collects non-empty OldPath for non-deleted patches where it differs from Path, and runSelectionPhase builds selectableFiles = reviewerFiles + those. Only the DecodeSelection gate reads selectableFiles; the prompt's changed_files still receives reviewerFiles, so acceptance is widened without changing reviewer obligations, and deleted paths stay excluded. TestSelectionOnlyAcceptsRenameSourcePath pins it — reverting the gate to stringSet(reviewerFiles) fails with llm: selected file "main.go" is not in changed files. Note for future readers: decode does not normalize a rename source to the head path; the assignment filter drops the source and glob coverage backfills the head path, yielding ["other.go", "renamed.go"].
|
|
||
| cohortScope := ledger.ReviewerCohortScope{PRKey: prepared.prKey, Profile: req.ProfileName, PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity)} | ||
| selection, reviewerResumeIDs, reusedCohort, err := loadReviewerCohort(ctx, opts, req, cohortScope, prepared.catalog, prepared.changedFiles, maxAgents) | ||
| selection, reviewerResumeIDs, reusedCohort, err := loadReviewerCohort(ctx, opts, req, cohortScope, prepared.catalog, reviewablePatchPaths(prepared.parsed.Patches), maxAgents) |
There was a problem hiding this comment.
U-S1/U-D2: "the reviewable changed files" is now a real domain concept with no single owner. It is recomputed from raw patches at three independent call sites — here, 1258, and 2063 — while preparedSelectionContext.changedFiles (populated at 1179 from patchPaths) keeps the unqualified name for the other set (all paths, deletions included) and is still handed to 513, 692, 864, and 1056. Two same-named notions in one file is exactly the divergence that produced the bug this PR fixes, and the next consumer has nothing to tell it which set it wants.
A related symptom is 1259: selectionPromptInputFromArtifacts sets ChangedFiles from the durable dossier metadata (internal/pipeline/prompts.go:359) and the caller then overwrites it. A reader of the builder will conclude the dossier metadata is the source of truth; the override is easy to drop in the next refactor of that function, and there is no test that would notice.
Suggested fix: compute it once where changedFiles is populated — add reviewableFiles []string to preparedSelectionContext at 393/1179 and thread it to 783 and into selectionPhaseRequest — and rename changedFiles to something that says it includes deletions (allChangedFiles). Then pass the reviewable set into selectionPromptInputFromArtifacts/selectionPromptInputFromThreadContext as a parameter instead of overwriting the field after the fact, so selectionPromptInput.ChangedFiles has one owner.
Reply inline to this comment.
There was a problem hiding this comment.
Declining.
This is the largest of the three restructure requests: a new field on preparedSelectionContext, a rename of changedFiles across the 513/692/864/1056 call sites, and re-threading selectionPromptInputFromArtifacts to take the reviewable set as a parameter.
The underlying observation is fair — two same-named notions in one file is how the original bug happened. But that is a design change, and doing it inside a fix PR means the fix and the redesign land in one diff that has to be reviewed as both.
Keeping this PR to the defect. The naming belongs in its own change where it can be judged on its own terms.
There was a problem hiding this comment.
Accepting the decline — the scope argument holds. A new field on preparedSelectionContext, a rename across the 513/692/864/1056 call sites, and re-threading selectionPromptInputFromArtifacts is a design change, and bundling it with the defect fix would force reviewers to judge two different things in one diff. This was a structural observation, not a defect blocking the fix.
Recording it here so the next change in this area has the context: the reviewable set is still recomputed at 783, 1258, and 2063, changedFiles still carries the unqualified name for the all-paths set, and selectionPromptInputFromArtifacts still sets ChangedFiles from dossier metadata only for the caller to overwrite it at 1259 — the override is the piece most likely to be dropped in a later refactor of that builder, and no test would catch it.
Resolving.
Summary:
Raised that "reviewable changed files" is now a durable domain concept with no single owner: it is recomputed from raw patches at three independent call sites (pipeline.go:783, 1258, 2063) while preparedSelectionContext.changedFiles (populated at 1179 from patchPaths) keeps the unqualified name for the all-paths set fed to 513, 692, 864, and 1056. Two same-named notions in one file is the divergence that produced the bug this PR fixes. A related symptom: selectionPromptInputFromArtifacts sets ChangedFiles from durable dossier metadata (prompts.go:359) and the caller overwrites it at 1259, so the builder reads as the source of truth while the override is untested and easy to drop. Author declined for scope: the proposed remedy — add reviewableFiles to preparedSelectionContext at 393/1179, thread it through selectionPhaseRequest, rename changedFiles to allChangedFiles across its call sites, and pass the reviewable set into the prompt builders as a parameter instead of overwriting the field — is a design change that would land alongside the defect fix in one diff. Deferred to a dedicated follow-up change; no defect is outstanding in this PR.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 8b095c69cd67
Profile: claude-reviewer-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 2 |
architecture:solid-reviewer-agnostic (2 findings)
Major - internal/pipeline/pipeline.go:2120
U-I1: the obligation/acceptance split landed here is right in principle, but the acceptance set was widened past "assignment plus deletions" all the way to every path in the PR, which drops the only enforcement of a reviewer's assignment at the output boundary.
ChangedFilesno longer depends onselectedat all, so a reviewer scoped to two files can now file findings on — and claiminspected_filesfor — any file in the diff, including files another reviewer owns and files outside theAllowedFileslist that also narrows its workspace (2081 → workbench.go:445-458).Three surfaces now disagree about the same rule: the prompt advertises
changed_files=reviewerAssignmentScope(selected, changedFiles)and instructs "file_path must be one of changed_files" (prompts.go:30, 719);decodeCoverageFilesstill rejects with "outside the allowed reviewer assignment" (internal/llm/contracts.go:343) which is no longer what is enforced; and this gate accepts the whole diff. Nothing pinned the narrower behavior, so the invariant left without a failing test.Suggested fix: widen by exactly what the fix needs instead of replacing the set —
ChangedFiles: stringSet(append(reviewerAssignmentScope(selected, changedFilePaths), mentionableExtraPaths(parsed.Patches)...)), where the extras are the paths a reviewer may cite but cannot be assigned (deleted paths, and see the rename note at 2637). That keepsreviewplan.go:727's deleted-file degradation reachable — the point of the accepted fix — without silently making every reviewer global.Defensible either way on the end state, but not on the disagreement: if global acceptance is intended, then say so where the model reads it — pass the full set as the prompt's
changed_filesand reword contracts.go:343 — rather than leaving the prompt and the error text describing a boundary the code stopped checking.
Major - internal/pipeline/pipeline.go:2637
U-S1/U-I1: this helper answers "which paths may a model cite that it cannot be assigned", but only the selection gate reads it (1261), so the two decode gates in this diff are now widened in opposite, non-overlapping directions: selection accepts rename sources but not deletions, findings accepts deletions but not rename sources (2120 uses
patchPaths, which emitspatch.Pathonly). The rename argument that carried this helper applies verbatim to reviewers —rename from <old>is in the diff they read and "renamed: new (from old)" is in the dossier change map — and the consequence is the same hard rejection of the entire payload via internal/llm/contracts.go:299, i.e. an isolated reviewer failure that discards every other finding it produced.Downstream already supports it, which is the same evidence that settled the deletion case:
reviewplan.go:751-757matchesfile.OldPath == pathand then normalizesanchored.FilePath = diffFile.Path(722-726), so a finding anchored on a pre-rename path is designed to land on the head path. Only the reviewer gate rejects it, and no test covers a reviewer citing a rename source.Suggested fix: make this the single owner of the concept rather than a selection-local widening — add a sibling
mentionablePatchPaths(patches) = patchPaths(patches) + renamedPatchOldPaths(patches)beside it, haveselectableFiles(1261) bereviewerFiles+ those extras, and have the reviewer gate at 2120 union the same extras onto the assignment scope. One helper, two callers, and the gates stop drifting apart the next time one of them is touched.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: I did not re-raise the declined singular/pluralfilterSelectedReviewerAssignmentthread; the unreachability argument holds at this head. I did not re-raise the two settled threads (fabricateddeleted file modeline, rename-source decode narrowing) — both are fixed at 8b095c6 and verified in the diff. Static review only; I did not execute the package tests in this session, so the PR's reported results are taken as stated.structure:repo-health— complete (constrained); skipped: none; constraints: Could not run the pipeline test suite in this workbench:go test ./internal/pipelinefails first on a cgo module-cache path containing a space, then on git fixture setup (fatal: not in a git directory) under CGO_ENABLED=0. Both are environment issues, not diff defects; review of the new commi... Three concerns from the previous round (pipeline.go:783 ownerless reviewable-files concept, pipeline.go:1311 deletion-only skip hidden in a filter helper, pipeline.go:2067 singular/plural filter divergence) are recorded as open disagreements the author declined; not re-reported here.architecture:solid-reviewer-agnostic— complete (constrained); skipped: none; constraints: CGO_CFLAGS/GOCACHE leaked from another reviewer's sandbox (path with a space) broke the default toolchain; re-ran with CGO_ENABLED=0: go build ./... and go vet clean, go test ./internal/pipeline ok (65s). Re-review of 8b095c6. Settled threads (fabricated mode line, deletion split at the findings gate, rename sources at the selection gate) and the two author declines (set ownership at 783, filter twin at 2067) are not re-filed. internal/gitprovider/github fails only TestProductionImportsStayInAdapterLayer, which splitsgo listoutput on whitespace and chokes on the workbench path. Not diff-caused; treated as environment noise.
Inspected files (5)
internal/gitprovider/github/rest.gointernal/gitprovider/github/rest_test.gointernal/pipeline/diff_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
7 PR discussion threads considered. 7 summarized; 5 resolved.
Completed in 11m 01s | ~$3.99 (est.) | claude-opus-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-opus-5 |
| Reviewers | go:implementation-tests, structure:repo-health, architecture:solid-reviewer-agnostic |
| Engine | claude_cli · claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 11m 01s wall · 12m 17s compute |
| Cost | ~$3.99 (est.) |
| Pricing basis | anthropic-public-2026-09-02 |
| Tokens | 100 in / 34.1k out |
Per-workstream usage
go:implementation-tests— claude-opus-5- In: 26
- Out: 6.5k
- Cache read: 839.3k
- Cache create: 21.7k
- Cost: ~$0.80 (est.)
- Duration: 2m 12s
structure:repo-health— claude-opus-5- In: 20
- Out: 6.4k
- Cache read: 694.5k
- Cache create: 18.2k
- Cost: ~$0.69 (est.)
- Duration: 2m 42s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 48
- Out: 19.1k
- Cache read: 2.5M
- Cache create: 47.3k
- Cost: ~$2.22 (est.)
- Duration: 6m 46s
orchestrator-rollup— claude-opus-5- In: 6
- Out: 2.1k
- Cache read: 265.2k
- Cache create: 9.6k
- Cost: ~$0.28 (est.)
- Duration: 37s
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 8be2199afd18
Profile: claude-reviewer-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 1 |
| architecture:solid-reviewer-agnostic | 1 |
structure:repo-health (1 finding)
Minor - internal/pipeline/pipeline.go:2119
The
citableFilesgate is the right shape forfile_path, butllm.FindingsOptions.ChangedFilesis also the gate forinspected_files/skipped_files(contracts.go:264), and coverage accounting no longer has the scope guarantee it documents. Before this PR the gate was exactlyassignmentScope, soresult.InspectedFiles ⊆ scopeheld by construction; now it is scope +mentionableExtraPaths, andbuildReviewerCoveragecopies inspected files through without intersecting scope —entry.InspectedFiles = filterReviewableFiles(copySortedStrings(result.InspectedFiles))at pipeline.go:2773, one line aboveentry.SkippedFiles = sortedIntersection(result.SkippedFiles, scope). The comment right above it (2770-2772) still asserts a reviewer 'doesn't emit an inspected file outside its scope', which is now false.Impact is durable-output accuracy rather than correctness: a reviewer that lists a deleted path or a rename source in
inspected_filesgets it written into the coverage row, and reviewplan renders that row as 'inspected N assigned files' (internal/reviewplan/summary.go:325-331) — labelling as assigned a path the reviewer was deliberately never assigned, and inflating the cross-reviewer inspected union at summary.go:559-566. Approval logic is unaffected becausecoverageMissingFilesonly subtracts fromscope.Fix: mirror the
SkippedFilesline —entry.InspectedFiles = sortedIntersection(filterReviewableFiles(result.InspectedFiles), scope)— so findings may cite unassignable paths while coverage rows stay an obligation ledger, and the 2770-2772 comment becomes true again. If instead the extras are meant to show up in coverage, update that comment and the 'assigned' wording in summary.go so the two surfaces agree.
architecture:solid-reviewer-agnostic (1 finding)
Minor - internal/pipeline/pipeline.go:2068
U-I1, close call: the enforced acceptance set is now right — assignment scope plus unassignable paths — but nothing tells the reviewer the extras are citable, so the capability the last two commits restored is only reachable when a model disregards its stated contract.
buildReviewerPromptpasses onlyreviewerAssignmentScope(selected, changedFiles)tofindingsOutputContract(prompts.go:30, 719), which advertises it asallowed_values.changed_filesand instructs "file_path must be one of changed_files" (prompts.go:705). A compliant reviewer that notices "deleting X breaks Y" will therefore self-censor the finding, because X is absent from the only allowed set it was shown — even though the gate at 2119 would now accept it. Both settled threads justified widening with "the path is visible in the diff the model reads", which is true, but the model is simultaneously told those paths are not allowed values.Secondary symptom at this line: the assignment scope is now derived twice from the same inputs — here, and again inside
buildReviewerPrompt— so the advertised set and the enforced set are independent computations that can drift, which is the same recomputation hazard recorded in the 783 thread.Suggested fix (small, no signature churn beyond one parameter): pass the citable set into
buildReviewerPromptand havefindingsOutputContractadvertise the two roles distinctly — keepchanged_filesas the assignment scope soinspected_files/skipped_filesobligations stay narrow, and add e.g.also_citable_files(thementionableExtraPathsresult) with a one-line instruction thatfile_pathmay be one of either. That makes the prompt describe what the gate enforces and removes the duplicate derivation.Defensible either way, and not blocking: if the intent is a deliberately tolerant gate — accept such a finding when a model volunteers it, without inviting commentary on deleted code — then the narrow prompt is a reasonable choice and a sentence saying so at this line would settle it for the next reader. What would change my verdict is evidence that the narrow prompt is intentional rather than an oversight; the commit messages and thread replies argue the capability matters, which is why I read the silence as unfinished rather than chosen.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: Prior-round findings are all settled at 8be2199 and were not re-raised: the fabricateddeleted file modeline is gone, the selection gate accepts rename sources, and the findings gate is back inside the assignment scope (citableFiles=reviewerAssignmentScope+mentionableExtraPaths), pin... Static review only; I did not execute the package tests in this session, so the PR's reported results are taken as stated. Two deferred structural observations (the ownerless reviewable-set concept recomputed at 783/1258/2063, and the singular/plural filterSelectedReviewerAssignment naming collision) were declined for scope with agreed reasoning and are not re-raised.structure:repo-health— complete (constrained); skipped: none; constraints: Structural observations already recorded and accepted as deferred (pipeline.go:783 ownerless reviewable-files concept; pipeline.go:1310 deletion-only skip inside filterSelectedReviewerAssignments; pipeline.go:2066 singular/plural filter divergence) are not re-reported. Verified with CGO_ENABLED=0 and an out-of-workbench GOTMPDIR/GOCACHE:go test ./internal/pipeline -run 'OutsideAssignment\|RenameSourcePath\|DeletedPath\|BuildReviewerCoverage'passes. The default environment here fails for unrelated reasons (GOTMPDIR points at another reviewer's scratch dir; cgo ...architecture:solid-reviewer-agnostic— complete (constrained); skipped: none; constraints: Confirmed the two new gate tests pass by name: TestDryRunReviewerFindingOnRenameSourcePathIsDecoded and TestDryRunReviewerFindingOutsideAssignmentIsRejected, plus the deleted-path and selection rename cases. GOTMPDIR/GOCACHE/CGO_CFLAGS leaked from another reviewer's sandbox and pointed at a deleted directory. Re-ran with local TMPDIR/GOTMPDIR/GOCACHE and CGO_ENABLED=0: go build ./... ok, go vet clean, go test ./internal/pipeline ok (65s). Re-review of 8be2199 on top of 8b095c6. Threads already settled or declined by the author (mode line, both decode gates, set ownership at 783, the filter twin at 2067, the deletion-only early return) are not re-filed. internal/gitprovider/github fails only TestProductionImportsStayInAdapterLayer, which splitsgo listoutput on whitespace and chokes on the workbench path containing a space. Not diff-caused.
Inspected files (5)
internal/gitprovider/github/rest.gointernal/gitprovider/github/rest_test.gointernal/pipeline/diff_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
2 PR discussion threads considered. 2 summarized; 2 resolved.
Completed in 7m 12s | ~$3.16 (est.) | claude-opus-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-opus-5 |
| Reviewers | go:implementation-tests, structure:repo-health, architecture:solid-reviewer-agnostic |
| Engine | claude_cli · claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 7m 12s wall · 8m 10s compute |
| Cost | ~$3.16 (est.) |
| Pricing basis | anthropic-public-2026-09-02 |
| Tokens | 68 in / 21.5k out |
Per-workstream usage
go:implementation-tests— claude-opus-5- In: 12
- Out: 2.4k
- Cache read: 502.0k
- Cache create: 15.9k
- Cost: ~$0.47 (est.)
- Duration: 51s
structure:repo-health— claude-opus-5- In: 20
- Out: 5.5k
- Cache read: 888.8k
- Cache create: 17.4k
- Cost: ~$0.76 (est.)
- Duration: 1m 59s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 30
- Out: 12.4k
- Cache read: 2.1M
- Cache create: 28.5k
- Cost: ~$1.67 (est.)
- Duration: 4m 51s
orchestrator-rollup— claude-opus-5- In: 6
- Out: 1.2k
- Cache read: 323.7k
- Cache create: 7.2k
- Cost: ~$0.26 (est.)
- Duration: 28s
| changedFilePaths := reviewablePatchPaths(parsed.Patches) | ||
| selected = filterSelectedReviewerAssignment(selected, changedFilePaths) | ||
| // A reviewer may cite its own assignment plus unassignable paths, nothing else. | ||
| citableFiles := append(append([]string(nil), reviewerAssignmentScope(selected, changedFilePaths)...), mentionableExtraPaths(parsed.Patches)...) |
There was a problem hiding this comment.
U-I1, close call: the enforced acceptance set is now right — assignment scope plus unassignable paths — but nothing tells the reviewer the extras are citable, so the capability the last two commits restored is only reachable when a model disregards its stated contract. buildReviewerPrompt passes only reviewerAssignmentScope(selected, changedFiles) to findingsOutputContract (prompts.go:30, 719), which advertises it as allowed_values.changed_files and instructs "file_path must be one of changed_files" (prompts.go:705). A compliant reviewer that notices "deleting X breaks Y" will therefore self-censor the finding, because X is absent from the only allowed set it was shown — even though the gate at 2119 would now accept it. Both settled threads justified widening with "the path is visible in the diff the model reads", which is true, but the model is simultaneously told those paths are not allowed values.
Secondary symptom at this line: the assignment scope is now derived twice from the same inputs — here, and again inside buildReviewerPrompt — so the advertised set and the enforced set are independent computations that can drift, which is the same recomputation hazard recorded in the 783 thread.
Suggested fix (small, no signature churn beyond one parameter): pass the citable set into buildReviewerPrompt and have findingsOutputContract advertise the two roles distinctly — keep changed_files as the assignment scope so inspected_files/skipped_files obligations stay narrow, and add e.g. also_citable_files (the mentionableExtraPaths result) with a one-line instruction that file_path may be one of either. That makes the prompt describe what the gate enforces and removes the duplicate derivation.
Defensible either way, and not blocking: if the intent is a deliberately tolerant gate — accept such a finding when a model volunteers it, without inviting commentary on deleted code — then the narrow prompt is a reasonable choice and a sentence saying so at this line would settle it for the next reader. What would change my verdict is evidence that the narrow prompt is intentional rather than an oversight; the commit messages and thread replies argue the capability matters, which is why I read the silence as unfinished rather than chosen.
Reply inline to this comment.
| return llm.DecodeFindings(data, llm.FindingsOptions{ | ||
| KnownAgents: map[string]bool{agent.ID: true}, | ||
| ChangedFiles: stringSet(assignmentScope), | ||
| ChangedFiles: stringSet(citableFiles), |
There was a problem hiding this comment.
The citableFiles gate is the right shape for file_path, but llm.FindingsOptions.ChangedFiles is also the gate for inspected_files/skipped_files (contracts.go:264), and coverage accounting no longer has the scope guarantee it documents. Before this PR the gate was exactly assignmentScope, so result.InspectedFiles ⊆ scope held by construction; now it is scope + mentionableExtraPaths, and buildReviewerCoverage copies inspected files through without intersecting scope — entry.InspectedFiles = filterReviewableFiles(copySortedStrings(result.InspectedFiles)) at pipeline.go:2773, one line above entry.SkippedFiles = sortedIntersection(result.SkippedFiles, scope). The comment right above it (2770-2772) still asserts a reviewer 'doesn't emit an inspected file outside its scope', which is now false.
Impact is durable-output accuracy rather than correctness: a reviewer that lists a deleted path or a rename source in inspected_files gets it written into the coverage row, and reviewplan renders that row as 'inspected N assigned files' (internal/reviewplan/summary.go:325-331) — labelling as assigned a path the reviewer was deliberately never assigned, and inflating the cross-reviewer inspected union at summary.go:559-566. Approval logic is unaffected because coverageMissingFiles only subtracts from scope.
Fix: mirror the SkippedFiles line — entry.InspectedFiles = sortedIntersection(filterReviewableFiles(result.InspectedFiles), scope) — so findings may cite unassignable paths while coverage rows stay an obligation ledger, and the 2770-2772 comment becomes true again. If instead the extras are meant to show up in coverage, update that comment and the 'assigned' wording in summary.go so the two surfaces agree.
Reply inline to this comment.
Drop the fabricated "deleted file mode 100644" line from the reconstructed GitHub diff. The pull request files payload carries no mode, so the hardcoded value was concretely wrong for removed executables and symlinks. The parser already classifies a removal from the "+++ /dev/null" header alone, which is still emitted. Widen the selection decode gate to accept rename source paths. The gate was narrowed to reviewable head paths, but an orchestrator can legitimately cite a "rename from" path that is plainly visible in the diff it was given, and an unknown path fails the entire selection phase. Separate the reviewer's obligation set from its acceptance set. Assignments, prompt, and coverage stay on reviewable paths, but finding decode now accepts every changed path so that a finding on a deleted file, or a deleted path in skipped_files, no longer discards the reviewer's entire payload.
Accepting every changed path in the reviewer decode gate removed the only enforcement of an assignment at the output boundary, so a reviewer scoped to a subset could file findings on, and claim coverage for, files owned by another reviewer or excluded from its workspace. Restore the assignment scope as the acceptance base. Express the widening once instead of twice. A single helper returns the paths a model may cite but is never assigned - removed files and the pre-rename sources still visible in the diff - and both the selection and finding gates union it onto their obligation set, so the two cannot drift apart again.
8be2199 to
f7067e6
Compare
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: f7067e60da62
Profile: claude-reviewer-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 0 |
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: This head is the rebase of the previously reviewed content onto main@dfd9eee (which carries only #590, thread-artifact filename collisions, untouched by these files); the assigned diff is unchanged from the last reviewed revision and all prior findings are settled. Two structural observations remain deferred by agreement and are not re-raised: the ownerless reviewable-set concept recomputed at pipeline.go:783/1261/2068, and the singular/plural filterSelectedReviewerAssignment naming collision. Verified by running the tests:go test ./internal/pipelinepasses (72s). In./internal/gitprovider/github, every test passes except TestProductionImportsStayInAdapterLayer, which fails on this workbench because the checkout path contains a%3Aand itsgo listoutput parser expects a space...structure:repo-health— complete (constrained); skipped: none; constraints: No new findings: the structural concerns in this diff are already live review threads. Unresolved: coverage rows accepting inspected/skipped paths outside assignment scope (pipeline.go:2122) and the reviewer prompt not advertising the citable extras (pipeline.go:2071). Settled/declined: pipeline.... This head is the previous head rebased onto main at dfd9eee (an unrelated thread-artifact case-sensitivity fix); the PR's own content is unchanged, so only line numbers shifted by ~3. Verified with CGO_ENABLED=0 and GOTMPDIR/GOCACHE redirected out of the workbench (the default values point into another reviewer's scratch dir):go test ./internal/pipeline ./internal/gitprovider/github -run 'Diff\|Selection\|Reviewer\|Deletion\|Coverage'passes.architecture:solid-reviewer-agnostic— complete (constrained); skipped: none; constraints: Checked the rebase for interaction: #590 touches pipeline.go only at the thread-analysis log dir (1102) and ThreadAnalysisLog call (1336), neither of which meets the selection or reviewer decode paths this PR changes. Head f7067e6 is a rebase of the previously reviewed tree onto dfd9eee (#590). The PR diff for all five files is byte-identical to the last reviewed revision; only line numbers shifted. No findings filed. My one outstanding concern is already the open thread at pipeline.go:2071 (the prompt does not advertise the citable extras the gate accepts); pipeline.go:2122 is open from another reviewer. Re-filing either would duplicate an existing thread. Verified on this head with env-local TMPDIR/GOTMPDIR/GOCACHE and CGO_ENABLED=0 (inherited Go env pointed at a deleted sandbox dir): go build ./... ok, go vet clean, go test ./internal/pipeline ok (65s). internal/gitprovider/github fails only TestProductionImportsStayInAdapterLayer, which splitsgo listoutput on whitespace and chokes on the workbench path containing a space. Not diff-caused.
Inspected files (5)
internal/gitprovider/github/rest.gointernal/gitprovider/github/rest_test.gointernal/pipeline/diff_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 5m 03s | ~$7.14 (est.) | claude-opus-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-opus-5 |
| Reviewers | go:implementation-tests, structure:repo-health, architecture:solid-reviewer-agnostic |
| Engine | claude_cli · claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 5m 03s wall · 9m 43s compute |
| Cost | ~$7.14 (est.) |
| Pricing basis | anthropic-public-2026-09-02 |
| Tokens | 66 in / 15.8k out |
Per-workstream usage
go:implementation-tests— claude-opus-5- In: 22
- Out: 4.0k
- Cache read: 1.0M
- Cache create: 99.0k
- Cost: ~$1.59 (est.)
- Duration: 3m 07s
structure:repo-health— claude-opus-5- In: 14
- Out: 2.8k
- Cache read: 630.5k
- Cache create: 102.7k
- Cost: ~$1.41 (est.)
- Duration: 2m 09s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 24
- Out: 7.9k
- Cache read: 1.8M
- Cache create: 174.2k
- Cost: ~$2.85 (est.)
- Duration: 4m 04s
orchestrator-rollup— claude-opus-5- In: 6
- Out: 1.1k
- Cache read: 234.8k
- Cache create: 114.2k
- Cost: ~$1.29 (est.)
- Duration: 21s
2 participants
Summary
When GitHub's pull request diff endpoint is too large, codereview-cli reconstructs the diff from the paginated files endpoint. Patchless removed files were emitted only as a binary marker, so the parser treated large or binary deletions as modified files and reviewer assignment and coverage could expand to hundreds of deleted paths. This change preserves deletion metadata and keeps deleted paths out of reviewer obligations without narrowing what a model is allowed to talk about.
Changes
--- a/<old>and+++ /dev/nullwhen reconstructing a removed file without a patch, while retaining the existing binary marker behavior for other patchless files. The/dev/nullheader alone drives classification; no file mode is emitted, since the provider payload does not carry one and any fixed value would be wrong for removed executables and symlinks.mentionableExtraPaths— removed files and pre-rename sources, which are visible in the diff but never assigned. This keeps a single cited deleted or renamed path from rejecting an entire payload, while the reviewer finding gate stays scoped to that reviewer's assignment.Testing
go test ./internal/gitprovider/github ./internal/pipeline(331 tests)go test ./...(71 packages, all green)git diff --checkNew coverage: a selection citing a rename source, a reviewer finding on a deleted path, a reviewer finding on a rename source, and a reviewer finding outside its assignment being rejected. Each was verified by reverting the corresponding production change and confirming the test fails.
Scope
The change is limited to GitHub diff reconstruction and reviewer assignment bookkeeping. It does not change provider APIs or merge or deployment behavior.