Salvage: GitHub App review precision, pinned source context and evidence-based findings

[Hmbown]

Rescues 14eb885d4e off fix/github-review-quality-20260913, a branch that has had no PR since 2026-09-13 and that nobody had ever built.

Partially addresses #6135. See "Why this does not close it" below.

What it does

Gives the GitHub App reviewer pinned source context around each hunk and pushes it toward concrete, evidence-bearing findings instead of generic commentary. 5 files, +406/−25: a new crates/tui/src/tools/review_pr.rs, changes to review.rs and review_hunks.rs, one lib.rs wiring line, and docs/GITHUB_APP.md.

Why a cherry-pick and not a branch merge

Do not merge that branch. It carries a second, unrelated commit — 4517d294c0, a Computer Use 0.2.2 sync — which conflicts across ~8 crates/tui/plugins/computer-use/ paths. An earlier audit reported the branch as "zero conflicts"; that measured the whole-branch merge and is misleading as written.

The single sha applies clean, verified before picking:

git merge-tree --write-tree --merge-base=14eb885d4e^ origin/main 14eb885d4e   # exit 0
git diff --stat origin/main <resulting-tree>                                   # == the commit's own diffstat

Nothing rides along.

The part that mattered

The commit compiles but does not pass CI's clippy line:

error: this expression creates a reference which is immediately dereferenced by the compiler
   --> crates/tui/src/tools/review_pr.rs:625:63
    = note: `-D clippy::needless-borrow` implied by `-D warnings`

Fixed in the second commit here. A clean cherry-pick tells you the text applied; it tells you nothing about whether the result builds under the gates. This one had sat unbuilt for two days.

Why this does not close #6135

Two gaps, both worth someone's attention rather than being buried under a merge:

1. The negative control named in the issue body has no in-repo artifact. The issue calls for a compiler-backed RegistryMutation control; what exists is an assertion in a commit message. Until that control exists as something CI runs, the precision claim is unverified.
2. source_context fails quiet on the cases that need it most. It returns None when budget < 512, and on overflow it discards an already-built report including its unavailable_files counters. So the large PRs with the most surrounding context to lose get none, and emit no receipt saying so. That is a silent-failure shape, not a size limit.

#6135 stays open for both.

Evidence

