fix(reviewer): account for changed files the prompt doesn't carry #90

Merged

rcsheets merged 2 commits from fix/context-completeness into main 2026-07-28 05:56:20 +00:00

Conversation 4 Commits 2 Files changed 7 +308 -23

rcsheets commented 2026-07-28 03:49:19 +00:00

Owner

Copy link

FullReviewPrompt told the model it had "the full contents of the changed files" unconditionally, and BuildUserMessage headed the section "Full contents of changed files". fetchChangedFiles guaranteed neither:

• it breaks out of the loop once MaxTotalFileChars (200k) is reached, dropping every remaining file;
• it continues past any FetchFile error, not just the deleted-on-head 404 its comment described.

Nothing marked the gap, so a large PR or one flaky Forgejo call silently shipped a subset under a header claiming the whole. Per-file truncation was already marked; whole-file omission was not, which is the worse of the two — a reviewer that believes it has every file reasons from absence ("nothing validates this", "this helper is unused") and states it as a finding.

Changes

• fetchChangedFiles returns the files it couldn't include, tagged with why: size budget, not on the head branch, or fetch failed (the last also logged, so lost fetches stop looking like deletions).
• New reviewer.OmittedFile / Request.OmittedFiles, rendered as a named list beside the contents section, with the header counting what's actually present (8 of 11 files).
• The system prompt now says a listed file is unknown, not absent, and not to conclude anything from not having seen it.
• The discovery pass gets the same field — it shares BuildUserMessage, and was otherwise liable to skip requesting a file it thought it had already seen.
• Drops "every directory the diff touches" from the discovery prompt: listings are capped at MaxListingDirs and unreadable directories are skipped, so "every" was never true either.

Tests cover the omission rendering (including the all-omitted case, where the note matters most, and quick tier where it must not appear) and fetchChangedFiles accounting for every path in the diff across all four outcomes.

🤖 Generated with Claude Code

`FullReviewPrompt` told the model it had *"the full contents of the changed files"* unconditionally, and `BuildUserMessage` headed the section *"Full contents of changed files"*. `fetchChangedFiles` guaranteed neither: - it `break`s out of the loop once `MaxTotalFileChars` (200k) is reached, dropping every remaining file; - it `continue`s past **any** `FetchFile` error, not just the deleted-on-head 404 its comment described. Nothing marked the gap, so a large PR or one flaky Forgejo call silently shipped a subset under a header claiming the whole. Per-file truncation was already marked; whole-file omission was not, which is the worse of the two — a reviewer that believes it has every file reasons from absence ("nothing validates this", "this helper is unused") and states it as a finding. **Changes** - `fetchChangedFiles` returns the files it couldn't include, tagged with why: size budget, not on the head branch, or fetch failed (the last also logged, so lost fetches stop looking like deletions). - New `reviewer.OmittedFile` / `Request.OmittedFiles`, rendered as a named list beside the contents section, with the header counting what's actually present (`8 of 11 files`). - The system prompt now says a listed file is *unknown*, not absent, and not to conclude anything from not having seen it. - The discovery pass gets the same field — it shares `BuildUserMessage`, and was otherwise liable to skip requesting a file it thought it had already seen. - Drops "every directory the diff touches" from the discovery prompt: listings are capped at `MaxListingDirs` and unreadable directories are skipped, so "every" was never true either. Tests cover the omission rendering (including the all-omitted case, where the note matters most, and quick tier where it must not appear) and `fetchChangedFiles` accounting for every path in the diff across all four outcomes. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

rcsheets added 1 commit 2026-07-28 03:49:19 +00:00

fix(reviewer): account for changed files the prompt doesn't carry ... 417df81c17

FullReviewPrompt told the model it had "the full contents of the changed
files" unconditionally, and BuildUserMessage headed the section "Full
contents of changed files". fetchChangedFiles guaranteed neither: it
`break`s out of the loop once MaxTotalFileChars is reached, dropping
every remaining file, and `continue`s past any FetchFile error at all —
not just the deleted-on-head 404 its comment described. Nothing marked
the gap, so a large PR or a flaky Forgejo call silently shipped a subset
under a header claiming the whole.

Per-file truncation was already marked; whole-file omission was not,
which is the worse of the two. A reviewer that believes it has every file
reasons from absence — "nothing validates this", "this helper is unused"
— and states it as a finding.

fetchChangedFiles now returns the files it couldn't include, tagged with
why (size budget, not on head, fetch failed, the last also logged). Those
reach the prompt as a named list next to the contents, with the header
counting what's present, and the system prompt now says a listed file is
unknown rather than absent. The discovery pass gets the same field, since
it shares BuildUserMessage and was otherwise liable to skip requesting a
file it thought it had already seen.

Also drops "every directory the diff touches" from the discovery prompt:
listings are capped at MaxListingDirs and unreadable directories are
skipped, so "every" was never true either.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

pr-reviewer-bot commented 2026-07-28 03:49:20 +00:00

Collaborator

Copy link

Automated review by pr-reviewer v0.41.3 | Safety Check | Claude Sonnet 5 | tracking id r-6826c0-5c48b3
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

---

⚠️ The primary review (Mistral Small) failed. The review below is from Claude Sonnet 5, which reviewed the same diff.

✅ Verdict: LGTM — Well-tested bug fix that closes a real gap (silent file omission) with consistent plumbing and no signs of introduced bugs or security issues.

Reviewed the diff. This is a focused, well-tested fix that:

• Changes fetchChangedFiles to no longer silently drop files past the budget or on fetch errors — now returns an OmittedFile list with reasons (budget/not-found/fetch-failed), and logs unexpected fetch errors instead of swallowing them.
• Plumbs OmittedFiles through Request, BuildUserMessage, and the discovery Inputs/Build path consistently.
• Updates prompt wording to stop overclaiming ("full contents"/"every directory") and instructs the model to treat omitted files as unknown rather than absent.
• Tests cover the omission rendering (including all-omitted and quick-tier-should-not-render cases) and the fetch accounting for all four outcomes (ok, budget-exhausted, 404, 500).

I checked the logic carefully:

• errors.Is(err, forgejo.ErrNotFound) distinguishes real 404s from other errors — reasonable assuming forgejo.FetchFile wraps/returns that sentinel on 404 (not shown in diff but plausible given usage).
• The budget-hit branch correctly appends the omitted file and continues instead of break, so all subsequent paths get accounted for; log message is only emitted once via budgetHit flag, which is fine.
• The header math (%d of %d) and rendering order (contents block, then omissions block) match what the tests assert.
• runner.go/discovery.go call sites are updated in step with the new return signature and field; no unused/mismatched wiring spotted.

No secrets, no accidentally committed files, no obvious breaking API changes beyond the intentional (and consistently updated) signature change to fetchChangedFiles. This looks like a solid bug fix with good test coverage.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.41.3 | Safety Check | Claude Sonnet 5 | tracking id `r-6826c0-5c48b3`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- > ⚠️ The primary review (**Mistral Small**) failed. The review below is from **Claude Sonnet 5**, which reviewed the same diff. **✅ Verdict: LGTM** — Well-tested bug fix that closes a real gap (silent file omission) with consistent plumbing and no signs of introduced bugs or security issues. Reviewed the diff. This is a focused, well-tested fix that: - Changes `fetchChangedFiles` to no longer silently drop files past the budget or on fetch errors — now returns an `OmittedFile` list with reasons (budget/not-found/fetch-failed), and logs unexpected fetch errors instead of swallowing them. - Plumbs `OmittedFiles` through `Request`, `BuildUserMessage`, and the discovery `Inputs`/`Build` path consistently. - Updates prompt wording to stop overclaiming ("full contents"/"every directory") and instructs the model to treat omitted files as unknown rather than absent. - Tests cover the omission rendering (including all-omitted and quick-tier-should-not-render cases) and the fetch accounting for all four outcomes (ok, budget-exhausted, 404, 500). I checked the logic carefully: - `errors.Is(err, forgejo.ErrNotFound)` distinguishes real 404s from other errors — reasonable assuming `forgejo.FetchFile` wraps/returns that sentinel on 404 (not shown in diff but plausible given usage). - The budget-hit branch correctly appends the omitted file and `continue`s instead of `break`, so all subsequent paths get accounted for; log message is only emitted once via `budgetHit` flag, which is fine. - The header math (`%d of %d`) and rendering order (contents block, then omissions block) match what the tests assert. - `runner.go`/`discovery.go` call sites are updated in step with the new return signature and field; no unused/mismatched wiring spotted. No secrets, no accidentally committed files, no obvious breaking API changes beyond the intentional (and consistently updated) signature change to `fetchChangedFiles`. This looks like a solid bug fix with good test coverage.

rcsheets force-pushed fix/context-completeness from 417df81c17 to 1463c6a25a 2026-07-28 04:45:23 +00:00 Compare