cargo check -p codewhale-tui --all-features --locked        clean
cargo fmt --all -- --check                                  clean
cargo clippy --workspace --all-targets --all-features
  --locked  (CI's exact allow list)                         clean  (after the fix)
test result: ok. 60 passed; 0 failed   (tools::review, --test-threads=2)
check-blocking-calls-budget.py   624 sites / 180 files, within budget
check-dead-code-budget.py        PASS, 254 at budget

🤖 Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 3m 56s —— View job

---

Review complete

• Gather context (diff, changed files, CLAUDE.md/AGENTS.md rules)
• Review crates/tui/src/tools/review_pr.rs (new file, +317)
• Review crates/tui/src/tools/review.rs changes
• Review crates/tui/src/tools/review_hunks.rs changes
• Review crates/tui/src/lib.rs wiring
• Review docs/GITHUB_APP.md doc changes
• Check for correctness bugs, provider/model safety, reuse, tests, security
• Post final review

Note: cargo check/clippy/cargo test require an approval this sandboxed run couldn't obtain, so I did not independently re-execute the PR's build/test evidence — the review below is source-level. The PR description's own cargo check / cargo fmt / cargo clippy --locked / tools::review 60 passed results are the build evidence on record.

Summary

This is a well-scoped, well-tested rescue of a stale commit, and the second commit's clippy fix (&prompt → prompt in the new test) is correct and minimal. The new review_pr.rs is careful about the things that matter for a reviewer that ingests attacker-controlled PR content: it never checks out or executes PR code, only reads pinned Git blobs by exact object ID (context_blob rejects symlinks, oversized blobs, and non-regular modes — covered by source_context_never_follows_a_pinned_symlink), runs gh/git via argv arrays (no shell interpolation), and wraps the whole payload with "untrusted_repository_data": true per the updated system-prompt "Review standard" section, which explicitly tells the model not to follow instructions embedded in the diff/title/body. File selection for context (DiffHunks::paths()) comes from the existing BTreeMap-backed hunk parser rather than a new one, so it's deterministic — good rung-2 reuse, and it matches the module's own doc comment about not inventing a second parser. Provider/model routing (EPIC #2608) is untouched by this diff.

Two source-level findings, both low severity — posted inline:

1. crates/tui/src/tools/review_pr.rs:409 — when the assembled source_context report exceeds its char budget, the function returns None and discards the already-built unavailable_files/omitted_files counters with it. This is exactly the gap the PR body discloses under "Why this does not close Improve GitHub App review precision with pinned source context and evidence-based findings #6135" ("fails quiet on the cases that need it most"), so it's not new information to the author, but docs/GITHUB_APP.md's new claim that "the request explicitly records unavailable files and omitted context" doesn't hold in this branch — the largest PRs get silent repository_context: null with no receipt. Worth a doc caveat or a truncate-to-fit fix in a follow-up; not a blocker since Improve GitHub App review precision with pinned source context and evidence-based findings #6135 stays open for it by design.
2. crates/tui/src/lib.rs:8610 — std::env::current_dir()? is computed twice in run_review (once at line 8598 for collect_diff, again here for review_workspace) where one variable would do. Cosmetic; no correctness impact since nothing awaits between the two calls.

Nothing else stood out: plan_pr_review's byte-for-byte diff reconstruction check, the same_revision/ensure_current re-verification against gh pr view, and the 300-file remote/local fallback path all have direct test coverage for the edge cases that matter (truncated hunks, missing full-index binary metadata, shallow/ambiguous merge-base history, mid-review PR mutation). The new build_pr_review_prompts wrapper correctly moves the blocking Git reads for source_context into spawn_blocking for both the tool and CLI call sites, consistent with the repo's blocking-call convention (#6149) — previously this code path had no Git I/O to worry about, so this is the PR proactively avoiding introducing a violation rather than fixing an existing one.

[claude]

Confirms the gap the PR description itself calls out under "Why this does not close #6135": when the assembled report exceeds budget, this returns None and silently drops the already-computed unavailable_files/omitted_files counters along with it — the caller (build_pr_pass_prompt) then just sees repository_context: null, with no signal that context was dropped for being oversized rather than simply unneeded (small pass, no room to spare). docs/GITHUB_APP.md's new "Review evidence and precision" section says "The request explicitly records unavailable files and omitted context," which isn't true for this branch — the largest PRs (the ones the feature is meant to help with) get no context and no receipt saying why. Not a blocker for this PR since it's explicitly scoped out and #6135 stays open for it, but worth a one-line doc caveat, or truncating to fit instead of dropping wholesale, and worth a repository_context.omitted != None-vs-dropped-because-empty distinction. Fix this →

[claude]

Minor: std::env::current_dir()? is already computed a few lines above (line 8598) and passed to collect_diff. This adds a second, redundant syscall for the same value rather than reusing it — low risk here since nothing awaits between the two calls, but worth collapsing into one let workspace = std::env::current_dir()?; used for both.

[codewhale-agent]

Codewhale review

Complete review coverage: 1/1 passes, 5 file patches, sha256:9abd3bc0fabcd0708437cac2ab5d759a7cd6fc5d7d2f999a103eec6c84dce763.

Pass 1: Adds source_context/context_blob (pinned-head Git blob excerpts around each pass's hunks), threads them into per-pass PR prompts via a new async build_pr_review_prompts used by both the CLI and the review tool, adds DiffHunks::paths/ranges, and documents the behaviour. The Git side is defensively written (literal pathspecs, --full-tree, -- separator, mode/type/size validation, symlink and NUL rejection, no replace objects / lazy fetch, per-file error containment), so no correctness or security failure was found in the collection path itself. The defects that remain are in the report/None handling: the receipt that the docs promise is discarded exactly on the passes with the tightest budget.

Findings

• [WARNING] Context is dropped together with its availability receipt when the budget is tight (crates/tui/src/tools/review_pr.rs:409)
  Trigger: a pass whose remaining budget is small, i.e. plan.manifest.max_chars_per_pass - pass.manifest.diff_chars < 512 (review.rs:484-486), or any pass where the assembled report would not fit. At review_pr.rs:341-343 source_context returns None before reading anything, and at review_pr.rs:409 it discards an already-built report that holds the unavailable_files (line 356) and omitted_files (lines 351/399) counters. build_pr_pass_prompt then emits "repository_context": null (review.rs:495) and the only signal left is the static context_limit text (review.rs:496); nothing in the request says whether the head was missing, every blob was non-regular/oversized/binary, or the budget was simply too small. User-visible impact: on large passes exactly the model-side evidence this change exists to add is silently absent, and a maintainer reading a posted review cannot tell 'context was attempted and could not be collected' from 'context never ran', so the precision claim is unverifiable where it matters most. It also contradicts the new documentation, which states the request records unavailable files and omitted context (docs/GITHUB_APP.md, the new 'Review evidence and precision' section). Smallest useful fix: never return None once the head is valid — return a minimal receipt (head_sha, empty files, the two counters, plus a reason such as budget/no-commit) so the counters survive; keep within budget by emitting no lines. The budget < 512 gate can additionally be documented as a threshold rather than being indistinguishable from a collection failure.
• [INFO] omitted_files conflates two different causes of omitted context (crates/tui/src/tools/review_pr.rs:351)
  review_pr.rs:351 initialises omitted_files to paths.len() - selected, i.e. files beyond the MAX_CONTEXT_FILES = 32 cap, and review_pr.rs:399-400 then increments the same counter for files whose candidates all failed the per-file allowance. Trigger: a pass over 300 files where 32 are capped and 5 of the selected ones get no lines — the report says omitted_files: 37 and the reader cannot tell whether the pass was truncated by the file cap or by character starvation, which are different remediation (raise the cap vs. raise CODEWHALE_REVIEW_MAX_CHARS). Smallest useful fix: keep the two counts separate (e.g. omitted_files for the cap and starved_files for the budget) or add a short reason per entry.

Suggestions

• crates/tui/src/tools/review_pr.rs:409 — Replace the (... ).then_some(report) drop with a return that always keeps the receipt: when the report does not fit, return the same report with files emptied (or a trimmed per-file list) plus a reason field instead of None, and move the budget < 512 case (line 341) to the same receipt-shaped result. This keeps unavailable_files/omitted_files observable in the request, which is what the new docs section promises, at no extra model input cost.

Assessment

Pass 1: The added collection path is deliberately conservative: --literal-pathspecs plus -- prevents pathspec/option injection from attacker-controlled diff paths, --full-tree makes paths independent of cwd, ls-tree output is checked for exact path, regular-file mode, blob type, a 40-hex object id and a 128 KiB bound, symlinks/submodules are rejected, cat-file blob bypasses checkout filters, NUL bytes and non-UTF-8 fail closed, and every per-file failure is contained as an unavailable_files increment (so the tool and CLI cannot fail a review because the head is not fetched locally). The two findings above are the concrete residue; the first is the same silent-failure shape the PR description acknowledges as open #6135 item 2, and I agree it is a real defect rather than just a size limit, because the counters are discarded rather than merely reduced. No build, clippy, test or runtime verification was performed by this review; the PR body's cargo check/clippy/60 passed claims were not reproduced here. Open questions I could not settle from the supplied context: (1) whether review_pr::model_diff (used for the prompt's diff field and for diff_chars) can alter hunk headers or the file set, since source_context parses its hunks and its contains_line exclusions from the raw pass.diff — if it can, lines shown as 'supplementary, not in the diff' could disagree with the patch the model receives; the added tests only exercise a plain single-file diff where the two are equal. (2) whether any caller of the signature-changed pub(crate) fn build_pr_pass_prompt exists outside the diffed files (it is crate-internal, and the two in-repo call sites plus the test are updated in this diff). (3) lib.rs:8613 now resolves std::env::current_dir()? unconditionally for codewhale review --pr, so the CLI's context source is the process cwd; any failure degrades silently, which is intended but means context quality depends on how the CLI was launched.

---

Advisory review by Codewhale (codewhale review --pr 6240 --post, head 672a54a228522ad2d1ab63542c1b358425d8b91f). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

[codewhale-agent]

[WARNING] Context is dropped together with its availability receipt when the budget is tight

Trigger: a pass whose remaining budget is small, i.e. plan.manifest.max_chars_per_pass - pass.manifest.diff_chars < 512 (review.rs:484-486), or any pass where the assembled report would not fit. At review_pr.rs:341-343 source_context returns None before reading anything, and at review_pr.rs:409 it discards an already-built report that holds the unavailable_files (line 356) and omitted_files (lines 351/399) counters. build_pr_pass_prompt then emits "repository_context": null (review.rs:495) and the only signal left is the static context_limit text (review.rs:496); nothing in the request says whether the head was missing, every blob was non-regular/oversized/binary, or the budget was simply too small. User-visible impact: on large passes exactly the model-side evidence this change exists to add is silently absent, and a maintainer reading a posted review cannot tell 'context was attempted and could not be collected' from 'context never ran', so the precision claim is unverifiable where it matters most. It also contradicts the new documentation, which states the request records unavailable files and omitted context (docs/GITHUB_APP.md, the new 'Review evidence and precision' section). Smallest useful fix: never return None once the head is valid — return a minimal receipt (head_sha, empty files, the two counters, plus a reason such as budget/no-commit) so the counters survive; keep within budget by emitting no lines. The budget < 512 gate can additionally be documented as a threshold rather than being indistinguishable from a collection failure.

[codewhale-agent]

[INFO] omitted_files conflates two different causes of omitted context

review_pr.rs:351 initialises omitted_files to paths.len() - selected, i.e. files beyond the MAX_CONTEXT_FILES = 32 cap, and review_pr.rs:399-400 then increments the same counter for files whose candidates all failed the per-file allowance. Trigger: a pass over 300 files where 32 are capped and 5 of the selected ones get no lines — the report says omitted_files: 37 and the reader cannot tell whether the pass was truncated by the file cap or by character starvation, which are different remediation (raise the cap vs. raise CODEWHALE_REVIEW_MAX_CHARS). Smallest useful fix: keep the two counts separate (e.g. omitted_files for the cap and starved_files for the budget) or add a short reason per entry.

[codewhale-agent]

Replace the (... ).then_some(report) drop with a return that always keeps the receipt: when the report does not fit, return the same report with files emptied (or a trimmed per-file list) plus a reason field instead of None, and move the budget < 512 case (line 341) to the same receipt-shaped result. This keeps unavailable_files/omitted_files observable in the request, which is what the new docs section promises, at no extra model input cost.

[Hmbown]

Windows failure: cause, and a fix that keeps the coverage

Test (windows-latest) fails on this branch:

FAIL [0.586s] (9569/15288) codewhale-tui tools::review_pr::tests::source_context_is_bounded_line_exact_and_uses_literal_paths
panicked at crates\tui\src\tools\review_pr.rs:692:56:
called `Result::unwrap()` on an `Err` value: Os { code: 123, kind: InvalidFilename,
  message: "The filename, directory name, or volume label syntax is incorrect." }

Cause. The test creates a file literally named [literal]*.rs to prove pathspecs are passed literally rather than globbed. * is one of Windows' reserved filename characters (< > : " / \ | ? *), so std::fs::write cannot create it at all. The test was never runnable on Windows — it is not a regression, it arrived that way in 14eb885d4e, on a branch that had no PR and that nobody built.

I see a #[cfg(unix)] gate in progress in a shared worktree. That works, and I have deliberately not touched it — but it drops Windows coverage for the literal-pathspec guard permanently, and that guard exists because a glob interpretation silently reads the wrong file. Windows is where a path bug is most likely to differ.

Alternative that keeps all three platforms. Change one line:

let path = "[l]iteral-other.rs";

[ and ] are legal in NTFS filenames; only * ? < > : " / \ | are not. The property under test survives intact, because the decoy is still reachable by a glob interpretation:

• read literally, the pattern names this file: [l]iteral-other.rs
• read as a glob, [l] matches the single character l, so the pattern resolves to literal-other.rs — the existing "wrong glob match" decoy the test already creates two lines below

So assert!(!context.to_string().contains("wrong glob match")) still fails if anything globs, which is the whole point of the test. The nested/ relative-path decoy is unaffected.

Happy either way — flagging it because the cfg-gate is the kind of fix that is invisible later, and this guard is worth keeping on the platform where paths behave differently.

For the record, this is the second thing this salvaged commit needed that a clean cherry-pick could not reveal: it also failed CI's clippy on a needless borrow (fixed in 672a54a22). Both are the cost of a commit sitting unbuilt on a branch with no PR.

[claude]

Claude finished @Hmbown's task in 1m 27s —— View job

---

Review complete

• Gather context (diff, prior review threads, CLAUDE.md/AGENTS.md rules)
• Review the two new commits since the last review (672a54a clippy fix, 55fffb3 Windows test gate)
• Check whether Hmbown's suggested [l]iteral-other.rs alternative was applied vs. the #[cfg(unix)] gate
• Re-check outstanding findings from prior reviews
• Post final review

Note: cargo check/clippy/cargo test require an approval this sandboxed run couldn't obtain, so this is a source-level review of the two new commits — same limitation as the prior automated pass on this PR. The commit messages' own clippy --locked clean / 1 passed; 0 failed results are the build evidence on record.

Summary

This sync adds exactly two commits on top of the previously reviewed 068f595, both scoped fixes for CI failures that a clean cherry-pick couldn't surface:

1. 672a54a — serde_json::from_str(&prompt) → serde_json::from_str(prompt) in a test, since prompt is already &str. Correct, minimal, matches -D clippy::needless-borrow. No behavior change.
2. 55fffb3 — gates source_context_is_bounded_line_exact_and_uses_literal_paths with #[cfg(unix)], since the test's [literal]*.rs fixture filename contains *, which is illegal on NTFS (std::fs::write fails with InvalidFilename before any assertion runs). This correctly diagnoses the windows-latest failure and follows the existing precedent of the adjacent source_context_never_follows_a_pinned_symlink test, which is gated the same way for the same class of reason. The one-line doc comment explaining why is a nice touch.

One thing worth flagging, though not a blocker: this took the #[cfg(unix)] route rather than the alternative Hmbown described in the PR thread (renaming the fixture to [l]iteral-other.rs, which is legal on Windows and still exercises the literal-vs-glob distinction via the existing literal-other.rs decoy). Hmbown was explicit that either approach was fine, so this isn't wrong — but it does mean the literal-pathspec guard now has zero coverage on Windows permanently, which is the platform where a glob-vs-literal path bug is most likely to actually diverge. If Windows coverage for this guard is wanted later, the rename is still a live, low-cost option.

No correctness bugs, provider/model-safety issues, or security concerns in either commit — both are test-only changes with no production code touched. The two substantive gaps from the earlier review round (source_context's budget-overflow None silently dropping the unavailable_files/omitted_files receipt at review_pr.rs:409, and omitted_files conflating the file-cap and budget-starvation cases at review_pr.rs:351) are unchanged by this diff — expected, since the PR description scopes both out under "Why this does not close #6135" and neither commit here touches that code path.

[codewhale-agent]

Codewhale review

Complete review coverage: 1/1 passes, 5 file patches, sha256:18075e1c3b0067d58685d1b833da8d03d2e9359eb98cb024043439f2376a2831.

Pass 1: Cherry-pick adds pinned-head source context (review_pr::source_context/context_blob), a JSON pass-prompt builder (review::build_pr_review_prompts/build_pr_pass_prompt), DiffHunks::paths/ranges, the run_review wiring, and docs. I checked the new control flow, guards and budget arithmetic against the supplied diff and source context; I found no defect I can demonstrate, and one evidence-contract problem in the branch that discards an already-built context report.

Findings

• [INFO] Over-budget repository_context drops the unavailable/omitted counters it already built (crates/tui/src/tools/review_pr.rs:409)
  crates/tui/src/tools/review.rs::build_pr_pass_prompt passes the residual budget (plan.manifest.max_chars_per_pass - pass.manifest.diff_chars) to review_pr::source_context and puts its result in the request as "repository_context". source_context walks the post-image paths, increments "unavailable_files" for each context_blob failure and "omitted_files" for files that contributed no line, and then ends with (report.to_string().chars().count() <= budget).then_some(report) (line 409); when the serialized report is over budget the function returns None and every counter and excerpt already collected is thrown away, so the pass prompt carries "repository_context": null, which the prompt text describes only as "no source context could fit". A reader of the request therefore cannot distinguish "the pinned head was not available locally" (the case the tests cover at crates/tui/src/tools/review_pr.rs) from "context was collected and then dropped", and the new sentence in docs/GITHUB_APP.md ("The request explicitly records unavailable files and omitted context") is not true of that branch. Impact is on the change's own evidence contract rather than on diff coverage. I did not construct an input that overflows — each file's allowance is derived from the remaining budget, so overflow requires the per-entry +1 slack to accumulate across files — so treat reachability as unproven; the discarding branch itself is plain in the source, and the PR description already lists this as open gap #2. The budget < 512 early return at line 342 has the same all-or-nothing shape but no pre-built counters.

Suggestions

• crates/tui/src/tools/review_pr.rs:409 — Do not turn an over-budget report into a bare None. Keep the counters that were already computed — return the report with its files array emptied (or add an explicit "dropped"/"truncated" marker) so the pass prompt distinguishes 'no pinned head locally' from 'excerpts collected but not sent'; otherwise qualify the docs/GITHUB_APP.md sentence that promises the request records unavailable files and omitted context. This needs a judgement call on the receipt shape, so no literal replacement is proposed.

Assessment

Pass 1: No defect in the five changed files could be demonstrated by inspection. The new source_context/context_blob path validates the pinned commit id, reads objects only through git ls-tree/git cat-file (no checkout, no filters, no fetch, symlink/submodule modes rejected, size and text-ness bounded), never clips a line, excludes lines already present in the diff, and its output is embedded only as supplementary evidence; the hunk parser is reused rather than reimplemented, and both the tool and CLI paths now do this work off the async runtime via spawn_blocking with the tool mapping failure to ToolError::execution_failed. No build, clippy or test run was performed by this review, so the author's compile/test claims are unverified here. Open questions, not asserted defects: (1) crates/tui/src/lib.rs is the one file missing from the supplied source context, so I could not check whether std::env::current_dir() in run_review is the same workspace the PR source/diff was resolved against; if it is not, every context_blob call fails and the change silently yields only unavailable_files counts for all passes; (2) the pass prompt is now JSON, so the diff's newlines/quotes are escaped (roughly one extra character per line) and the whole-plan manifest plus the untrimmed PR description also ride in the same string, none of which is subtracted when the context budget is computed as max_chars_per_pass - pass.diff_chars — the plan's input accounting may under-describe the real request size, but no in-repo consumer enforcing prompt length was visible to me; (3) the tests added here were not executed, so the glob/literal-path, nested-workspace, symlink, binary and oversized-blob expectations are unverified.

---

Advisory review by Codewhale (codewhale review --pr 6240 --post, head 55fffb3dba729a0d522cf197012a2863f28e8509). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

[codewhale-agent]

[INFO] Over-budget repository_context drops the unavailable/omitted counters it already built

crates/tui/src/tools/review.rs::build_pr_pass_prompt passes the residual budget (plan.manifest.max_chars_per_pass - pass.manifest.diff_chars) to review_pr::source_context and puts its result in the request as "repository_context". source_context walks the post-image paths, increments "unavailable_files" for each context_blob failure and "omitted_files" for files that contributed no line, and then ends with (report.to_string().chars().count() <= budget).then_some(report) (line 409); when the serialized report is over budget the function returns None and every counter and excerpt already collected is thrown away, so the pass prompt carries "repository_context": null, which the prompt text describes only as "no source context could fit". A reader of the request therefore cannot distinguish "the pinned head was not available locally" (the case the tests cover at crates/tui/src/tools/review_pr.rs) from "context was collected and then dropped", and the new sentence in docs/GITHUB_APP.md ("The request explicitly records unavailable files and omitted context") is not true of that branch. Impact is on the change's own evidence contract rather than on diff coverage. I did not construct an input that overflows — each file's allowance is derived from the remaining budget, so overflow requires the per-entry +1 slack to accumulate across files — so treat reachability as unproven; the discarding branch itself is plain in the source, and the PR description already lists this as open gap #2. The budget < 512 early return at line 342 has the same all-or-nothing shape but no pre-built counters.

[codewhale-agent]

Do not turn an over-budget report into a bare None. Keep the counters that were already computed — return the report with its files array emptied (or add an explicit "dropped"/"truncated" marker) so the pass prompt distinguishes 'no pinned head locally' from 'excerpts collected but not sent'; otherwise qualify the docs/GITHUB_APP.md sentence that promises the request records unavailable files and omitted context. This needs a judgement call on the receipt shape, so no literal replacement is proposed.

[Hmbown]

Windows: restored rather than gated — 1c29c4423

55fffb3db got the diagnosis exactly right, and that diagnosis is what made the fix obvious: * is a reserved NTFS filename character, so std::fs::write(dir.path().join("[literal]*.rs")) returned InvalidFilename (os 123) before a single assertion ran. That test has never executed on Windows since it was written — this was not a regression.

I've added a commit on top rather than rewriting that one, because the analysis in it is worth keeping in the history.

What changed: the #[cfg(unix)] gate comes off and the fixture is spelled portably.

let path = "[l]iteral-other.rs";

Only NTFS-legal characters — the reserved set is < > : " / \ | ? *, and brackets are not in it — and it proves exactly the same property:

• read literally, the pathspec names that file;
• read as a glob, [l] matches the single character l and the pattern resolves to literal-other.rs, the "wrong glob match" decoy the test already creates two lines down.

So an implementation that globs still reads the decoy and still trips assert!(!context.to_string().contains("wrong glob match")). The nested/ relative-path decoy is unaffected.

Verified empirically rather than reasoned from the spec — with both files present:

$ ls
[l]iteral-other.rs
literal-other.rs
$ for m in [l]iteral-other.rs; do echo "$m"; done
literal-other.rs

Why not just take the gate. It works, and it is the smaller diff. But it gives up this guard on the one platform whose filename rules caused the failure — and path handling is precisely where Windows differs from the other two. A guard that exists because globbing silently reads the wrong file is worth keeping where paths behave differently.

Honest limits: I could not compile or run this locally. Both worktrees that hold this branch are occupied by other sessions — one is mid-release-build, the other holds live work — and I made this commit through git plumbing specifically so I would not write into either. CI runs all three platforms on this PR and is the verification. If Windows still fails, the gate is the right fallback and I will say so.

Thanks to whoever pushed 55fffb3db — finding os 123 is a much better root cause than "Windows is flaky", and the remedy was the only part worth changing.

[claude]

Claude finished @Hmbown's task in 1m 10s —— View job

---

Review complete

• Gather context (diff, prior review threads, CLAUDE.md/AGENTS.md rules)
• Review the one new commit since the last review (1c29c44)
• Verify it matches the alternative discussed in the PR thread
• Re-check outstanding findings from prior reviews
• Post final review

Note: cargo check/clippy/cargo test require an approval this sandboxed run couldn't obtain (confirmed by attempting cargo test -p codewhale-tui --lib tools::review_pr::tests::..., which was blocked), so this is again a source-level review — same limitation as every prior automated pass on this PR. This repo is also a shallow clone here, so git diff origin/main...HEAD has no local merge-base; the changed-file list and diffs from the earlier reviews are used instead.

Summary

1c29c44 is the only new commit on top of the previously reviewed 55fffb3/672a54a. It replaces the #[cfg(unix)] gate on source_context_is_bounded_line_exact_and_uses_literal_paths with the portable fixture Hmbown proposed in the PR thread, restoring Windows coverage for the literal-vs-glob guard:

• crates/tui/src/tools/review_pr.rs:678-690 — the fixture is renamed from [literal]*.rs to [l]iteral-other.rs. * is NTFS-reserved (< > : " / \ | ? *); [ and ] are not, so the file is now creatable on Windows. The property under test survives: read literally the pathspec names [l]iteral-other.rs; read as a glob, [l] matches the single character l, resolving to the pre-existing literal-other.rs decoy (review_pr.rs:701) whose content is "wrong glob match\n". So assert!(!context.to_string().contains("wrong glob match")) (review_pr.rs:723) still fails under any implementation that globs pathspecs instead of treating them literally — exactly the regression this test exists to catch. The nested/ relative-path decoy two lines below is untouched by the rename.
• This is precisely the alternative described in Hmbown's PR comment, verified against the actual test body rather than taken on description alone: the loop over budget in [512, 800, 1_024, 2_000] and all downstream assertions (bounded size, correct files[0].path, no decoy leakage, no oversized-line clipping, per-line text matching the source) are unchanged, so this is a pure fixture-name substitution with no semantic drift.
• No other lines in review_pr.rs, review.rs, review_hunks.rs, lib.rs, or docs/GITHUB_APP.md changed in this commit, so the two substantive findings from the earlier rounds are unchanged and still stand, unresolved by design (both are explicitly scoped out of this PR under "Why this does not close Improve GitHub App review precision with pinned source context and evidence-based findings #6135"):
  • crates/tui/src/tools/review_pr.rs:409 — an over-budget source_context report is discarded via .then_some(report), dropping the already-computed unavailable_files/omitted_files counters and yielding a bare None (repository_context: null) with no receipt distinguishing "context collected but dropped for size" from "context never attempted."
  • crates/tui/src/tools/review_pr.rs:351 — omitted_files conflates files dropped by the MAX_CONTEXT_FILES cap with files starved of budget, so the count alone can't tell a reader which remediation (raise the cap vs. raise the char budget) applies.
  • crates/tui/src/lib.rs:8610 — cosmetic double std::env::current_dir()? call, no correctness impact.

No new correctness bugs, provider/model-routing concerns (EPIC #2608 is untouched by this diff), security issues, or missing test coverage introduced by 1c29c44. This is a clean, minimal, test-only fix that does what it says.

[Hmbown]

Merging with one documented red: Codewhale review failed on provider spend, not code — pass 1/1 hit provider stop reason length after burning the full 65,536-token output budget on reasoning (job 104605859980, usage: 23,591 in / 65,536 out, publication: not_attempted). Every substantive gate is green: Test on macos/ubuntu/windows, Lint, Safety gate, npm smoke, DCO, link. Per the #6229 precedent this class of red does not block.