pr-reviewer-bot commented 2026-07-28 04:45:24 +00:00

Collaborator

Copy link

Automated review by pr-reviewer v0.41.5 | Safety Check | Claude Sonnet 5 | tracking id r-6833e4-4b2071
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

---

⚠️ The primary review (Mistral Small) failed. The review below is from Claude Sonnet 5, which reviewed the same diff.

✅ Verdict: LGTM — The PR is a targeted, well-tested fix for a real correctness gap (silently dropped file contents) with no signs of introduced bugs, secrets, or breaking changes.

The change is a well-scoped, self-consistent fix: fetchChangedFiles now returns omitted files with reasons instead of silently dropping them via break/bare continue, the prompts and message builder are updated to reflect this, and the new field is threaded through Request, Inputs, and runner.go consistently. Tests cover the omission rendering (including all-omitted and quick-tier-should-not-show cases) and the fetch accounting logic across all four outcomes (ok, budget, 404, other error).

Checked for logic issues:

• The budget-exhaustion loop change from break to continue with a budgetHit flag correctly still marks all remaining files as omitted rather than fetching them (verified against the test using a 5-file diff with big.go third-to-last).
• errors.Is(err, forgejo.ErrNotFound) distinction between deleted files and real fetch failures looks correct and is the core bug fix described.
• Wiring through runner.go, discovery.go, and reviewer.go is mechanical and consistent — no dropped parameters or mismatched signatures noticed.
• No secrets, credentials, or unrelated debug code in the diff.

Nothing here looks like it would break existing behavior or introduce a new vulnerability; it only adds transparency about content the model previously assumed it had.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.41.5 | Safety Check | Claude Sonnet 5 | tracking id `r-6833e4-4b2071`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- > ⚠️ The primary review (**Mistral Small**) failed. The review below is from **Claude Sonnet 5**, which reviewed the same diff. **✅ Verdict: LGTM** — The PR is a targeted, well-tested fix for a real correctness gap (silently dropped file contents) with no signs of introduced bugs, secrets, or breaking changes. The change is a well-scoped, self-consistent fix: `fetchChangedFiles` now returns omitted files with reasons instead of silently dropping them via `break`/bare `continue`, the prompts and message builder are updated to reflect this, and the new field is threaded through `Request`, `Inputs`, and `runner.go` consistently. Tests cover the omission rendering (including all-omitted and quick-tier-should-not-show cases) and the fetch accounting logic across all four outcomes (ok, budget, 404, other error). Checked for logic issues: - The budget-exhaustion loop change from `break` to `continue` with a `budgetHit` flag correctly still marks all remaining files as omitted rather than fetching them (verified against the test using a 5-file diff with `big.go` third-to-last). - `errors.Is(err, forgejo.ErrNotFound)` distinction between deleted files and real fetch failures looks correct and is the core bug fix described. - Wiring through `runner.go`, `discovery.go`, and `reviewer.go` is mechanical and consistent — no dropped parameters or mismatched signatures noticed. - No secrets, credentials, or unrelated debug code in the diff. Nothing here looks like it would break existing behavior or introduce a new vulnerability; it only adds transparency about content the model previously assumed it had.

rcsheets added 1 commit 2026-07-28 04:50:04 +00:00

fix(reviewer): label requested fragments by provenance ... 5cf2b0eae4

The discovery prompt now tells the model that a changed file listed as
omitted is one it hasn't seen and may request. That request is honoured —
acceptFragments only dedupes against the contents map, which an omitted
file is by definition absent from, so it passes through to the normal
fetch/extract path (covered by a new repocontext test).

What it came back under was wrong. The fragment section was headed
"Additional repository context (existing files — not changed in this
PR)", so acting on the invitation got the file relabelled as one the PR
doesn't touch — the same class of false statement the omission list was
added to prevent, and newly reachable, since before this the model had no
way to know those files existed to ask for.

The header now states only how the fragments got here, and each one
carries its own provenance: an existing file, or a changed file whose
full contents were omitted above.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

pr-reviewer-bot commented 2026-07-28 04:50:05 +00:00

Collaborator

Copy link

Automated review by pr-reviewer v0.41.5 | Safety Check | Claude Sonnet 5 | tracking id r-6834fd-30d3db
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

---

⚠️ The primary review (Mistral Small) failed. The review below is from Claude Sonnet 5, which reviewed the same diff.

✅ Verdict: LGTM — The PR correctly fixes the described silent-omission bug with consistent plumbing and solid test coverage; nothing alarming found.

Reviewed the diff. This is a focused, well-tested fix that:

• Changes fetchChangedFiles to continue accounting for remaining paths after the size budget is hit (via omitted list) instead of breaking silently, and distinguishes real fetch errors (errors.Is(err, forgejo.ErrNotFound)) from actual 404s, logging the former as a warning. This directly fixes the described bug and looks correct.
• Plumbs OmittedFile/OmittedFiles through Request, Inputs, reviewInputs, and the discovery pass consistently — all call sites updated (runner.go, discovery.go, reviewer.go).
• BuildUserMessage correctly guards the omission block behind Tier == TierFull, and the header math (%d of %d files) is correct given len(req.FileContents) + len(req.OmittedFiles).
• Provenance labeling for repo-context fragments (existing vs. omitted-changed-file) is handled with a straightforward map lookup, no obvious edge cases missed.
• Tests cover the new behavior thoroughly: all-omitted case, quick-tier suppression, fetch-failure vs. not-found distinction, budget exhaustion continuing to account for later paths, and the discovery pass not skipping already-omitted files.

No secrets, credentials, or accidental binaries. No breaking API changes beyond internal signature changes (all call sites updated in the same PR). Prompt wording changes are appropriately scoped and match the stated intent. Nothing here looks alarming or accidentally pushed.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.41.5 | Safety Check | Claude Sonnet 5 | tracking id `r-6834fd-30d3db`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- > ⚠️ The primary review (**Mistral Small**) failed. The review below is from **Claude Sonnet 5**, which reviewed the same diff. **✅ Verdict: LGTM** — The PR correctly fixes the described silent-omission bug with consistent plumbing and solid test coverage; nothing alarming found. Reviewed the diff. This is a focused, well-tested fix that: - Changes `fetchChangedFiles` to continue accounting for remaining paths after the size budget is hit (via `omitted` list) instead of `break`ing silently, and distinguishes real fetch errors (`errors.Is(err, forgejo.ErrNotFound)`) from actual 404s, logging the former as a warning. This directly fixes the described bug and looks correct. - Plumbs `OmittedFile`/`OmittedFiles` through `Request`, `Inputs`, `reviewInputs`, and the discovery pass consistently — all call sites updated (`runner.go`, `discovery.go`, `reviewer.go`). - `BuildUserMessage` correctly guards the omission block behind `Tier == TierFull`, and the header math (`%d of %d files`) is correct given `len(req.FileContents) + len(req.OmittedFiles)`. - Provenance labeling for repo-context fragments (existing vs. omitted-changed-file) is handled with a straightforward map lookup, no obvious edge cases missed. - Tests cover the new behavior thoroughly: all-omitted case, quick-tier suppression, fetch-failure vs. not-found distinction, budget exhaustion continuing to account for later paths, and the discovery pass not skipping already-omitted files. No secrets, credentials, or accidental binaries. No breaking API changes beyond internal signature changes (all call sites updated in the same PR). Prompt wording changes are appropriately scoped and match the stated intent. Nothing here looks alarming or accidentally pushed.

rcsheets force-pushed fix/context-completeness from 5cf2b0eae4 to 38e8990aed 2026-07-28 05:55:19 +00:00 Compare

pr-reviewer-bot commented 2026-07-28 05:55:20 +00:00

Collaborator

Copy link

Automated review by pr-reviewer v0.41.6 | Safety Check | Claude Sonnet 5 | tracking id r-684448-3167cd
This is an AI-generated review and may contain mistkaes.

Status: ✅ Completed

---

⚠️ The primary review (Mistral Small) failed. The review below is from Claude Sonnet 5, which reviewed the same diff.

✅ Verdict: LGTM — The change is a well-tested, internally consistent bugfix (file-omission tracking plus a legitimate regex-alternation fix) with no signs of secrets, security issues, or breaking changes.

This PR is a well-scoped bugfix with good test coverage. Walking through the substantive changes:

1. fetchChangedFiles behavior change: previously breaking on budget exhaustion (dropping remaining files silently) and continueing past any fetch error (treating all errors as the expected 404-deleted case). Now it continues through the budget (still dropping the files but recording why) and distinguishes ErrNotFound from other errors, logging the latter. This is a legitimate fix, not a behavioral regression — the files were already being dropped, this just makes the omission visible and observable. Confirmed via errors.Is(err, forgejo.ErrNotFound) — assumes FetchFile wraps/returns that sentinel; worth double-checking forgejo.FetchFile's error paths do so consistently (not shown in diff, but plausible given the PR's stated intent and the test using an httptest server returning 404 vs 500 to differentiate).

2. diffanalysis.go regex fixes: this is a nice, independently-justified fix for the alternation-precedence bug ((^|/)auth|oauth unintentionally applying the anchor to only the first branch). Tests explicitly cover both the false positives previously produced (e.g., internal/design, cmd/immigration) and confirm real matches still fire. Also fixes duplicate tag entries for a single file (e.g. multiple Helm patterns tagging the same path). These changes are pre-existing bug fixes bundled into this PR; scope creep, but low risk and well tested.

3. Prompt/message changes: BuildUserMessage now renders an omitted-files list and adjusts headers/wording to stop overclaiming "full contents". Tests cover the all-omitted case, the quick-tier exclusion, and fragment-provenance labeling for omitted-but-requested files. This looks internally consistent.

4. Discovery/runner wiring: OmittedFiles threaded through Inputs, Request, and buildContextBundle consistently; signature changes to fetchChangedFiles and buildContextBundle are updated at all call sites in runner.go.

No secrets, no obviously dangerous logic, no breaking API/service-down risk — this only affects prompt construction and file-omission bookkeeping, and if anything reduces the risk of the reviewer model making unsupported claims. The diff is internally consistent with its own tests, which are thorough for the new omission logic. I don't see anything alarming that shouldn't have been pushed.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.41.6 | Safety Check | Claude Sonnet 5 | tracking id `r-684448-3167cd`* *This is an AI-generated review and may contain mistkaes.* **Status:** ✅ Completed --- > ⚠️ The primary review (**Mistral Small**) failed. The review below is from **Claude Sonnet 5**, which reviewed the same diff. **✅ Verdict: LGTM** — The change is a well-tested, internally consistent bugfix (file-omission tracking plus a legitimate regex-alternation fix) with no signs of secrets, security issues, or breaking changes. This PR is a well-scoped bugfix with good test coverage. Walking through the substantive changes: 1. **`fetchChangedFiles` behavior change**: previously `break`ing on budget exhaustion (dropping remaining files silently) and `continue`ing past any fetch error (treating all errors as the expected 404-deleted case). Now it continues through the budget (still dropping the files but recording why) and distinguishes `ErrNotFound` from other errors, logging the latter. This is a legitimate fix, not a behavioral regression — the files were already being dropped, this just makes the omission visible and observable. Confirmed via `errors.Is(err, forgejo.ErrNotFound)` — assumes `FetchFile` wraps/returns that sentinel; worth double-checking `forgejo.FetchFile`'s error paths do so consistently (not shown in diff, but plausible given the PR's stated intent and the test using an httptest server returning 404 vs 500 to differentiate). 2. **`diffanalysis.go` regex fixes**: this is a nice, independently-justified fix for the alternation-precedence bug (`(^|/)auth|oauth` unintentionally applying the anchor to only the first branch). Tests explicitly cover both the false positives previously produced (e.g., `internal/design`, `cmd/immigration`) and confirm real matches still fire. Also fixes duplicate tag entries for a single file (e.g. multiple Helm patterns tagging the same path). These changes are pre-existing bug fixes bundled into this PR; scope creep, but low risk and well tested. 3. **Prompt/message changes**: `BuildUserMessage` now renders an omitted-files list and adjusts headers/wording to stop overclaiming "full contents". Tests cover the all-omitted case, the quick-tier exclusion, and fragment-provenance labeling for omitted-but-requested files. This looks internally consistent. 4. **Discovery/runner wiring**: `OmittedFiles` threaded through `Inputs`, `Request`, and `buildContextBundle` consistently; signature changes to `fetchChangedFiles` and `buildContextBundle` are updated at all call sites in `runner.go`. No secrets, no obviously dangerous logic, no breaking API/service-down risk — this only affects prompt construction and file-omission bookkeeping, and if anything reduces the risk of the reviewer model making unsupported claims. The diff is internally consistent with its own tests, which are thorough for the new omission logic. I don't see anything alarming that shouldn't have been pushed.

rcsheets merged commit 8ed8f340bd into main 2026-07-28 05:56:20 +00:00

rcsheets deleted branch fix/context-completeness 2026-07-28 05:56:21 +00:00

rcsheets referenced this pull request from a commit 2026-07-28 05:56:22 +00:00

Merge pull request 'fix(reviewer): account for changed files the prompt doesn't carry' (#90) from fix/context-completeness into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

brooktrails/pr-reviewer!90

No description provided.